usbnet: modern method to get random MAC

[PATCHv2 net] usbnet: modern method to get random MAC

Posted by Oliver Neukum 1 year, 5 months ago

The driver generates a random MAC once on load
and uses it over and over, including on two devices
needing a random MAC at the same time.

Jakub suggested revamping the driver to the modern
API for setting a random MAC rather than fixing
the old stuff.

The bug is as old as the driver.

Signed-off-by: Oliver Neukum <oneukum@suse.com>

---

v2: Correct commentary style

 drivers/net/usb/usbnet.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index dfc37016690b..40536e1cb4df 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);
@@ -1743,7 +1740,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.
@@ -1819,9 +1815,9 @@ 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;
+	/* this flags the device for user space */
+	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);
@@ -2229,7 +2225,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.45.2

Re: [PATCHv2 net] usbnet: modern method to get random MAC

Posted by Eric Dumazet 1 year, 3 months ago

On Thu, Aug 29, 2024 at 7:52 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> The driver generates a random MAC once on load
> and uses it over and over, including on two devices
> needing a random MAC at the same time.
>
> Jakub suggested revamping the driver to the modern
> API for setting a random MAC rather than fixing
> the old stuff.
>
> The bug is as old as the driver.
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
>
> ---
>
> v2: Correct commentary style
>
>  drivers/net/usb/usbnet.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index dfc37016690b..40536e1cb4df 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);
> @@ -1743,7 +1740,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.
> @@ -1819,9 +1815,9 @@ 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;
> +       /* this flags the device for user space */
> +       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);
> @@ -2229,7 +2225,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.45.2
>

As diagnosed by John Sperbeck :

This patch implies all ->bind() method took care of populating net->dev_addr ?

Otherwise the following existing heuristic is no longer working

// heuristic:  "usb%d" for links we know are two-host,
// else "eth%d" when there's reasonable doubt.  userspace
// can rename the link if it knows better.
if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
    ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
     (net->dev_addr [0] & 0x02) == 0))
strscpy(net->name, "eth%d", sizeof(net->name));

Re: [PATCHv2 net] usbnet: modern method to get random MAC

Posted by Oliver Neukum 1 year, 3 months ago

On 14.10.24 21:59, Eric Dumazet wrote:

> As diagnosed by John Sperbeck :
> 
> This patch implies all ->bind() method took care of populating net->dev_addr ?
> 
> Otherwise the following existing heuristic is no longer working
> 
> // heuristic:  "usb%d" for links we know are two-host,
> // else "eth%d" when there's reasonable doubt.  userspace
> // can rename the link if it knows better.
> if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
>      ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
>       (net->dev_addr [0] & 0x02) == 0))
> strscpy(net->name, "eth%d", sizeof(net->name));
> 

Hi,

you need to have a MAC to be an ethernet device, don't you?

	Regards
		Oliver

Re: [PATCHv2 net] usbnet: modern method to get random MAC

Posted by Eric Dumazet 1 year, 3 months ago

On Tue, Oct 15, 2024 at 12:24 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> On 14.10.24 21:59, Eric Dumazet wrote:
>
> > As diagnosed by John Sperbeck :
> >
> > This patch implies all ->bind() method took care of populating net->dev_addr ?
> >
> > Otherwise the following existing heuristic is no longer working
> >
> > // heuristic:  "usb%d" for links we know are two-host,
> > // else "eth%d" when there's reasonable doubt.  userspace
> > // can rename the link if it knows better.
> > if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
> >      ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
> >       (net->dev_addr [0] & 0x02) == 0))
> > strscpy(net->name, "eth%d", sizeof(net->name));
> >
>
> Hi,
>
> you need to have a MAC to be an ethernet device, don't you?

Before or after your patch, there was/is a MAC address, eventually random.

The problem is about the test, which is now done while dev->dev_addr
is full of zeroes, which is not a valid address,
as shown by :

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e4775fb5a2f6..1a316773319f 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1750,7 +1750,8 @@ usbnet_probe (struct usb_interface *udev, const
struct usb_device_id *prod)
                // can rename the link if it knows better.
                if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
                    ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
-                    (net->dev_addr [0] & 0x02) == 0))
+                    (is_valid_ether_addr(net->dev_addr) &&
+                     (net->dev_addr [0] & 0x02) == 0)))
                        strscpy(net->name, "eth%d", sizeof(net->name));
                /* WLAN devices should always be named "wlan%d" */
                if ((dev->driver_info->flags & FLAG_WLAN) != 0)

To be clear : We are hitting a regression after your patch was
backported to stable versions.

Re: [PATCHv2 net] usbnet: modern method to get random MAC

Posted by Oliver Neukum 1 year, 3 months ago


On 15.10.24 01:00, Eric Dumazet wrote:
> On Tue, Oct 15, 2024 at 12:24 AM Oliver Neukum <oneukum@suse.com> wrote:
>>
>> On 14.10.24 21:59, Eric Dumazet wrote:
>>
>>> As diagnosed by John Sperbeck :
>>>
>>> This patch implies all ->bind() method took care of populating net->dev_addr ?
>>>
>>> Otherwise the following existing heuristic is no longer working
>>>
>>> // heuristic:  "usb%d" for links we know are two-host,
>>> // else "eth%d" when there's reasonable doubt.  userspace
>>> // can rename the link if it knows better.
>>> if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
>>>       ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
>>>        (net->dev_addr [0] & 0x02) == 0))
>>> strscpy(net->name, "eth%d", sizeof(net->name));
>>>
>>
>> Hi,
>>
>> you need to have a MAC to be an ethernet device, don't you?
> 
> Before or after your patch, there was/is a MAC address, eventually random.
> 
> The problem is about the test, which is now done while dev->dev_addr
> is full of zeroes, which is not a valid address,
> as shown by :

Hi,

I am sorry I misunderstood you. Yes, I overlooked the test for whether
the MAC has been altered. I am preparing a patch. Could you give me
John Perbeck's address, so I can include him in "reported-by"?

	Sorry
		Oliver

Re: [PATCHv2 net] usbnet: modern method to get random MAC

Posted by Eric Dumazet 1 year, 3 months ago

On Tue, Oct 15, 2024 at 9:49 AM Oliver Neukum <oneukum@suse.com> wrote:
>
>
>
> On 15.10.24 01:00, Eric Dumazet wrote:
> > On Tue, Oct 15, 2024 at 12:24 AM Oliver Neukum <oneukum@suse.com> wrote:
> >>
> >> On 14.10.24 21:59, Eric Dumazet wrote:
> >>
> >>> As diagnosed by John Sperbeck :
> >>>
> >>> This patch implies all ->bind() method took care of populating net->dev_addr ?
> >>>
> >>> Otherwise the following existing heuristic is no longer working
> >>>
> >>> // heuristic:  "usb%d" for links we know are two-host,
> >>> // else "eth%d" when there's reasonable doubt.  userspace
> >>> // can rename the link if it knows better.
> >>> if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
> >>>       ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
> >>>        (net->dev_addr [0] & 0x02) == 0))
> >>> strscpy(net->name, "eth%d", sizeof(net->name));
> >>>
> >>
> >> Hi,
> >>
> >> you need to have a MAC to be an ethernet device, don't you?
> >
> > Before or after your patch, there was/is a MAC address, eventually random.
> >
> > The problem is about the test, which is now done while dev->dev_addr
> > is full of zeroes, which is not a valid address,
> > as shown by :
>
> Hi,
>
> I am sorry I misunderstood you. Yes, I overlooked the test for whether
> the MAC has been altered. I am preparing a patch. Could you give me
> John Perbeck's address, so I can include him in "reported-by"?
>

Reported-by: Greg Thelen <gthelen@google.com>
Diagnosed-by: John Sperbeck <jsperbeck@google.com>

Thank you.

Re: [PATCHv2 net] usbnet: modern method to get random MAC

Posted by Simon Horman 1 year, 5 months ago

On Thu, Aug 29, 2024 at 07:50:55PM +0200, Oliver Neukum wrote:
> The driver generates a random MAC once on load
> and uses it over and over, including on two devices
> needing a random MAC at the same time.
> 
> Jakub suggested revamping the driver to the modern
> API for setting a random MAC rather than fixing
> the old stuff.
> 
> The bug is as old as the driver.

I think this is appropriate:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> Signed-off-by: Oliver Neukum <oneukum@suse.com>

Reviewed-by: Simon Horman <horms@kernel.org>