| Message ID | 20240829175201.670718-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bab8eb0dd4cb995caa4a0529d5655531c2ec5e8e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv2,net] usbnet: modern method to get random MAC | expand |
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>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 29 Aug 2024 19:50:55 +0200 you 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. > > [...] Here is the summary with links: - [PATCHv2,net] usbnet: modern method to get random MAC https://git.kernel.org/netdev/net/c/bab8eb0dd4cb You are awesome, thank you!
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));
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
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.
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
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.
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);
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(-)