Skip to content

Commit

Permalink
udev,networkd: use the interface name as fallback basis for MAC and I…
Browse files Browse the repository at this point in the history
…Pv4LL seed

Fixes #3374. The problem is that we set MACPolicy=persistent (i.e. we would
like to generate persistent MAC addresses for interfaces which don't have a
fixed MAC address), but various virtual interfaces including bridges, tun/tap,
bonds, etc., do not not have the necessary ID_NET_NAME_* attributes and udev
would not assing the address and warn:
  Could not generate persistent MAC address for $name: No such file or directory

Basic requirements which I think a solution for this needs to satisfy:

1. No changes to MAC address generation for those cases which are currently
  handled successfully. This means that net_get_unique_predictable_data() must
  keep returning the same answer, which in turn means net_get_name() must keep
  returning the same answer. We can only add more things we look at with lower
  priority so that we start to cover cases which were not covered before.

2. Like 1, but for IPvLL seed and DHCP IAD. This is less important, but "nice
  to have".

3. Keep MACPolicy=persistent. If people don't want it, they can always apply
  local configuration, but in general stable MACs are a good thing. I have never
  seen anyone complain about that.

== Various approaches that have been proposed

=== #3374 (comment) (tomty89)
if !ID_BUS and INTERFACE, use INTERFACE

I think this almost does the good thing, but I don't see the reason to reject ID_BUS
(i.e. physical hardware). Stable MACs are very useful for physical hardware that has
no physical MAC.

=== #3374 (comment) (teg)
if (should_rename(device, true))

This means looking at name_assign_type. In particular for
NET_NAME_USER should_rename(..., true) returns true. It only returns false
for NET_NAME_PREDICTABLE. So this would cover stuff like br0, bond0, etc,
but would not cover lo and other devices with predictable names. That doesn't
make much sense.

But did teg mean should_rename() or !should_rename()?

=== #3374 (comment) (tomty89):
+ if (!should_rename(device, true))
+        return udev_device_get_sysname(device)

This covers only devices with NET_NAME_PREDICTABLE. Since the problem applies as
much to bridges and such, this isn't neough.

=== #3374 (comment)  (grafi-tt)
+        /* if the machine doesn't provide data about the device, use the ifname specified by userspace
+        * (this is the case when the device is virtual, e.g., bridge or bond) */
+        s = udev_device_get_sysattr_value(device, "name_assign_type");
+        if (s && safe_atou(s, &type) >= 0 && type == NET_NAME_USER)
+                return udev_device_get_sysname(device);

This does not cover bond0, vnet0, tun/tap and similar.
grafi-tt also proposes patching the kernel, but *not* setting name_assign_type
seems intentional in those cases, because the device name is a result of
enumeration, not set by the userspace.

=== #3374 (comment) (tomty89)
(also PR #11372)
- MACAddressPolicy=persistent

This break requirement 3. above. It would solve the immediate problem, but I
think the disruption is too big.

=== This patch

This patch means that we will set a "stable" MAC for pretty much any virtual
device by default, where "stable" means keyed off the machine-id and interface
name.

It seems like a big change, but we already did this for most physical devices.
Doing it also for virtual devices doesn't seem like a big issue. It will make
the setup and monitoring of virtualized networks slightly nicer. I don't think
anyone is depending on having the MAC address changed when those devices are
destoryed and recreated. If they do, they'd have to change MACAddressPolicy=.

== Implementation
net_get_name() is called from dhcp_ident_set_iaid() so I didn't change
net_get_name() like in grafi-tt's patch, but net_get_unique_predictable_data().

net_get_unique_predictable_data() is called from get_mac() in link-config.c
and sd_ipv4ll_set_address_seed(), so both of those code paths are affected
and will now get data in some cases where they errored out previously.

The return code is changed to -ENODATA since that gives a nicer error string.
  • Loading branch information
keszybz committed Jan 21, 2019
1 parent b0a28c2 commit 6d36464
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 11 deletions.
19 changes: 12 additions & 7 deletions src/libsystemd-network/network-internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "alloc-util.h"
#include "condition.h"
#include "conf-parser.h"
#include "device-util.h"
#include "dhcp-lease-internal.h"
#include "ether-addr-util.h"
#include "hexdecoct.h"
Expand Down Expand Up @@ -40,31 +41,35 @@ const char *net_get_name(sd_device *device) {

int net_get_unique_predictable_data(sd_device *device, uint64_t *result) {
size_t l, sz = 0;
const char *name = NULL;
const char *name;
int r;
uint8_t *v;

assert(device);

/* net_get_name() will return one of the device names based on stable information about the
* device. If this is not available, we fall back to using the device name. */
name = net_get_name(device);
if (!name)
return -ENOENT;
(void) sd_device_get_sysname(device, &name);
if (!name)
return log_device_debug_errno(device, SYNTHETIC_ERRNO(ENODATA),
"No stable identifying information found");

log_device_debug(device, "Using \"%s\" as stable identifying information", name);
l = strlen(name);
sz = sizeof(sd_id128_t) + l;
v = alloca(sz);

/* fetch some persistent data unique to this machine */
/* 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. */
/* Let's hash the machine ID plus the device name. We use
* a fixed, but originally randomly created hash key here. */
*result = htole64(siphash24(v, sz, HASH_KEY.bytes));

return 0;
}

Expand Down
7 changes: 3 additions & 4 deletions src/udev/net/link-config.c
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,7 @@ static bool mac_is_random(sd_device *device) {
return type == NET_ADDR_RANDOM;
}

static int get_mac(sd_device *device, bool want_random,
struct ether_addr *mac) {
static int get_mac(sd_device *device, bool want_random, struct ether_addr *mac) {
int r;

if (want_random)
Expand Down Expand Up @@ -459,7 +458,7 @@ int link_config_apply(link_config_ctx *ctx, link_config *config,
case MACPOLICY_PERSISTENT:
if (mac_is_random(device)) {
r = get_mac(device, false, &generated_mac);
if (r == -ENOENT) {
if (r == -ENODATA) {
log_warning_errno(r, "Could not generate persistent MAC address for %s: %m", old_name);
break;
} else if (r < 0)
Expand All @@ -470,7 +469,7 @@ int link_config_apply(link_config_ctx *ctx, link_config *config,
case MACPOLICY_RANDOM:
if (!mac_is_random(device)) {
r = get_mac(device, true, &generated_mac);
if (r == -ENOENT) {
if (r == -ENODATA) {
log_warning_errno(r, "Could not generate random MAC address for %s: %m", old_name);
break;
} else if (r < 0)
Expand Down

3 comments on commit 6d36464

@pasky
Copy link

@pasky pasky commented on 6d36464 Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the assumption that this would be a low-impact change is wrong, this apparently is impacting a lot of people as it's rolling out into the wild: https://bugzilla.suse.com/show_bug.cgi?id=1136600

@keszybz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't expected to be a low-impact change. If clearly impacts certain use cases. This was an answer to a long-standing bug with a hundred comments. And a lot of thought was put into the final solution.

The problem with the "predictable naming" schemes, and in this case "predictable macs", is that any change to the scheme, even if objectively for the better, cannot be safely introduced on existing installations. The answer to this is the naming-scheme settings. It seems to have been an error not to put this change under a naming-scheme qualifier.

@ShepardZhao
Copy link

@ShepardZhao ShepardZhao commented on 6d36464 Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when this will release to master branch. we have try to get latest version 254 it does not contain the above code. and yes we are facing exactly issue like "Could not generate persistent MAC address for $name: No such file or directory"

the version we used is ubuntu 16.04 and it seems they do not use above code as well

Please sign in to comment.