| Message ID | 20250203-netdevsim-perm_addr-v1-1-10084bc93044@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: netdevsim: Support setting dev->perm_addr | expand |
On Mon, 03 Feb 2025 18:21:24 +0100 Toke Høiland-Jørgensen wrote: > Network management daemons that match on the device permanent address > currently have no virtual interface types to test against. > NetworkManager, in particular, has carried an out of tree patch to set > the permanent address on netdevsim devices to use in its CI for this > purpose. > > To support this use case, add a debugfs file for netdevsim to set the > permanent address to an arbitrary value. netdevsim is not for user space testing. We have gone down the path of supporting random features in it already, and then wasted time trying to maintain them thru various devlink related perturbations, just to find out that the features weren't actually used any more. NetworkManager can do the HW testing using virtme-ng. If you want to go down the netdevsim path you must provide a meaningful in-tree test, but let's be clear that we will 100% delete both the test and the netdevsim functionality if it causes any issues.
On Mon, Feb 03, 2025 at 02:39:58PM -0800, Jakub Kicinski wrote: > On Mon, 03 Feb 2025 18:21:24 +0100 Toke Høiland-Jørgensen wrote: > > Network management daemons that match on the device permanent address > > currently have no virtual interface types to test against. > > NetworkManager, in particular, has carried an out of tree patch to set > > the permanent address on netdevsim devices to use in its CI for this > > purpose. > > > > To support this use case, add a debugfs file for netdevsim to set the > > permanent address to an arbitrary value. > > netdevsim is not for user space testing. We have gone down the path > of supporting random features in it already, and then wasted time trying > to maintain them thru various devlink related perturbations, just to > find out that the features weren't actually used any more. > > NetworkManager can do the HW testing using virtme-ng. > > If you want to go down the netdevsim path you must provide a meaningful > in-tree test, but let's be clear that we will 100% delete both the test > and the netdevsim functionality if it causes any issues. Hi Toke What are your actual requirements? A permanent address is not expected to change, it is by definition, permanent. Could it be hard coded in netdevsim that the first instance created gets the MAC address 24:42:42:42:42:42? And maybe to make testing a bit more evil, keep the current behaviour that the actually used MAC is random, since that MAC address is not permanent. Andrew
Andrew Lunn <andrew@lunn.ch> writes: > On Mon, Feb 03, 2025 at 02:39:58PM -0800, Jakub Kicinski wrote: >> On Mon, 03 Feb 2025 18:21:24 +0100 Toke Høiland-Jørgensen wrote: >> > Network management daemons that match on the device permanent address >> > currently have no virtual interface types to test against. >> > NetworkManager, in particular, has carried an out of tree patch to set >> > the permanent address on netdevsim devices to use in its CI for this >> > purpose. >> > >> > To support this use case, add a debugfs file for netdevsim to set the >> > permanent address to an arbitrary value. >> >> netdevsim is not for user space testing. We have gone down the path >> of supporting random features in it already, and then wasted time trying >> to maintain them thru various devlink related perturbations, just to >> find out that the features weren't actually used any more. >> >> NetworkManager can do the HW testing using virtme-ng. >> >> If you want to go down the netdevsim path you must provide a meaningful >> in-tree test, but let's be clear that we will 100% delete both the test >> and the netdevsim functionality if it causes any issues. > > Hi Toke > > What are your actual requirements? A permanent address is not expected > to change, it is by definition, permanent. Could it be hard coded in > netdevsim that the first instance created gets the MAC address > 24:42:42:42:42:42? And maybe to make testing a bit more evil, keep the > current behaviour that the actually used MAC is random, since that MAC > address is not permanent. I believe that would work, yeah. AFAIU, we just need some virtual device that has the permanent address field set at all. I'll check with the NM folks and respin with a static value, assuming this works for them. Thanks for the suggestion! -Toke
Jakub Kicinski <kuba@kernel.org> writes: > On Mon, 03 Feb 2025 18:21:24 +0100 Toke Høiland-Jørgensen wrote: >> Network management daemons that match on the device permanent address >> currently have no virtual interface types to test against. >> NetworkManager, in particular, has carried an out of tree patch to set >> the permanent address on netdevsim devices to use in its CI for this >> purpose. >> >> To support this use case, add a debugfs file for netdevsim to set the >> permanent address to an arbitrary value. > > netdevsim is not for user space testing. We have gone down the path > of supporting random features in it already, and then wasted time trying > to maintain them thru various devlink related perturbations, just to > find out that the features weren't actually used any more. > > NetworkManager can do the HW testing using virtme-ng. Sorry if I'm being dense, but how would that work? What device type would one create inside a virtme-ng environment that would have a perm_addr set? > If you want to go down the netdevsim path you must provide a meaningful > in-tree test, but let's be clear that we will 100% delete both the test > and the netdevsim functionality if it causes any issues. Can certainly add a test case, sure! Any preference for where to put it? Somewhere in selftests/net, I guess, but where? rtnetlink.sh and bpf_offload.py seem to be the only files currently doing anything with netdevsim. I could add a case to the former? -Toke
On Tue, 04 Feb 2025 12:20:56 +0100 Toke Høiland-Jørgensen wrote: > > netdevsim is not for user space testing. We have gone down the path > > of supporting random features in it already, and then wasted time trying > > to maintain them thru various devlink related perturbations, just to > > find out that the features weren't actually used any more. > > > > NetworkManager can do the HW testing using virtme-ng. > > Sorry if I'm being dense, but how would that work? What device type > would one create inside a virtme-ng environment that would have a > perm_addr set? virtme-ng is just a qemu wrapper. Qemu supports a bunch of emulated HW. > > If you want to go down the netdevsim path you must provide a meaningful > > in-tree test, but let's be clear that we will 100% delete both the test > > and the netdevsim functionality if it causes any issues. > > Can certainly add a test case, sure! Any preference for where to put it? > Somewhere in selftests/net, I guess, but where? rtnetlink.sh and > bpf_offload.py seem to be the only files currently doing anything with > netdevsim. I could add a case to the former? No preference, just an emphasis on _meaningful_. Kernel supports loading OOT modules, too. I really don't want us to be in the business of carrying test harnesses for random pieces of user space code.
Jakub Kicinski <kuba@kernel.org> writes: > On Tue, 04 Feb 2025 12:20:56 +0100 Toke Høiland-Jørgensen wrote: >> > netdevsim is not for user space testing. We have gone down the path >> > of supporting random features in it already, and then wasted time trying >> > to maintain them thru various devlink related perturbations, just to >> > find out that the features weren't actually used any more. >> > >> > NetworkManager can do the HW testing using virtme-ng. >> >> Sorry if I'm being dense, but how would that work? What device type >> would one create inside a virtme-ng environment that would have a >> perm_addr set? > > virtme-ng is just a qemu wrapper. Qemu supports a bunch of emulated HW. Hmm, okay, I'll pass the suggestion along. >> > If you want to go down the netdevsim path you must provide a meaningful >> > in-tree test, but let's be clear that we will 100% delete both the test >> > and the netdevsim functionality if it causes any issues. >> >> Can certainly add a test case, sure! Any preference for where to put it? >> Somewhere in selftests/net, I guess, but where? rtnetlink.sh and >> bpf_offload.py seem to be the only files currently doing anything with >> netdevsim. I could add a case to the former? > > No preference, just an emphasis on _meaningful_. OK, so checking that the feature works is not enough, in other words? > Kernel supports loading OOT modules, too. I really don't want us > to be in the business of carrying test harnesses for random pieces > of user space code. Right. How do you feel about Andrew's suggestion of just setting a static perm_addr for netdevsim devices? -Toke
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c index 42f247cbdceecbadf27f7090c030aa5bd240c18a..3a7fcc32901c754eadf7d6ea43cd0ddc29586cf9 100644 --- a/drivers/net/netdevsim/netdev.c +++ b/drivers/net/netdevsim/netdev.c @@ -782,6 +782,47 @@ static const struct file_operations nsim_qreset_fops = { .owner = THIS_MODULE, }; +static ssize_t +nsim_permaddr_write(struct file *file, const char __user *data, + size_t count, loff_t *ppos) +{ + struct netdevsim *ns = file->private_data; + u8 eth_addr[ETH_ALEN]; + char buf[32]; + ssize_t ret; + + if (count >= sizeof(buf)) + return -EINVAL; + if (copy_from_user(buf, data, count)) + return -EFAULT; + buf[count] = '\0'; + + ret = sscanf(buf, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx", + ð_addr[0], ð_addr[1], ð_addr[2], + ð_addr[3], ð_addr[4], ð_addr[5]); + if (ret != 6) + return -EINVAL; + + rtnl_lock(); + if (netif_running(ns->netdev)) { + ret = -EBUSY; + goto exit_unlock; + } + + memcpy(ns->netdev->perm_addr, eth_addr, sizeof(eth_addr)); + + ret = count; +exit_unlock: + rtnl_unlock(); + return ret; +} + +static const struct file_operations nsim_permaddr_fops = { + .open = simple_open, + .write = nsim_permaddr_write, + .owner = THIS_MODULE, +}; + static ssize_t nsim_pp_hold_read(struct file *file, char __user *data, size_t count, loff_t *ppos) @@ -997,6 +1038,9 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port) ns->qr_dfs = debugfs_create_file("queue_reset", 0200, nsim_dev_port->ddir, ns, &nsim_qreset_fops); + ns->permaddr_dfs = debugfs_create_file("perm_addr", 0200, + nsim_dev_port->ddir, ns, + &nsim_permaddr_fops); return ns; diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h index dcf073bc4802e7f7f8c14a2b8d94d24cd31f1f7b..fffec5dbf80759240a323f7c3630c79c5c68faec 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -140,6 +140,7 @@ struct netdevsim { struct page *page; struct dentry *pp_dfs; struct dentry *qr_dfs; + struct dentry *permaddr_dfs; struct nsim_ethtool ethtool; struct netdevsim __rcu *peer;
Network management daemons that match on the device permanent address currently have no virtual interface types to test against. NetworkManager, in particular, has carried an out of tree patch to set the permanent address on netdevsim devices to use in its CI for this purpose. To support this use case, add a debugfs file for netdevsim to set the permanent address to an arbitrary value. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- drivers/net/netdevsim/netdev.c | 44 +++++++++++++++++++++++++++++++++++++++ drivers/net/netdevsim/netdevsim.h | 1 + 2 files changed, 45 insertions(+) --- base-commit: 0ad9617c78acbc71373fb341a6f75d4012b01d69 change-id: 20250128-netdevsim-perm_addr-5fca47a08157