[RFC] usbnet: assign unique random MAC

[RFC] usbnet: assign unique random MAC

[Oliver Neukum]

The old method had the bug of issuing the same
random MAC over and over even to every device.
This bug is as old as the driver.

This new method generates each device whose minidriver
does not provide its own MAC its own unique random
MAC.

A module parameter to go back to the old behavior
is included.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/usbnet.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 2d14b0d78541..53cb3a8d48c3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -61,8 +61,10 @@
 
 /*-------------------------------------------------------------------------*/
 
-// randomly generated ethernet address
-static u8	node_id [ETH_ALEN];
+/* for the legacy behavior */
+
+u8 legacyrandomid[ETH_ALEN];
+static bool legacymac = false;
 
 /* use ethtool to change the level for any given device */
 static int msg_level = -1;
@@ -1731,7 +1733,6 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	dev->net = net;
 	strscpy(net->name, "usb%d", sizeof(net->name));
-	eth_hw_addr_set(net, node_id);
 
 	/* rx and tx sides can use different message sizes;
 	 * bind() should set rx_urb_size in that case.
@@ -1805,9 +1806,19 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 		goto out4;
 	}
 
-	/* let userspace know we have a random address */
-	if (ether_addr_equal(net->dev_addr, node_id))
-		net->addr_assign_type = NET_ADDR_RANDOM;
+	/*
+	 * if the device does not come with a MAC
+	 * we ask the network core to generate us one
+	 * and flag the device accordingly
+	 */
+	if (!is_valid_ether_addr(net->dev_addr)) {
+		if (legacymac) {
+			eth_hw_addr_set(net, legacyrandomid);
+			net->addr_assign_type = NET_ADDR_RANDOM;
+		} else {
+			eth_hw_addr_random(net);
+		}
+	}
 
 	if ((dev->driver_info->flags & FLAG_WLAN) != 0)
 		SET_NETDEV_DEVTYPE(net, &wlan_type);
@@ -2217,7 +2228,7 @@ static int __init usbnet_init(void)
 	BUILD_BUG_ON(
 		sizeof_field(struct sk_buff, cb) < sizeof(struct skb_data));
 
-	eth_random_addr(node_id);
+	eth_random_addr(legacyrandomid);
 	return 0;
 }
 module_init(usbnet_init);
@@ -2227,6 +2238,8 @@ static void __exit usbnet_exit(void)
 }
 module_exit(usbnet_exit);
 
+module_param(legacymac, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(legacymac, "Use a legacy style common MAC if device need a random MAC");
 MODULE_AUTHOR("David Brownell");
 MODULE_DESCRIPTION("USB network driver framework");
 MODULE_LICENSE("GPL");
-- 
2.42.1

Re: [RFC] usbnet: assign unique random MAC

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> A module parameter to go back to the old behavior
> is included.

Is this really required?  If we add it now then we can never get rid of
it.  Why not try without, and add this back if/when somebody complains
about the new behaviour?

I believe there's a fair chance no one will notice or complain.  And we
have much cleaner code and one module param less.


Bjørn

Re: [RFC] usbnet: assign unique random MAC

[Oliver Neukum]

On 16.11.23 13:39, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> 
>> A module parameter to go back to the old behavior
>> is included.
> 
> Is this really required?  If we add it now then we can never get rid of
> it.  Why not try without, and add this back if/when somebody complains
> about the new behaviour?
> 
> I believe there's a fair chance no one will notice or complain.  And we
> have much cleaner code and one module param less.

Isn't it a bit evil to change behavior?
Do you think I should make a different version for stable
with the logic for retaining the old behavior inverted?

	Regards
		Oliver

Re: [RFC] usbnet: assign unique random MAC

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> On 16.11.23 13:39, Bjørn Mork wrote:
>> Oliver Neukum <oneukum@suse.com> writes:
>> 
>>> A module parameter to go back to the old behavior
>>> is included.
>> Is this really required?  If we add it now then we can never get rid
>> of
>> it.  Why not try without, and add this back if/when somebody complains
>> about the new behaviour?
>> I believe there's a fair chance no one will notice or complain.  And
>> we
>> have much cleaner code and one module param less.
>
> Isn't it a bit evil to change behavior?

Only if someone actually depend on the old behaviour.  And I think
there's a fair chance no one does.

I don't propose forcing this change on anyone.  Only to try and see if
we can apply if without any force involved.

Note that the module parameter solution also will be a breaking change
for anyone depending on the current behaviour.  If you want to avoid
that, then you need to invert the logic. And then you might as well drop
the whole change.

> Do you think I should make a different version for stable
> with the logic for retaining the old behavior inverted?

I assumed this was unsuitable for stable backports.  Is there any reason
to backport it?


Bjørn

Re: [RFC] usbnet: assign unique random MAC

[Oliver Neukum]

On 16.11.23 14:21, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> 
>> On 16.11.23 13:39, Bjørn Mork wrote:
>>> Oliver Neukum <oneukum@suse.com> writes:

>> Isn't it a bit evil to change behavior?
> 
> Only if someone actually depend on the old behaviour.  And I think
> there's a fair chance no one does.

Very well. I'll take it out.

>> Do you think I should make a different version for stable
>> with the logic for retaining the old behavior inverted?
> 
> I assumed this was unsuitable for stable backports.  Is there any reason
> to backport it?

You could argue that handing out the same MAC twice
violates standards.

	Regards
		Oliver

[RFC] usbnet: assign unique random MAC

[Oliver Neukum]

The old method had the bug of issuing the same
random MAC over and over even to every device.
This bug is as old as the driver.

This new method generates each device whose minidriver
does not provide its own MAC its own unique random
MAC.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/usbnet.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 2d14b0d78541..37e3bb2170bc 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -61,9 +61,6 @@
 
 /*-------------------------------------------------------------------------*/
 
-// randomly generated ethernet address
-static u8	node_id [ETH_ALEN];
-
 /* use ethtool to change the level for any given device */
 static int msg_level = -1;
 module_param (msg_level, int, 0);
@@ -1731,7 +1728,6 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	dev->net = net;
 	strscpy(net->name, "usb%d", sizeof(net->name));
-	eth_hw_addr_set(net, node_id);
 
 	/* rx and tx sides can use different message sizes;
 	 * bind() should set rx_urb_size in that case.
@@ -1805,9 +1801,13 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 		goto out4;
 	}
 
-	/* let userspace know we have a random address */
-	if (ether_addr_equal(net->dev_addr, node_id))
-		net->addr_assign_type = NET_ADDR_RANDOM;
+	/*
+	 * if the device does not come with a MAC
+	 * we ask the network core to generate us one
+	 * and flag the device accordingly
+	 */
+	if (!is_valid_ether_addr(net->dev_addr))
+			eth_hw_addr_random(net);
 
 	if ((dev->driver_info->flags & FLAG_WLAN) != 0)
 		SET_NETDEV_DEVTYPE(net, &wlan_type);
@@ -2217,7 +2217,6 @@ static int __init usbnet_init(void)
 	BUILD_BUG_ON(
 		sizeof_field(struct sk_buff, cb) < sizeof(struct skb_data));
 
-	eth_random_addr(node_id);
 	return 0;
 }
 module_init(usbnet_init);
-- 
2.42.1

Re: [RFC] usbnet: assign unique random MAC

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> You could argue that handing out the same MAC twice
> violates standards.

You can't argue that to the Sparc crowd.  Which has to be considered
when sending stuff to netdev :-)


Bjørn

Re: [RFC] usbnet: assign unique random MAC

[Oliver Neukum]

On 16.11.23 15:49, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> 
>> You could argue that handing out the same MAC twice
>> violates standards.
> 
> You can't argue that to the Sparc crowd.  Which has to be considered
> when sending stuff to netdev :-)

Should I reword the commit message?

	Regards
		Oliver

Re: [RFC] usbnet: assign unique random MAC

[Stephen Hemminger]

On Thu, 16 Nov 2023 13:30:24 +0100
Oliver Neukum <oneukum@suse.com> wrote:

>  module_exit(usbnet_exit);
>  
> +module_param(legacymac, bool, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(legacymac, "Use a legacy style common MAC if device need a random MAC");
>  MODULE_AUTHOR("David Brownell");

Module params are bad idea, just do the right thing.

Re: [RFC] usbnet: assign unique random MAC

[Stephen Hemminger]

On Thu, 16 Nov 2023 15:05:52 +0100
Oliver Neukum <oneukum@suse.com> wrote:

> The old method had the bug of issuing the same
> random MAC over and over even to every device.
> This bug is as old as the driver.
> 
> This new method generates each device whose minidriver
> does not provide its own MAC its own unique random
> MAC.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/net/usb/usbnet.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 2d14b0d78541..37e3bb2170bc 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -61,9 +61,6 @@
>  
>  /*-------------------------------------------------------------------------*/
>  
> -// randomly generated ethernet address
> -static u8	node_id [ETH_ALEN];
> -
>  /* use ethtool to change the level for any given device */
>  static int msg_level = -1;
>  module_param (msg_level, int, 0);
> @@ -1731,7 +1728,6 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>  
>  	dev->net = net;
>  	strscpy(net->name, "usb%d", sizeof(net->name));
> -	eth_hw_addr_set(net, node_id);
>  
>  	/* rx and tx sides can use different message sizes;
>  	 * bind() should set rx_urb_size in that case.
> @@ -1805,9 +1801,13 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>  		goto out4;
>  	}
>  
> -	/* let userspace know we have a random address */
> -	if (ether_addr_equal(net->dev_addr, node_id))
> -		net->addr_assign_type = NET_ADDR_RANDOM;
> +	/*
> +	 * if the device does not come with a MAC
> +	 * we ask the network core to generate us one
> +	 * and flag the device accordingly
> +	 */
> +	if (!is_valid_ether_addr(net->dev_addr))
> +			eth_hw_addr_random(net);
>  
>  	if ((dev->driver_info->flags & FLAG_WLAN) != 0)
>  		SET_NETDEV_DEVTYPE(net, &wlan_type);
> @@ -2217,7 +2217,6 @@ static int __init usbnet_init(void)
>  	BUILD_BUG_ON(
>  		sizeof_field(struct sk_buff, cb) < sizeof(struct skb_data));
>  
> -	eth_random_addr(node_id);
>  	return 0;
>  }
>  module_init(usbnet_init);

The code part looks fine. Fix the style issues though.

 $ ./scripts/checkpatch.pl /tmp/mac.mbox 
WARNING: networking block comments don't use an empty /* line, use /* Comment...
#163: FILE: drivers/net/usb/usbnet.c:1805:
+	/*
+	 * if the device does not come with a MAC

WARNING: suspect code indent for conditional statements (8, 24)
#167: FILE: drivers/net/usb/usbnet.c:1809:
+	if (!is_valid_ether_addr(net->dev_addr))
+			eth_hw_addr_random(net);

total: 0 errors, 2 warnings, 0 checks, 39 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/mac.mbox has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Re: [RFC] usbnet: assign unique random MAC

[Benjamin Poirier]

On 2023-11-16 15:05 +0100, Oliver Neukum wrote:
> The old method had the bug of issuing the same
> random MAC over and over even to every device.
> This bug is as old as the driver.
> 
> This new method generates each device whose minidriver
> does not provide its own MAC its own unique random
> MAC.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/net/usb/usbnet.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 2d14b0d78541..37e3bb2170bc 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -61,9 +61,6 @@
>  
>  /*-------------------------------------------------------------------------*/
>  
> -// randomly generated ethernet address
> -static u8	node_id [ETH_ALEN];
> -
>  /* use ethtool to change the level for any given device */
>  static int msg_level = -1;
>  module_param (msg_level, int, 0);
> @@ -1731,7 +1728,6 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>  
>  	dev->net = net;
>  	strscpy(net->name, "usb%d", sizeof(net->name));
> -	eth_hw_addr_set(net, node_id);
>  
>  	/* rx and tx sides can use different message sizes;
>  	 * bind() should set rx_urb_size in that case.
> @@ -1805,9 +1801,13 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>  		goto out4;
>  	}
>  
> -	/* let userspace know we have a random address */
> -	if (ether_addr_equal(net->dev_addr, node_id))
> -		net->addr_assign_type = NET_ADDR_RANDOM;
> +	/*
> +	 * if the device does not come with a MAC

The patch's formatting has some problems. Please run checkpatch before
resubmitting.

> +	 * we ask the network core to generate us one
> +	 * and flag the device accordingly
> +	 */
> +	if (!is_valid_ether_addr(net->dev_addr))
> +			eth_hw_addr_random(net);

Before initialization, dev_addr is null (00:00:00:00:00:00). Since this
patch moves the fallback address initialization after the 
	if (info->bind) {
block, if the bind() did not initialize the address, this patch changes
the result of the
		     (net->dev_addr [0] & 0x02) == 0))
test within the block, no? The test now takes place on an uninitialized
address and the result goes from false to true.

Re: [RFC] usbnet: assign unique random MAC

[Andrew Lunn]

On Thu, Nov 16, 2023 at 01:39:59PM +0100, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> 
> > A module parameter to go back to the old behavior
> > is included.
> 
> Is this really required?  If we add it now then we can never get rid of
> it.  Why not try without, and add this back if/when somebody complains
> about the new behaviour?
> 
> I believe there's a fair chance no one will notice or complain.  And we
> have much cleaner code and one module param less.

As Stephen pointed out, module parameters are not really liked in
netdev. I suggest not having it. Post this patch for net-next, and
don't make the commit message sound like it is a fix, otherwise it
might get back ported by the Machine Learning fix spotting bot, which
we probably don't want.

    Andrew

---
pw-bot: cr

Re: [RFC] usbnet: assign unique random MAC

[Oliver Neukum]

On 16.11.23 21:48, Benjamin Poirier wrote:

> Before initialization, dev_addr is null (00:00:00:00:00:00). Since this
> patch moves the fallback address initialization after the
> 	if (info->bind) {
> block, if the bind() did not initialize the address, this patch changes
> the result of the
> 		     (net->dev_addr [0] & 0x02) == 0))
> test within the block, no? The test now takes place on an uninitialized
> address and the result goes from false to true.

Hi,

right this issue exists. V2 will come up.

	Regards
		Oliver