TEST-85-NETWORK-NetworkdSRIOVTests failed in GitHub Actions

[yuwata]

https://github.com/systemd/systemd/actions/runs/9137583438/job/25127740138?pr=32907

gh run download 9137583438 --name ci-mkosi-9137583438-1-arch-rolling-failed-test-journals -D ci/ci-mkosi-9137583438-1-arch-rolling-failed-test-journals

[yuwata]

Hm. eth0 is not renamed...

May 18 14:46:47 H systemd-udevd[725]: eth0: Device ready for processing (SEQNUM=2659, ACTION=add)
May 18 14:46:47 H (udev-worker)[786]: eth0: Processing device (SEQNUM=2659, ACTION=add)
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/50-udev-default.rules:31 Importing properties from results of builtin command 'net_driver'
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/75-net-description.rules:6 Importing properties from results of builtin command 'hwdb 'net:naming:drnetdevsim:''
May 18 14:46:47 H (udev-worker)[786]: eth0: No entry found from hwdb.
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/75-net-description.rules:6 Failed to run builtin 'hwdb 'net:naming:drnetdevsim:'': No data available
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/75-net-description.rules:8 Importing properties from results of builtin command 'net_id'
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/80-net-setup-link.rules:5 Importing properties from results of builtin command 'path_id'
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/80-net-setup-link.rules:5 Failed to run builtin 'path_id': No such file or directory
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/80-net-setup-link.rules:9 Importing properties from results of builtin command 'net_setup_link'
May 18 14:46:47 H (udev-worker)[786]: eth0: Failed to query name_assign_type: Invalid argument
May 18 14:46:47 H (udev-worker)[786]: eth0: Failed to get "name_assign_type" attribute, ignoring: Invalid argument
May 18 14:46:47 H (udev-worker)[786]: eth0: Device has addr_assign_type=1
May 18 14:46:47 H (udev-worker)[786]: eth0: Config file /run/systemd/network/25-sriov.link is applied
May 18 14:46:47 H (udev-worker)[786]: eth0: Using "eth0" as stable identifying information
May 18 14:46:47 H (udev-worker)[786]: eth0: Applying persistent MAC address: c2:97:67:6e:5c:47
May 18 14:46:47 H systemd-udevd[725]: eth0: sd-device-monitor(manager): Passed 191 byte to netlink monitor.
May 18 14:46:47 H systemd-udevd[725]: netdevsim99: SEQNUM=2662 blocked by SEQNUM=2658
May 18 14:46:47 H (udev-worker)[786]: eth0: Policies didn't yield a name and Name= is not given, not renaming.
May 18 14:46:47 H (udev-worker)[786]: eth0: device/sriov_numvfs sysfs attribute set to '3'.
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/80-net-setup-link.rules:11 NAME 'eth0'
May 18 14:46:47 H (udev-worker)[786]: eth0: /usr/lib/udev/rules.d/99-systemd.rules:69 RUN '/usr/lib/systemd/systemd-sysctl --prefix=/net/ipv4/conf/$name --prefix=/net/ipv4/neigh/$name --prefix=/net/ipv6/conf/$name --prefix=/net/ipv6/nei>
May 18 14:46:47 H (udev-worker)[786]: eth0: sd-device: Created database file '/run/udev/data/n17' for '/devices/netdevsim99/net/eth0'.
May 18 14:46:47 H (udev-worker)[786]: eth0: Running command "/usr/lib/systemd/systemd-sysctl --prefix=/net/ipv4/conf/eth0 --prefix=/net/ipv4/neigh/eth0 --prefix=/net/ipv6/conf/eth0 --prefix=/net/ipv6/neigh/eth0"
May 18 14:46:47 H (udev-worker)[786]: eth0: Starting '/usr/lib/systemd/systemd-sysctl --prefix=/net/ipv4/conf/eth0 --prefix=/net/ipv4/neigh/eth0 --prefix=/net/ipv6/conf/eth0 --prefix=/net/ipv6/neigh/eth0'
May 18 14:46:47 H (udev-worker)[786]: Successfully forked off '(spawn)' as PID 789.
May 18 14:46:47 H (udev-worker)[786]: eth0: Process '/usr/lib/systemd/systemd-sysctl --prefix=/net/ipv4/conf/eth0 --prefix=/net/ipv4/neigh/eth0 --prefix=/net/ipv6/conf/eth0 --prefix=/net/ipv6/neigh/eth0' succeeded.
May 18 14:46:47 H (udev-worker)[786]: eth0: sd-device: Created database file '/run/udev/data/n17' for '/devices/netdevsim99/net/eth0'.
May 18 14:46:47 H (udev-worker)[786]: eth0: Device processed (SEQNUM=2659, ACTION=add)
May 18 14:46:47 H (udev-worker)[786]: eth0: sd-device-monitor(worker): Passed 368 byte to netlink monitor.
(snip)
May 18 14:46:47 H (udev-worker)[784]: netdevsim99: Processing device (SEQNUM=2662, ACTION=bind)

[bluca]

This seems to happen only on archlinux, I don't think I've seen it on other jobs, so it might be due to some distro-specific configuration/setup

[yuwata]

Or kernel bug?

[yuwata]

From https://github.com/systemd/systemd/actions/runs/9150396388/job/25155113839?pr=32928 for PR #32928, this is a regression in kernel 6.9.
Fedora 40 (kernel 6.8) : PASS
Fedora rawhide (kernel 6.9) : FAIL

[yuwata]

Ah, caused by torvalds/linux@8debcf5.

systemd-udevd checks if a network interface is a stacked netdev or not with iflink sysattr. If it is 0, then assume it is a stacked device. If equivalent to ifindex value, then assume it is not stacked.
However, after the commit, iflink of netdevsim will never be equivalent to ifindex...

[bluca]

Another kernel API break, nice! We are raking up quite the collection

[yuwata]

Yeah, it should return ifindex rather than 0 if there is no peer.

[yuwata]

@spikeh and @davem330

I think the commit torvalds/linux@8debcf5 should be fixed something like the following.

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index c22897bf5509..8b8a5532a33d 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -324,7 +324,7 @@ static int nsim_get_iflink(const struct net_device *dev)
 
        rcu_read_lock();
        peer = rcu_dereference(nsim->peer);
-       iflink = peer ? READ_ONCE(peer->netdev->ifindex) : 0;
+       iflink = peer ? READ_ONCE(peer->netdev->ifindex) : READ_ONCE(dev->ifindex);
        rcu_read_unlock();
 
        return iflink;

[spikeh]

What is the expectation of systemd-udevd? netdevsim has the same behaviour as veth:

static int veth_get_iflink(const struct net_device *dev)
{
	struct veth_priv *priv = netdev_priv(dev);
	struct net_device *peer;
	int iflink;

	rcu_read_lock();
	peer = rcu_dereference(priv->peer);
	iflink = peer ? READ_ONCE(peer->ifindex) : 0;
	rcu_read_unlock();

	return iflink;
}

[yuwata]

@spikeh Thank you for taking a look.

What is the expectation of systemd-udevd? netdevsim has the same behaviour as veth:

static int veth_get_iflink(const struct net_device *dev)
{
	struct veth_priv *priv = netdev_priv(dev);
	struct net_device *peer;
	int iflink;

	rcu_read_lock();
	peer = rcu_dereference(priv->peer);
	iflink = peer ? READ_ONCE(peer->ifindex) : 0;
	rcu_read_unlock();

	return iflink;
}

Hm? Is it possible to configure veth without peer?? I guess returning 0 here is just for safety, and should never happen.

systemd-udevd tries to generate predictable names for 'non-stacked' network interfaces:

systemd/src/udev/udev-builtin-net_id.c

Lines 1288 to 1314 in 45af01d

	static int device_is_stacked(sd_device *dev) {
	int ifindex, iflink, r;
	assert(dev);
	r = sd_device_get_ifindex(dev, &ifindex);
	if (r < 0)
	return r;
	r = device_get_sysattr_int_filtered(dev, "iflink", &iflink);
	if (r < 0)
	return r;
	return ifindex != iflink;
	}
	static int builtin_net_id(UdevEvent *event, int argc, char *argv[]) {
	sd_device *dev = ASSERT_PTR(ASSERT_PTR(event)->dev);
	const char *prefix;
	int r;
	/* skip stacked devices, like VLANs, ... */
	r = device_is_stacked(dev);
	if (r < 0)
	return log_device_debug_errno(dev, r, "Failed to check if the device is stacked: %m");
	if (r > 0)
	return 0;

Of course, veth is not a stacked netdev, but anyway we do not generate names for veth.

Previously (kernel 6.8 or older), netdevsim provides iflink == ifindex, and we generate a name for netdevsim interfaces, based on its ID and port number. That way, we can easily distinguish multiple netdevsim interfaces.
With 6.9 or newer, now iflink != ifindex, so udevd does not generate the name, and causes a regression.

Background:
One reason we generate a predictable name for netdevsim is that we cannot specify interface names and list of MAC addresses of interfaces on create, as it is created by hitting new_device attribute, unlike other netdev like veth, tunnel, vlan, and so on. So, there is no way to distinguish interfaces, especially when we create multiple netdevsim interfaces at once, e.g. echo 99 10 > new_device.

For veth, on creation, we can specify interface name and MAC address, so we can distinguish them without any automatically generated predictable names.

[yuwata]

BTW, even when a netdevsim interface has a peer, should the peer be exposed through iflink?? Not a strong opinion, and not familar, but IIUC netdevsim is a kind of emulator of 'real' ethernet devices, and they typically provides iflink == ifindex, no?

[yuwata]

But, at the same time, our detection logic may not be good, as if the peer (or underlying device for e.g. tunnel or vlan) is in another network namespace, then iflink can equal to ifindex.

[spikeh]

Thanks for explaining. Yes, netdevsim is definitely doing the wrong thing here. Tbh I copied from veth without thinking about it too much, so apologies for causing breakages.

Not many real devices have ndo_get_iflink() defined. From userspace would you ever need it to return the linked peer like veth? Depending on your answer, I can either remove the ndo or make it return dev->ifindex if there is no peer like you suggested.

[yuwata]

@spikeh I think we should not mix backward compatibility in the kernel API and improving (or fixing) logic of virtual netdev detection in udevd. And I think we should do both below.

• For kernel side, Please make iflink attribute of netdevsim interfaces return ifindex when no peer is set, to keep backward compatibility. This is important when an old udevd is running on newer kernel.
• For udev side, we should not use iflink attribute to judge if udevd should generate persistent network interface names. I guess, refusing to generate names when a device syspath starts with /sys/devices/virtual/net/ should work fine. And with this way, not only we can support netdevsim with v6.9 (without backporting future fix I request), but also may extend support other rare cases (e.g. ipoib and virtual wlan device also have their own ndo_get_iflink, though I have not checked their implementation in detail). But that should be done in a future release.

So, in short, please please apply the change something like that I proposed in #32910 (comment).