[PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Toke Høiland-Jørgensen]

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(+)

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",
+		     &eth_addr[0], &eth_addr[1], &eth_addr[2],
+		     &eth_addr[3], &eth_addr[4], &eth_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;

---
base-commit: 0ad9617c78acbc71373fb341a6f75d4012b01d69
change-id: 20250128-netdevsim-perm_addr-5fca47a08157

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Jakub Kicinski]

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.
-- 
pw-bot: cr

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Andrew Lunn]

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

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Toke Høiland-Jørgensen]

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

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Toke Høiland-Jørgensen]

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

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Jakub Kicinski]

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.

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Toke Høiland-Jørgensen]

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

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Jakub Kicinski]

On Wed, 05 Feb 2025 10:05:17 +0100 Toke Høiland-Jørgensen wrote:
> >> 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?

Depends on your definition of "feature works". Going thru all the
address types and how they behave would be a reasonable test I think.
Checking that an address from debugfs makes it to netlink would not.

> > 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?

I don't see how that'd be sufficient for a meaningful test.

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Eric Dumazet]

On Wed, Feb 5, 2025 at 6:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 05 Feb 2025 10:05:17 +0100 Toke Høiland-Jørgensen wrote:
> > >> 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?
>
> Depends on your definition of "feature works". Going thru all the
> address types and how they behave would be a reasonable test I think.
> Checking that an address from debugfs makes it to netlink would not.
>
> > > 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?
>
> I don't see how that'd be sufficient for a meaningful test.

Perhaps allow IFLA_PERM_ADDRESS to be set at veth creation time,
or other virtual devices...

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Jakub Kicinski]

On Wed, 5 Feb 2025 18:08:00 +0100 Eric Dumazet wrote:
> > > Right. How do you feel about Andrew's suggestion of just setting a
> > > static perm_addr for netdevsim devices?  
> >
> > I don't see how that'd be sufficient for a meaningful test.  
> 
> Perhaps allow IFLA_PERM_ADDRESS to be set at veth creation time,
> or other virtual devices...

I'd rather we didn't sprinkle testing code into real drivers, FWIW.

Re: [PATCH net-next] net: netdevsim: Support setting dev->perm_addr

[Jiri Pirko]

Mon, Feb 03, 2025 at 06:21:24PM +0100, toke@redhat.com 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.
>
>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(+)
>
>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",
>+		     &eth_addr[0], &eth_addr[1], &eth_addr[2],
>+		     &eth_addr[3], &eth_addr[4], &eth_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,
>+};

Well, hwaddr is not a runtime config, it's rather provisioning time
config. I believe the best fit would be to put it to new_device_store()
or new_port_store().

We already implement couple of things there in new_device_store(), like
port count and number of queues.


>+
> 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;
>
>---
>base-commit: 0ad9617c78acbc71373fb341a6f75d4012b01d69
>change-id: 20250128-netdevsim-perm_addr-5fca47a08157
>
>