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().
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.
Thoughts?
>
>> (not tested)
>>
>> [You could then add back the requirement to sd_ipv4ll_start() that
>> set_address_seed() must have been called, and drop the call with
>> random data from within the library (as that would be dead code now).]
>>
>>> r = sd_ipv4ll_attach_event(link->ipv4ll, NULL, 0);
>>> if (r < 0)
>>> return r;
>>> diff --git a/src/network/networkd.h b/src/network/networkd.h
>>> index 0c01719..b998b62 100644
>>> --- a/src/network/networkd.h
>>> +++ b/src/network/networkd.h
>>> @@ -196,6 +196,7 @@ struct Link {
>>> char *ifname;
>>> char *state_file;
>>> struct ether_addr mac;
>>> + struct udev_device *udev_device;
>>>
>>> unsigned flags;
>>>
>>> diff --git a/src/shared/net-util.c b/src/shared/net-util.c
>>> index 50cfa2c..0b96032 100644
>>> --- a/src/shared/net-util.c
>>> +++ b/src/shared/net-util.c
>>> @@ -24,6 +24,9 @@
>>> #include <arpa/inet.h>
>>> #include <fnmatch.h>
>>>
>>> +#include "strv.h"
>>> +#include "siphash24.h"
>>> +#include "libudev-private.h"
>>> #include "net-util.h"
>>> #include "log.h"
>>> #include "utf8.h"
>>> @@ -31,6 +34,42 @@
>>> #include "conf-parser.h"
>>> #include "condition.h"
>>>
>>> +#define HASH_KEY
>>> SD_ID128_MAKE(d3,1e,48,fa,90,fe,4b,4c,9d,af,d5,d7,a1,b1,2e,8a)
>>> +
>>> +int net_get_unique_predictable_data(struct udev_device *device, uint64_t
>>> *result) {
>>> + size_t l, sz = 0;
>>> + const char *name, *field = NULL;
>>> + int r;
>>> + uint8_t *v;
>>> +
>>> + /* fetch some persistent data unique (on this machine) to this
>>> device */
>>> + FOREACH_STRING(field, "ID_NET_NAME_ONBOARD", "ID_NET_NAME_SLOT",
>>> "ID_NET_NAME_PATH", "ID_NET_NAME_MAC") {
>>> + name = udev_device_get_property_value(device, field);
>>> + if (name)
>>> + break;
>>> + }
>>> +
>>> + if (!name)
>>> + return -ENOENT;
>>> +
>>> + l = strlen(name);
>>> + sz = sizeof(sd_id128_t) + l;
>>> + v = alloca(sz);
>>> +
>>> + /* fetch some persistent data unique to this machine */
>>> + r = sd_id128_get_machine((sd_id128_t*) v);
>>> + if (r < 0)
>>> + return r;
>>> + memcpy(v + sizeof(sd_id128_t), name, l);
>>> +
>>> + /* Let's hash the machine ID plus the device name. We
>>> + * use a fixed, but originally randomly created hash
>>> + * key here. */
>>> + siphash24(result, v, sz, HASH_KEY.bytes);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> bool net_match_config(const struct ether_addr *match_mac,
>>> const char *match_path,
>>> const char *match_driver,
>>> diff --git a/src/shared/net-util.h b/src/shared/net-util.h
>>> index 99479e1..01a6b72 100644
>>> --- a/src/shared/net-util.h
>>> +++ b/src/shared/net-util.h
>>> @@ -62,3 +62,5 @@ int config_parse_ifalias(const char *unit, const char
>>> *filename, unsigned line,
>>> int ltype, const char *rvalue, void *data, void
>>> *userdata);
>>>
>>> int net_parse_inaddr(const char *address, unsigned char *family, void
>>> *dst);
>>> +
>>> +int net_get_unique_predictable_data(struct udev_device *device, uint64_t
>>> *result);
>>> diff --git a/src/systemd/sd-ipv4ll.h b/src/systemd/sd-ipv4ll.h
>>> index 0207006..2397d43 100644
>>> --- a/src/systemd/sd-ipv4ll.h
>>> +++ b/src/systemd/sd-ipv4ll.h
>>> @@ -42,6 +42,7 @@ int sd_ipv4ll_get_address(sd_ipv4ll *ll, struct in_addr
>>> *address);
>>> int sd_ipv4ll_set_callback(sd_ipv4ll *ll, sd_ipv4ll_cb_t cb, void
>>> *userdata);
>>> int sd_ipv4ll_set_mac(sd_ipv4ll *ll, const struct ether_addr *addr);
>>> int sd_ipv4ll_set_index(sd_ipv4ll *ll, int interface_index);
>>> +int sd_ipv4ll_set_address_seed (sd_ipv4ll *ll, uint64_t entropy);
>>> int sd_ipv4ll_start (sd_ipv4ll *ll);
>>> int sd_ipv4ll_stop (sd_ipv4ll *ll);
>>> void sd_ipv4ll_free (sd_ipv4ll *ll);
>>> diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c
>>> index d3f1aff..7049181 100644
>>> --- a/src/udev/net/link-config.c
>>> +++ b/src/udev/net/link-config.c
>>> @@ -303,36 +303,11 @@ static int get_mac(struct udev_device *device, bool
>>> want_random, struct ether_ad
>>> if (want_random)
>>> random_bytes(mac->ether_addr_octet, ETH_ALEN);
>>> else {
>>> - const char *name;
>>> uint8_t result[8];
>>> - size_t l, sz;
>>> - uint8_t *v;
>>> -
>>> - /* fetch some persistent data unique (on this machine) to
>>> this device */
>>> - name = udev_device_get_property_value(device,
>>> "ID_NET_NAME_ONBOARD");
>>> - if (!name) {
>>> - name = udev_device_get_property_value(device,
>>> "ID_NET_NAME_SLOT");
>>> - if (!name) {
>>> - name =
>>> udev_device_get_property_value(device, "ID_NET_NAME_PATH");
>>> - if (!name)
>>> - return -ENOENT;
>>> - }
>>> - }
>>>
>>> - l = strlen(name);
>>> - sz = sizeof(sd_id128_t) + l;
>>> - v = alloca(sz);
>>> -
>>> - /* fetch some persistent data unique to this machine */
>>> - r = sd_id128_get_machine((sd_id128_t*) v);
>>> + r = net_get_unique_predictable_data(device,
>>> (uint64_t*)result);
>>> if (r < 0)
>>> return r;
>>> - memcpy(v + sizeof(sd_id128_t), name, l);
>>> -
>>> - /* Let's hash the machine ID plus the device name. We
>>> - * use a fixed, but originally randomly created hash
>>> - * key here. */
>>> - siphash24(result, v, sz, HASH_KEY.bytes);
>>>
>>> assert_cc(ETH_ALEN <= sizeof(result));
>>> memcpy(mac->ether_addr_octet, result, ETH_ALEN);
>>> --
>>> 1.7.10.4
>>>
>>> _______________________________________________
>>> systemd-devel mailing list
>>> systemd-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
> _______________________________________________
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
