On Fri, Mar 21, 2014 at 8:46 AM, Umut Tezduyar Lindskog <u...@tezduyar.com> wrote: > On Thu, Mar 20, 2014 at 7:16 PM, Tom Gundersen <t...@jklm.no> wrote: >> On Thu, Mar 20, 2014 at 6:52 PM, Tom Gundersen <t...@jklm.no> wrote: >>> On Wed, Mar 19, 2014 at 2:36 PM, Umut Tezduyar Lindskog >>> <umut.tezdu...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel