| Message ID | 20231116140616.4848-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] usbnet: assign unique random MAC | expand |
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.
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.
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
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 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(-)