udev: set link alternative name if link is already up during rename #25221

enr0n · 2022-11-01T10:56:28Z

The motivation for this PR is that in Ubuntu we have seen an issue similar to #15445, where udev's interface renaming of a subordinate device races with mlx5_core's own configuration.

yuwata

If an interface is configured with a .link file that contains the following:

[Link]
NamePolicy=path
AlternativeNamesPolicy=onboard

then, even if the AlternativeNamesPolicy= does not have path, the name based on the path may be set as an alternative name of the interface. I do not think it is expected.

src/udev/udev-event.c

enr0n · 2022-11-01T16:46:33Z

If an interface is configured with a .link file that contains the following:
[Link]
NamePolicy=path
AlternativeNamesPolicy=onboard
then, even if the AlternativeNamesPolicy= does not have path, the name based on the path may be set as an alternative name of the interface. I do not think it is expected.

That is a good point. I will address this. Thank you for reviewing!

enr0n · 2022-11-02T16:46:29Z

I have taken a different approach with the latest commits, which I believe addresses your concern regarding the alternative names policy, since it will only restore the alternative name if it was deleted. PTAL.

src/udev/net/link-config.c

enr0n · 2022-11-16T19:33:32Z

I added a commit to skip setting old_name as an alternative name if the name_assign_type corresponding to old_name was NET_NAME_ENUM. I think this might be better than refusing names that start with eth, wlan, etc., but let me know if you still prefer that approach.

yuwata

Please add testcases for the change.

src/libsystemd/sd-netlink/netlink-util.c

enr0n · 2022-11-23T20:40:54Z

I have made the changes you suggested, and I added some tests. Thanks!

yuwata · 2022-11-30T02:24:50Z

Hmm, I am now confused.

If an interface foo has

Name=aaa
AlternativeNames=aaa bbb

Then, this PR makes the interface renamed to aaa with altnames foo and bbb. That's not expected.

enr0n · 2022-11-30T21:05:07Z

Yes, that is true. Can you please explain when you would expect the old name to be kept as an alternative name?

I can see that this logic was introduced by 434a348, and reading the commit message, I understand why the new name must be removed if it is currently an alternative name. However, I do not understand why the old name is set as an alternative name in that case.

enr0n · 2022-12-02T22:03:03Z

I pushed a version that does not ever swap old_name to an alternative name, as it seems this is not wanted in most cases. This approach would simplify things a bit.

I also added an additional commit that makes rename_netif() handle -EBUSY instead of avoiding the rename attempt if the device is already up, because it looks like that restriction will be removed in the mainline kernel soon (https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=bd039b5ea2a9).

yuwata · 2022-12-07T02:38:16Z

@enr0n Please (also?) implement a test in test/test-network/systemd-networkd-tests.py. If our CIs do not like the new test in C, then I think it is OK to drop it.

enr0n · 2022-12-07T22:36:34Z

For the test in systemd-networkd-tests.py, I just tried to cover some basic expectations for what happens when a device is renamed to a current alternative name. I couldn't think of a consistent way to trigger an error from there that would cause the rename to fail after deleting an alternative name. Let me know if that is OK or if you want something different from that test.

enr0n · 2022-12-12T17:44:05Z

I think the jammy autopkgtest failures are test bed errors and unrelated.

yuwata

Mostly good. Several minor comments.

src/libsystemd/sd-netlink/netlink-util.c

src/libsystemd/sd-netlink/test-netlink-util.c

test/test-network/systemd-networkd-tests.py

When configuring a link's alternative names, the link's new name to-be is not allowed to be included because interface renaming will fail if the new name is already present as an alternative name. However, rtnl_set_link_name will delete the conflicting alternative name before renaming the device, if necessary. Allow the new link name to be set as an alternative name before the device is renamed. This means that if the rename is later skipped (i.e. because the link is already up), then the name can at least still be present as an alternative name.

Commit 434a348 ("netlink: do not fail when new interface name is already used as an alternative name") added logic to set the old interface name as an alternative name, but only when the new name is currently an alternative name. This is not the desired outcome in most cases, and the important part of this commit was to delete the new name from the list of alternative names if necessary.

If a current alternative name is to be used to rename a network interface, the alternative name must be removed first. If interface renaming fails, restore the alternative name that was deleted if necessary.

Currently rename_netif() will not attempt to rename a device if it is already up, because the kernel will return -EBUSY unless live renaming is allowed on the device. This restriction will be removed in a future kernel version [1]. To cover both cases, always attempt to rename the interface and return 0 if we get -EBUSY. [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=bd039b5ea2a9

Add a test that verifies a deleted alternative name is restored on error in rtnl_set_link_name().

enr0n · 2022-12-15T15:46:06Z

Thanks for your review! I have addressed all comments in the latest push. PTAL.

yuwata · 2022-12-15T15:50:06Z

LGTM. Thank you!

enr0n · 2022-12-16T20:07:23Z

I think the jammy-ppc64el failure is unrelated; TEST-67-INTEGRITY is the failed test.

keszybz · 2023-02-10T16:30:30Z

Queued up for v252.5. There was a trivial conflict.

github-actions bot added the udev label Nov 1, 2022

yuwata requested changes Nov 1, 2022

View reviewed changes

src/udev/udev-event.c Outdated Show resolved Hide resolved

yuwata added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 1, 2022

enr0n force-pushed the nic-rename-fallback branch 2 times, most recently from fa6968f to caab516 Compare November 2, 2022 16:42

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 2, 2022

enr0n requested a review from yuwata November 7, 2022 15:51

yuwata requested changes Nov 7, 2022

View reviewed changes

src/udev/net/link-config.c Show resolved Hide resolved

yuwata added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 7, 2022

slyon mentioned this pull request Nov 7, 2022

Check if the name to set to an interface is already an altname (LP: #1994822) canonical/netplan#292

Open

enr0n force-pushed the nic-rename-fallback branch from caab516 to b0d3385 Compare November 16, 2022 19:29

enr0n requested a review from yuwata November 16, 2022 19:33

yuwata requested changes Nov 17, 2022

View reviewed changes

src/libsystemd/sd-netlink/netlink-util.c Outdated Show resolved Hide resolved

src/libsystemd/sd-netlink/netlink-util.c Outdated Show resolved Hide resolved

enr0n force-pushed the nic-rename-fallback branch from b0d3385 to 869ca90 Compare November 23, 2022 20:36

enr0n requested a review from yuwata November 23, 2022 20:41

enr0n force-pushed the nic-rename-fallback branch 4 times, most recently from 779b90c to 9e72b37 Compare November 28, 2022 17:41

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 28, 2022

yuwata added this to the v253 milestone Nov 28, 2022

enr0n force-pushed the nic-rename-fallback branch from 9e72b37 to f9c4bc1 Compare December 2, 2022 21:57

enr0n force-pushed the nic-rename-fallback branch from f9c4bc1 to ba32e5e Compare December 6, 2022 21:58

enr0n force-pushed the nic-rename-fallback branch 2 times, most recently from 74a12c3 to 3ca16af Compare December 7, 2022 22:32

keszybz added the please-review PR is ready for (re-)review by a maintainer label Dec 8, 2022

yuwata requested changes Dec 15, 2022

View reviewed changes

yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Dec 15, 2022

enr0n added 6 commits December 15, 2022 09:21

sd-netlink: restore altname on error in rtnl_set_link_name

4d60066

If a current alternative name is to be used to rename a network interface, the alternative name must be removed first. If interface renaming fails, restore the alternative name that was deleted if necessary.

sd-netlink: add a test for rtnl_set_link_name()

b338a8b

Add a test that verifies a deleted alternative name is restored on error in rtnl_set_link_name().

test-network: add a test for renaming device to current altname

f68f644

enr0n force-pushed the nic-rename-fallback branch from 3ca16af to f68f644 Compare December 15, 2022 15:44

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 15, 2022

yuwata approved these changes Dec 15, 2022

View reviewed changes

yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Dec 15, 2022

bluca approved these changes Dec 17, 2022

View reviewed changes

bluca added the needs-stable-backport label Dec 17, 2022

bluca merged commit 2c99e8c into systemd:main Dec 17, 2022

keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Dec 20, 2022

keszybz removed the needs-stable-backport label Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

udev: set link alternative name if link is already up during rename #25221

udev: set link alternative name if link is already up during rename #25221

enr0n commented Nov 1, 2022 •

edited

Loading

yuwata left a comment

enr0n commented Nov 1, 2022

enr0n commented Nov 2, 2022 •

edited

Loading

enr0n commented Nov 16, 2022

yuwata left a comment

enr0n commented Nov 23, 2022

yuwata commented Nov 30, 2022 •

edited

Loading

enr0n commented Nov 30, 2022

enr0n commented Dec 2, 2022

yuwata commented Dec 7, 2022

enr0n commented Dec 7, 2022

enr0n commented Dec 12, 2022

yuwata left a comment

enr0n commented Dec 15, 2022

yuwata commented Dec 15, 2022

enr0n commented Dec 16, 2022

keszybz commented Feb 10, 2023

udev: set link alternative name if link is already up during rename #25221

udev: set link alternative name if link is already up during rename #25221

Conversation

enr0n commented Nov 1, 2022 • edited Loading

yuwata left a comment

Choose a reason for hiding this comment

enr0n commented Nov 1, 2022

enr0n commented Nov 2, 2022 • edited Loading

enr0n commented Nov 16, 2022

yuwata left a comment

Choose a reason for hiding this comment

enr0n commented Nov 23, 2022

yuwata commented Nov 30, 2022 • edited Loading

enr0n commented Nov 30, 2022

enr0n commented Dec 2, 2022

yuwata commented Dec 7, 2022

enr0n commented Dec 7, 2022

enr0n commented Dec 12, 2022

yuwata left a comment

Choose a reason for hiding this comment

enr0n commented Dec 15, 2022

yuwata commented Dec 15, 2022

enr0n commented Dec 16, 2022

keszybz commented Feb 10, 2023

enr0n commented Nov 1, 2022 •

edited

Loading

enr0n commented Nov 2, 2022 •

edited

Loading

yuwata commented Nov 30, 2022 •

edited

Loading