Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bridge: Fix hwaddr set race between us and udev #2380

Merged
merged 1 commit into from Jan 1, 2020

Conversation

liskin
Copy link
Contributor

@liskin liskin commented May 19, 2019

systemd and udev in their default configuration attempt to set a
persistent MAC address for network interfaces that don't have one
already (systemd-def-link). We set the address only after creating the
interface, so there is a race between us and udev. There are several
outcomes (that actually occur, this race is very much not a theoretical
one):

  • We set the address before udev gets to the networking rules, so udev
    sees /sys/devices/virtual/net/docker0/addr_assign_type = 3
    (NET_ADDR_SET). This means there's no need to assign a different
    address and everything is fine.

  • udev reads /sys/devices/virtual/net/docker0/addr_assign_type before
    we set the address, gets 1 (NET_ADDR_RANDOM), and proceeds to
    generate and set a persistent address.

    Old versions of udev (pre-v242, i.e. without udev-patch) would then
    fail to generate an address, spit out "Could not generate persistent
    MAC address for docker0: No such file or directory" (see udev-issue,
    and everything would be probably fine as well.

    Current version of udev (with udev-patch) will generate an address
    just fine and then race us setting it. As udev does more work than we,
    the most probable outcome is that udev will overwrite the address we
    set and possibly cause some trouble later on.

On a clean Debian Buster (from Vagrant) VM with systemd/udev 242 from
Debian Experimental, docker network create net1 up to net7 resulted
in 3 bridges having a 02:42: address and 4 bridges having a seemingly
random (actually generated from interface name) address. With systemd
241, the result would be all bridges having a 02:42:, but some "Could
not generate persistent MAC address for" messages in the log.

The fix is to revert the MAC address setting fix from 79b3e77,
as it is no longer necessary with current netlink (netlink-addr-add),
and set the address atomically when creating the bridge interface, not
after that.


Do note that a similar race happens when creating veth devices as well.
I wasn't able to reproduce getting a wrong (non-02:42:) address,
possibly because the address is set by docker later, maybe only after
the interface is moved to another network namespace (but I'm just
guessing here). Still, different timings result in various error
messages being logged ("link_config: could not get ethtool features for
vethd9c938e" and the like) depending on when the interface disappears
from the primary network namespace. I'm not sure how to fix this and I
don't intend to dig deeper into this.

systemd and udev in their default configuration attempt to set a
persistent MAC address for network interfaces that don't have one
already [systemd-def-link]. We set the address only after creating the
interface, so there is a race between us and udev. There are several
outcomes (that actually occur, this race is very much not a theoretical
one):

* We set the address before udev gets to the networking rules, so udev
  sees `/sys/devices/virtual/net/docker0/addr_assign_type = 3`
  (NET_ADDR_SET). This means there's no need to assign a different
  address and everything is fine.

* udev reads `/sys/devices/virtual/net/docker0/addr_assign_type` before
  we set the address, gets `1` (NET_ADDR_RANDOM), and proceeds to
  generate and set a persistent address.

  Old versions of udev (pre-v242, i.e. without [udev-patch]) would then
  fail to generate an address, spit out "Could not generate persistent
  MAC address for docker0: No such file or directory" (see [udev-issue],
  and everything would be probably fine as well.

  Current version of udev (with [udev-patch]) will generate an address
  just fine and then race us setting it. As udev does more work than we,
  the most probable outcome is that udev will overwrite the address we
  set and possibly cause some trouble later on.

On a clean Debian Buster (from Vagrant) VM with systemd/udev 242 from
Debian Experimental, `docker network create net1` up to `net7` resulted
in 3 bridges having a 02:42: address and 4 bridges having a seemingly
random (actually generated from interface name) address. With systemd
241, the result would be all bridges having a 02:42:, but some "Could
not generate persistent MAC address for" messages in the log.

The fix is to revert the MAC address setting fix from 79b3e77,
as it is no longer necessary with current netlink [netlink-addr-add],
and set the address atomically when creating the bridge interface, not
after that.

[systemd-def-link]: https://github.com/systemd/systemd/blob/a166cd3aacdbfd4df196bb4ca9f43cff19cf9fec/network/99-default.link
[udev-patch]: systemd/systemd@6d36464
[udev-issue]: systemd/systemd#3374
[netlink-addr-add]: vishvananda/netlink@7d9b424

...

Do note that a similar race happens when creating veth devices as well.
I wasn't able to reproduce getting a wrong (non-02:42:) address,
possibly because the address is set by docker later, maybe only after
the interface is moved to another network namespace (but I'm just
guessing here). Still, different timings result in various error
messages being logged ("link_config: could not get ethtool features for
vethd9c938e" and the like) depending on when the interface disappears
from the primary network namespace. I'm not sure how to fix this and I
don't intend to dig deeper into this.

Signed-off-by: Tomas Janousek <tomi@nomi.cz>
@liskin
Copy link
Contributor Author

liskin commented May 19, 2019

Additionally, the race makes it difficult to match on address in udev rules, causing trouble for NetworkManager: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/15#note_162509

@saadrahim
Copy link

Any updates on when this issue will be merged into master?

@scalp42
Copy link

scalp42 commented Dec 12, 2019

Seeing the same issues here:

Dec 12 19:40:19 buildkite-i-01b13667741b4ca36.shared-usw2-ci1 systemd-udevd[3851]: Could not generate persistent MAC address for veth5d07c35: No such file or directory
Dec 12 19:40:19 buildkite-i-01b13667741b4ca36.shared-usw2-ci1 systemd-udevd[3852]: link_config: autonegotiation is unset or enabled, the speed and duplex are not writable.
Dec 12 19:40:19 buildkite-i-01b13667741b4ca36.shared-usw2-ci1 systemd-udevd[3852]: Could not generate persistent MAC address for vethc949c65: No such file or directory
Dec 12 19:40:19 buildkite-i-01b13667741b4ca36.shared-usw2-ci1 containerd[1330]: time="2019-12-12T19:40:19.139873930Z" level=info msg="shim containerd-shim started" address="/containerd-shim/moby/e1d6c6e0be01e0063c8ad864fcc6846bb06397bb5b3d0c6188711445c0f0867d/shim.sock" debug=false pid=3860

@thaJeztah
Copy link
Member

ping @arkodg PTAL

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

Changes look good to me, thanks @liskin
ptal @mavenugo

@thaJeztah
Copy link
Member

@euanh @selansen ptal as well 🤗

Copy link
Collaborator

@selansen selansen left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants