| Message ID | 20241203130457.904325-1-asmadeus@codewreck.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: usb: usbnet: restore usb%d name exception for local mac addresses | expand |
On Tue, Dec 03, 2024 at 10:04:55PM +0900, Dominique Martinet wrote: > From: Dominique Martinet <dominique.martinet@atmark-techno.com> > > The previous commit assumed that local addresses always came from the > kernel, but some devices hand out local mac addresses so we ended up > with point-to-point devices with a mac set by the driver, renaming to > eth%d when they used to be named usb%d. > > Userspace should not rely on device name, but for the sake of stability > restore the local mac address check portion of the naming exception: > point to point devices which either have no mac set by the driver or > have a local mac handed out by the driver will keep the usb%d name. > > Fixes: 8a7d12d674ac ("net: usb: usbnet: fix name regression") > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > drivers/net/usb/usbnet.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 44179f4e807f..d044dc7b7622 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -178,6 +178,17 @@ int usbnet_get_ethernet_addr(struct usbnet *dev, int iMACAddress) > } > EXPORT_SYMBOL_GPL(usbnet_get_ethernet_addr); > > +static bool usbnet_needs_usb_name_format(struct usbnet *dev, struct net_device *net) > +{ > + /* Point to point devices which don't have a real MAC address > + * (or report a fake local one) have historically used the usb%d > + * naming. Preserve this.. > + */ > + return (dev->driver_info->flags & FLAG_POINTTOPOINT) != 0 && > + (is_zero_ether_addr(net->dev_addr) || > + is_local_ether_addr(net->dev_addr)); > +} > + > static void intr_complete (struct urb *urb) > { > struct usbnet *dev = urb->context; > @@ -1762,13 +1773,10 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) > if (status < 0) > goto out1; > > - // 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. > + /* heuristic: rename to "eth%d" if we are not sure this link > + * is two-host (these links keep "usb%d") */ > if ((dev->driver_info->flags & FLAG_ETHER) != 0 && > - ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 || > - /* somebody touched it*/ > - !is_zero_ether_addr(net->dev_addr))) > + !usbnet_needs_usb_name_format(dev, net)) > strscpy(net->name, "eth%d", sizeof(net->name)); > /* WLAN devices should always be named "wlan%d" */ > if ((dev->driver_info->flags & FLAG_WLAN) != 0) > -- > 2.47.0 > > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
On Tue, Dec 03, 2024 at 10:04:55PM +0900, Dominique Martinet wrote: > From: Dominique Martinet <dominique.martinet@atmark-techno.com> > > The previous commit assumed that local addresses always came from the > kernel, but some devices hand out local mac addresses so we ended up > with point-to-point devices with a mac set by the driver, renaming to > eth%d when they used to be named usb%d. > > Userspace should not rely on device name, but for the sake of stability > restore the local mac address check portion of the naming exception: > point to point devices which either have no mac set by the driver or > have a local mac handed out by the driver will keep the usb%d name. Are you saying the OTP or NVMEM has a locally administered MAC address stored in it? Is there a mechanism to change it? The point about locally administered MAC addresses is that they are locally administered. Andrew
On Tue, 3 Dec 2024 21:47:57 +0100 Andrew Lunn wrote: > Are you saying the OTP or NVMEM has a locally administered MAC address > stored in it? Is there a mechanism to change it? The point about > locally administered MAC addresses is that they are locally > administered. You may also enjoy reading this :) commit bfe9b9d2df669a57a95d641ed46eb018e204c6ce Author: Kristian Evensen <kristian.evensen@gmail.com> Date: Thu Jul 21 11:10:06 2016 +0200 cdc_ether: Improve ZTE MF823/831/910 handling The firmware in several ZTE devices (at least the MF823/831/910 modems/mifis) use OS fingerprinting to determine which type of device to export. In addition, these devices export a REST API which can be used to control the type of device. So far, on Linux, the devices have been seen as RNDIS or CDC Ether. When CDC Ether is used, devices of the same type are, as with RNDIS, exported with the same, bogus random MAC address. In addition, the devices (at least on all firmware revisions I have found) use the bogus MAC when sending traffic routed from external networks. And as a final feature, the devices sometimes export the link state incorrectly. There are also references online to several other ZTE devices displaying this behavior, with several different PIDs and MAC addresses.
Andrew Lunn wrote on Tue, Dec 03, 2024 at 09:47:57PM +0100: > On Tue, Dec 03, 2024 at 10:04:55PM +0900, Dominique Martinet wrote: > > From: Dominique Martinet <dominique.martinet@atmark-techno.com> > > > > The previous commit assumed that local addresses always came from the > > kernel, but some devices hand out local mac addresses so we ended up > > with point-to-point devices with a mac set by the driver, renaming to > > eth%d when they used to be named usb%d. > > > > Userspace should not rely on device name, but for the sake of stability > > restore the local mac address check portion of the naming exception: > > point to point devices which either have no mac set by the driver or > > have a local mac handed out by the driver will keep the usb%d name. > > Are you saying the OTP or NVMEM has a locally administered MAC address > stored in it? I'm afraid so... (At least the Gemalto^W Cinterion^W Thales^W Telit ELS31-J we use on a couple of boards seem to do that, it's soldered on our boards so I can't swap it out easily to confirm but the mac address is stable accross reboots) The good news is that after having been sold at least 4 times it's been made EOL now, so in another 12-ish years I'll probably be able to ignore this particular problem :) > Is there a mechanism to change it? Looking at some confidential documentation I found on our file server there seems to be an usb function that contains the mac address and various ethernet statistics, but it's not clear to me if it's actually writable or even how to actually use it in practice and it was certainly not designed with being modified in mind. (I suspect there should be some vendor AT command that would allow overriding the setting somewhere but I can't find that either) OTOH, just changing the mac locally (ip link set usb0 addr 02:12:34:56:78:90) works and dhcp gets me a new IP, so it's not like overriding it is a problem either. (interestingly putting the old mac back gets me the old IP back, so there's a real dhcp server with leases behind this and I suspect I could just bridge this out and it'd work as expected...) > The point about locally administered MAC addresses is that they are > locally administered. Honest question here our of curiosity, my reading of a few random pages on the internet is that it would be acceptable for the modem to randomly generate it? (under the assumption that e.g. a reset would clear it and get me a new mac) Or does it have to be assigned as late as possible, e.g. we'd want linux to be generating it in this case? Thanks,
> > The point about locally administered MAC addresses is that they are > > locally administered. > > Honest question here our of curiosity, my reading of a few random pages > on the internet is that it would be acceptable for the modem to randomly > generate it? > (under the assumption that e.g. a reset would clear it and get me a new > mac) > > Or does it have to be assigned as late as possible, e.g. we'd want linux > to be generating it in this case? Honest, answer is, i've never read what IEEE says about locally administered MAC addresses. The general pattern in linux is, if there is no alternative, generate a random MAC address in the locally administered range. The sysadmin should be able to change it via ip link set address, and more likely making it 'permanent' by setting it in /etc/network/interfaces or whatever the distribution uses. I would not be too surprised if some MAC drivers are broken and that fails. Since you can change the MAC address at runtime, and you say they do appear to be somewhat random, i don't see why we cannot live with this. It can still be locally administrated if need be. But it is no something we want to see OEMs do, they really should get an OUI. Andrew
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 44179f4e807f..d044dc7b7622 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -178,6 +178,17 @@ int usbnet_get_ethernet_addr(struct usbnet *dev, int iMACAddress) } EXPORT_SYMBOL_GPL(usbnet_get_ethernet_addr); +static bool usbnet_needs_usb_name_format(struct usbnet *dev, struct net_device *net) +{ + /* Point to point devices which don't have a real MAC address + * (or report a fake local one) have historically used the usb%d + * naming. Preserve this.. + */ + return (dev->driver_info->flags & FLAG_POINTTOPOINT) != 0 && + (is_zero_ether_addr(net->dev_addr) || + is_local_ether_addr(net->dev_addr)); +} + static void intr_complete (struct urb *urb) { struct usbnet *dev = urb->context; @@ -1762,13 +1773,10 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) if (status < 0) goto out1; - // 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. + /* heuristic: rename to "eth%d" if we are not sure this link + * is two-host (these links keep "usb%d") */ if ((dev->driver_info->flags & FLAG_ETHER) != 0 && - ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 || - /* somebody touched it*/ - !is_zero_ether_addr(net->dev_addr))) + !usbnet_needs_usb_name_format(dev, net)) strscpy(net->name, "eth%d", sizeof(net->name)); /* WLAN devices should always be named "wlan%d" */ if ((dev->driver_info->flags & FLAG_WLAN) != 0)