[systemd-devel] [PATCH] V2 sd-ipv4ll: generate predictable addresses

Tom Gundersen teg at jklm.no
Fri Mar 21 03:45:39 PDT 2014


On Fri, Mar 21, 2014 at 8:46 AM, Umut Tezduyar Lindskog
<umut at tezduyar.com> wrote:
> On Thu, Mar 20, 2014 at 7:16 PM, Tom Gundersen <teg at jklm.no> wrote:
>> On Thu, Mar 20, 2014 at 6:52 PM, Tom Gundersen <teg at jklm.no> wrote:
>>> On Wed, Mar 19, 2014 at 2:36 PM, Umut Tezduyar Lindskog
>>> <umut.tezduyar at axis.com> wrote:
>>>> ---
>>>>  src/libsystemd-network/sd-ipv4ll.c |   86 +++++++++++++++++++++-------
>>>>  src/network/networkd-link.c        |   12 +++-
>>>>  src/network/networkd.h             |    1 +
>>>>  src/shared/net-util.c              |   39 +++++++++++++
>>>>  src/shared/net-util.h              |    2 +
>>>>  src/systemd/sd-ipv4ll.h            |    1 +
>>>>  src/udev/net/link-config.c         |   27 +--------
>>>>  7 files changed, 119 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/src/libsystemd-network/sd-ipv4ll.c b/src/libsystemd-network/sd-ipv4ll.c
>>>> index c6f6e01..fbe3efd 100644
>>>> --- a/src/libsystemd-network/sd-ipv4ll.c
>>>> +++ b/src/libsystemd-network/sd-ipv4ll.c
>>>> @@ -76,6 +76,8 @@ struct sd_ipv4ll {
>>>>          usec_t defend_window;
>>>>          int next_wakeup_valid;
>>>>          be32_t address;
>>>> +        struct random_data *random_data;
>>>> +        char *random_data_state;
>>>>          /* External */
>>>>          be32_t claimed_address;
>>>>          struct ether_addr mac_addr;
>>>> @@ -130,30 +132,27 @@ static int ipv4ll_stop(sd_ipv4ll *ll, int event) {
>>>>          return 0;
>>>>  }
>>>>
>>>> -static be32_t ipv4ll_pick_address(sd_ipv4ll *ll) {
>>>> +static int ipv4ll_pick_address(sd_ipv4ll *ll, be32_t *address) {
>>>>          be32_t addr;
>>>> +        int r;
>>>> +        int32_t random;
>>>>
>>>>          assert(ll);
>>>> +        assert(address);
>>>> +        assert(ll->random_data);
>>>>
>>>> -        if (ll->address) {
>>>> -                do {
>>>> -                        uint32_t r = random_u32() & 0x0000FFFF;
>>>> -                        addr = htonl(IPV4LL_NETWORK | r);
>>>> -                } while (addr == ll->address ||
>>>> -                        (ntohl(addr) & IPV4LL_NETMASK) != IPV4LL_NETWORK ||
>>>> -                        (ntohl(addr) & 0x0000FF00) == 0x0000 ||
>>>> -                        (ntohl(addr) & 0x0000FF00) == 0xFF00);
>>>> -        } else {
>>>> -                uint32_t a = 1;
>>>> -                int i;
>>>> -
>>>> -                for (i = 0; i < ETH_ALEN; i++)
>>>> -                        a += ll->mac_addr.ether_addr_octet[i]*i;
>>>> -                a = (a % 0xFE00) + 0x0100;
>>>> -                addr = htonl(IPV4LL_NETWORK | (uint32_t) a);
>>>> -        }
>>>> +        do {
>>>> +                r = random_r(ll->random_data, &random);
>>>> +                if (r < 0)
>>>> +                        return r;
>>>> +                addr = htonl((random & 0x0000FFFF) | IPV4LL_NETWORK);
>>>> +        } while (addr == ll->address ||
>>>> +                (ntohl(addr) & IPV4LL_NETMASK) != IPV4LL_NETWORK ||
>>>> +                (ntohl(addr) & 0x0000FF00) == 0x0000 ||
>>>> +                (ntohl(addr) & 0x0000FF00) == 0xFF00);
>>>>
>>>> -        return addr;
>>>> +        *address = addr;
>>>> +        return 0;
>>>>  }
>>>>
>>>>  static int ipv4ll_timer(sd_event_source *s, uint64_t usec, void *userdata) {
>>>> @@ -306,7 +305,9 @@ static void ipv4ll_run_state_machine(sd_ipv4ll *ll, IPv4LLTrigger trigger, void
>>>>                          ll->claimed_address = 0;
>>>>
>>>>                          /* Pick a new address */
>>>> -                        ll->address = ipv4ll_pick_address(ll);
>>>> +                        r = ipv4ll_pick_address(ll, &ll->address);
>>>> +                        if (r < 0)
>>>> +                                goto out;
>>>>                          ll->conflict++;
>>>>                          ll->defend_window = 0;
>>>>                          ipv4ll_set_state(ll, IPV4LL_STATE_WAITING_PROBE, 1);
>>>> @@ -435,6 +436,36 @@ int sd_ipv4ll_get_address(sd_ipv4ll *ll, struct in_addr *address){
>>>>          return 0;
>>>>  }
>>>>
>>>> +int sd_ipv4ll_set_address_seed (sd_ipv4ll *ll, uint64_t entropy) {
>>>> +        int r;
>>>> +
>>>> +        assert_return(ll, -EINVAL);
>>>> +
>>>> +        free(ll->random_data);
>>>> +        free(ll->random_data_state);
>>>> +
>>>> +        ll->random_data = new0(struct random_data, 1);
>>>> +        ll->random_data_state = new0(char, 128);
>>>> +
>>>> +        if (!ll->random_data || !ll->random_data_state) {
>>>> +                r = -ENOMEM;
>>>> +                goto error;
>>>> +        }
>>>> +
>>>> +        r = initstate_r((unsigned int)entropy, ll->random_data_state, 128, ll->random_data);
>>>> +        if (r < 0)
>>>> +                goto error;
>>>> +
>>>> +error:
>>>> +        if (r < 0){
>>>> +                free(ll->random_data);
>>>> +                free(ll->random_data_state);
>>>> +                ll->random_data = NULL;
>>>> +                ll->random_data_state = NULL;
>>>> +        }
>>>> +        return r;
>>>> +}
>>>> +
>>>>  int sd_ipv4ll_start (sd_ipv4ll *ll) {
>>>>          int r;
>>>>
>>>> @@ -453,8 +484,17 @@ int sd_ipv4ll_start (sd_ipv4ll *ll) {
>>>>          ll->defend_window = 0;
>>>>          ll->claimed_address = 0;
>>>>
>>>> -        if (ll->address == 0)
>>>> -                ll->address = ipv4ll_pick_address(ll);
>>>> +        if (!ll->random_data) {
>>>> +                r = sd_ipv4ll_set_address_seed(ll, random_u64());
>>>> +                if (r < 0)
>>>> +                        goto out;
>>>> +        }
>>>> +
>>>> +        if (ll->address == 0) {
>>>> +                r = ipv4ll_pick_address(ll, &ll->address);
>>>> +                if (r < 0)
>>>> +                        goto out;
>>>> +        }
>>>>
>>>>          ipv4ll_set_state (ll, IPV4LL_STATE_INIT, 1);
>>>>
>>>> @@ -493,6 +533,8 @@ void sd_ipv4ll_free (sd_ipv4ll *ll) {
>>>>          sd_ipv4ll_stop(ll);
>>>>          sd_ipv4ll_detach_event(ll);
>>>>
>>>> +        free(ll->random_data);
>>>> +        free(ll->random_data_state);
>>>>          free(ll);
>>>>  }
>>>>
>>>> diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
>>>> index fdc351f..7a589a5 100644
>>>> --- a/src/network/networkd-link.c
>>>> +++ b/src/network/networkd-link.c
>>>> @@ -72,6 +72,8 @@ int link_new(Manager *manager, struct udev_device *device, Link **ret) {
>>>>          if (r < 0)
>>>>                  return r;
>>>>
>>>> +        link->udev_device = udev_device_ref(device);
>>>> +
>>>>          *ret = link;
>>>>          link = NULL;
>>>>
>>>> @@ -94,6 +96,8 @@ void link_free(Link *link) {
>>>>          free(link->ifname);
>>>>          free(link->state_file);
>>>>
>>>> +        udev_device_unref(link->udev_device);
>>>> +
>>>>          free(link);
>>>>  }
>>>>
>>>> @@ -893,10 +897,16 @@ static int link_acquire_conf(Link *link) {
>>>>
>>>>          if (link->network->ipv4ll) {
>>>>                  if (!link->ipv4ll) {
>>>> +                        uint64_t seed = 0;
>>>>                          r = sd_ipv4ll_new(&link->ipv4ll);
>>>>                          if (r < 0)
>>>>                                  return r;
>>>> -
>>>> +                        r = net_get_unique_predictable_data(link->udev_device, &seed);
>>>> +                        if (r >= 0) {
>>>> +                                r = sd_ipv4ll_set_address_seed(link->ipv4ll, seed);
>>>> +                                if (r < 0)
>>>> +                                        return r;
>>>> +                        }
>>>
>>> I thought a bit more about this case, and I think we should try a bit
>>> harder here to get a predictable seed.
>>>
>>> The situation I have in mind is in the container, where we will not
>>> have any udev properties available, so
>>> net_get_unique_predictable_data() will always fail. However, we may
>>> still have a predictable mac address. Or it could be random of course,
>>> we don't know, but at this point the alternative is random data
>>> anyway, so might as well use the mac address. At least for the devices
>>> we create ourselves from nspawn the address will be a predictable one.
>>>
>>> So I'd suggest replacing the if block above with
>>>
>>> if (r < 0) {
>>>         siphash24((uint64_t*)&seed, ETH_ALEN,
>>> &link->mac.ether_addr_octet, HASH_KEY.bytes)
>>> }
>>
>> Actually, as the mac address is available also inside the library,
>> maybe best to simply put this logic inside the library so the users
>> don't have to care. I.e., leave the caller in networkd as you have it
>> now, just replace the use of random_u32() when the seed is not set
>> inside the library with a hash of the mac. What do you think?
>
> I am fine with it but we also need a solution in case there is an
> address collision and we need to pick up a new address. I don't think
> we can get rid of random_32().

Yeah, I didn't mean to use the mac address directly, only as the seed
for the address generation in case a seed has not been set externally.

> I have also been thinking and I am having yet another propose
> (inspired by the discussion). We have to have a unique MAC for ipv4ll
> to work anyways and without setting MAC, ipv4ll will not start. How
> about as soon as MAC is set, we siphash it and then initialize
> initstate_r in the library. This will drop the need of
> sd_ipv4ll_set_address_seed API and I think simplify things a lot.

I think we should still support the sd_ipv4ll_set_address_seed() API,
but fall back to using the mac address as you outline.

The reason is that we don't only want a unique seed (which the MAC
address must be), but if at all possible we want one which is
persistent between reboots. MAC addresses are not guaranteed to be
persistent, so I think we should only rely on them as a fallback.

Cheers,

Tom


More information about the systemd-devel mailing list