Re: [PATCH net] net: usb: usbnet: fix name regression

Re: [PATCH net] net: usb: usbnet: fix name regression

[Dominique MARTINET]

Hi,

Oliver Neukum wrote on Thu, Oct 17, 2024 at 09:18:37AM +0200:
> The fix for MAC addresses broke detection of the naming convention
> because it gave network devices no random MAC before bind()
> was called. This means that the check for the local assignment bit
> was always negative as the address was zeroed from allocation,
> instead of from overwriting the MAC with a unique hardware address.

So we hit the exact inverse problem with this patch: our device ships an
LTE modem which exposes a cdc-ethernet interface that had always been
named usb0, and with this patch it started being named eth1, breaking
too many hardcoded things expecting the name to be usb0 and making our
devices unable to connect to the internet after updating the kernel.


Long term we'll probably add an udev rule or something to make the name
explicit in userspace and not risk this happening again, but perhaps
there's a better way to keep the old behavior?

(In particular this hit all stable kernels last month so I'm sure we
won't be the only ones getting annoyed with this... Perhaps reverting
both patches for stable branches might make sense if no better way
forward is found -- I've added stable@ in cc for heads up/opinions)


> +++ b/drivers/net/usb/usbnet.c
> @@ -1767,7 +1767,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))
> +		     /* somebody touched it*/
> +		     !is_zero_ether_addr(net->dev_addr)))

... or actually now I'm looking at it again, perhaps is the check just
backwards, or am I getting this wrong?
previous check was rename if (mac[0] & 0x2 == 0), which reads to me as
"nobody set the 2nd bit"
new check now renames if !is_zero, so renames if it was set, which is
the opposite?...

>  			strscpy(net->name, "eth%d", sizeof(net->name));
>  		/* WLAN devices should always be named "wlan%d" */

Thanks,
-- 
Dominique Martinet

Re: [PATCH net] net: usb: usbnet: fix name regression

[Greg Kroah-Hartman]

On Mon, Dec 02, 2024 at 12:50:15PM +0900, Dominique MARTINET wrote:
> Hi,
> 
> Oliver Neukum wrote on Thu, Oct 17, 2024 at 09:18:37AM +0200:
> > The fix for MAC addresses broke detection of the naming convention
> > because it gave network devices no random MAC before bind()
> > was called. This means that the check for the local assignment bit
> > was always negative as the address was zeroed from allocation,
> > instead of from overwriting the MAC with a unique hardware address.
> 
> So we hit the exact inverse problem with this patch: our device ships an
> LTE modem which exposes a cdc-ethernet interface that had always been
> named usb0, and with this patch it started being named eth1, breaking
> too many hardcoded things expecting the name to be usb0 and making our
> devices unable to connect to the internet after updating the kernel.
> 
> 
> Long term we'll probably add an udev rule or something to make the name
> explicit in userspace and not risk this happening again, but perhaps
> there's a better way to keep the old behavior?
> 
> (In particular this hit all stable kernels last month so I'm sure we
> won't be the only ones getting annoyed with this... Perhaps reverting
> both patches for stable branches might make sense if no better way
> forward is found -- I've added stable@ in cc for heads up/opinions)

Device names have NEVER been stable.  They move around and can change on
every boot, let alone almost always changing between kernel versions.
That's why we created udev, to solve this issue.

But how is changing the MAC address changing the device naming here?
How are they linked to suddenly causing the names to switch around?

thanks,

greg k-h

Re: [PATCH net] net: usb: usbnet: fix name regression

[Dominique MARTINET]

Greg Kroah-Hartman wrote on Mon, Dec 02, 2024 at 07:29:52AM +0100:
> On Mon, Dec 02, 2024 at 12:50:15PM +0900, Dominique MARTINET wrote:
> > So we hit the exact inverse problem with this patch: our device ships an
> > LTE modem which exposes a cdc-ethernet interface that had always been
> > named usb0, and with this patch it started being named eth1, breaking
> > too many hardcoded things expecting the name to be usb0 and making our
> > devices unable to connect to the internet after updating the kernel.
> > 
> > 
> > Long term we'll probably add an udev rule or something to make the name
> > explicit in userspace and not risk this happening again, but perhaps
> > there's a better way to keep the old behavior?
> > 
> > (In particular this hit all stable kernels last month so I'm sure we
> > won't be the only ones getting annoyed with this... Perhaps reverting
> > both patches for stable branches might make sense if no better way
> > forward is found -- I've added stable@ in cc for heads up/opinions)
> 
> Device names have NEVER been stable.  They move around and can change on
> every boot, let alone almost always changing between kernel versions.
> That's why we created udev, to solve this issue.

Yes, I agree and we will add an udev rule to enforce the name for later
updates (I really am just a messenger here as "the kernel guy", after
having been asked why did this change), and I have no problem with this
behavior changing on master whatever the direction this takes
(... assuming the check was written as intended)

Now you're saying this I guess the main reason we were affected is that
alpine never made the "stable network interface name" systemd-udev
switch, so for most people that interface will just be named
enx028072781510 anyway and most people will probably not notice this...
(But then again these don't use the "new" name either, so they just
don't care)


With that said, and while I agree the names can change, I still think
there's some hierarchy here -- the X part of ethX/usbX can change on
every boot and I have zero problem with that, but I wouldn't expect the
"type" part to change so easily, and I would have assume stable kernels
would want to at least try to preserve these unless there is a good
reason not to.
The two commits here (8a7d12d674ac ("net: usb: usbnet: fix name
regression") and bab8eb0dd4cb ("usbnet: modern method to get random
MAC") before it) are just cleanup I see no problem reverting them for
stable kernels if they cause any sort of issue, regardless of how
userspace "should" work.


> But how is changing the MAC address changing the device naming here?
> How are they linked to suddenly causing the names to switch around?

That's also something I'd like to understand :)

Apparently, usbnet had a rule that said that if a device is ethernet,
and either (it's not point-to-point) or (mac[0] & 0x2 == 0) then we
would rename it to ethX instad of the usbX name.

commit 8a7d12d674ac ("net: usb: usbnet: fix name regression") made it so
the last part of the check would rename it to ethX if the mac has been
set by any driver, so my understanding is that all drivers that used to
set the mac to something that avoided the 0x2 would now get renamed?...
But as you can see above from the stable device name I gave, the mac
address does start with 02, so I don't understand why it hadn't been
renamed before or what this rules mean and why it's here...?

The commit message mentions commit bab8eb0dd4cb ("usbnet: modern method
to get random MAC") which changed the timing usbnet would set a random
mac, but in my case the mac does come from the hardware (it's stable
across reboots), so I guess I wasn't affected by that commit but the new
one trying to make it better for people with a random mac made it worse
for my case?


Anyway, as said above we'll try to figure something for udev, and this
will hopefully be a heads up for anyone else falling here doing a web
search.
(Our users are rather adverse to changes so I don't see myself enabling
static interface names anytime soon, but time will tell how that turns
out...)

Cheers,
-- 
Dominique

RE: [PATCH net] net: usb: usbnet: fix name regression

[David Laight]

From: Dominique MARTINET
> Sent: 02 December 2024 03:50
> 
> Hi,
> 
> Oliver Neukum wrote on Thu, Oct 17, 2024 at 09:18:37AM +0200:
> > The fix for MAC addresses broke detection of the naming convention
> > because it gave network devices no random MAC before bind()
> > was called. This means that the check for the local assignment bit
> > was always negative as the address was zeroed from allocation,
> > instead of from overwriting the MAC with a unique hardware address.
> 
> So we hit the exact inverse problem with this patch: our device ships an
> LTE modem which exposes a cdc-ethernet interface that had always been
> named usb0, and with this patch it started being named eth1, breaking
> too many hardcoded things expecting the name to be usb0 and making our
> devices unable to connect to the internet after updating the kernel.

Erm does that mean your modem has a locally administered MAC address?
It really shouldn't.

> Long term we'll probably add an udev rule or something to make the name
> explicit in userspace and not risk this happening again, but perhaps
> there's a better way to keep the old behavior?
> 
> (In particular this hit all stable kernels last month so I'm sure we
> won't be the only ones getting annoyed with this... Perhaps reverting
> both patches for stable branches might make sense if no better way
> forward is found -- I've added stable@ in cc for heads up/opinions)
> 
> 
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -1767,7 +1767,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))
> > +		     /* somebody touched it*/
> > +		     !is_zero_ether_addr(net->dev_addr)))
> 
> ... or actually now I'm looking at it again, perhaps is the check just
> backwards, or am I getting this wrong?
> previous check was rename if (mac[0] & 0x2 == 0), which reads to me as
> "nobody set the 2nd bit"
> new check now renames if !is_zero, so renames if it was set, which is
> the opposite?...

The 2nd bit (aka mac[0] & 2) is the 'locally administered' bit.
The intention of the standard was that all manufacturers would get
a valid 14-bit OUI and the EEPROM (or equivalent) would contain an
addresses with that bit clear, such addresses should be globally unique.
Alternatively the local network administrator could assign an address
with that bit set, required by protocols like DECnet.

This has never actually been strictly true, a few manufacturers used
'locally administered addresses' (02:cf:1f:xx:xx:xx comes to mind)
and systems typically allow any (non-broadcast) be set.

So basing any decision on whether a MAC address is local or global
is always going to be confusing.

Linux will allocate a random (locally administered) address if none
is found - usually due to a corrupt eeprom.

	David

> 
> >  			strscpy(net->name, "eth%d", sizeof(net->name));
> >  		/* WLAN devices should always be named "wlan%d" */
> 
> Thanks,
> --
> Dominique Martinet

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net] net: usb: usbnet: fix name regression

['Dominique MARTINET']

(Sorry for back-to-back replies, hadn't noticed this when I wrote the
previous mail)

David Laight wrote on Mon, Dec 02, 2024 at 08:17:59AM +0000:
> > So we hit the exact inverse problem with this patch: our device ships an
> > LTE modem which exposes a cdc-ethernet interface that had always been
> > named usb0, and with this patch it started being named eth1, breaking
> > too many hardcoded things expecting the name to be usb0 and making our
> > devices unable to connect to the internet after updating the kernel.
> 
> Erm does that mean your modem has a locally administered MAC address?
> It really shouldn't.

Unfortunately, that's what it gives us:
# ip a show dev eth1
4: eth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
    link/ether 02:80:72:78:15:10 brd ff:ff:ff:ff:ff:ff

# udevadm info /sys/class/net/eth1
P: /devices/platform/soc/2100000.bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1:1.0/net/eth1
E: DEVPATH=/devices/platform/soc/2100000.bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1:1.0/net/eth1
E: ID_BUS=usb
E: ID_MM_CANDIDATE=1
E: ID_MODEL=GTO
E: ID_MODEL_ENC=GTO
E: ID_MODEL_ID=00a0
E: ID_NET_NAME_MAC=enx028072781510
E: ID_REVISION=0307
E: ID_SERIAL=Gemalto_GTO_101578728002
E: ID_SERIAL_SHORT=101578728002
E: ID_TYPE=generic
E: ID_USB_DRIVER=cdc_ether
E: ID_USB_INTERFACES=:020600:0a0000:020201:
E: ID_USB_INTERFACE_NUM=00
E: ID_VENDOR=Gemalto
E: ID_VENDOR_ENC=Gemalto
E: ID_VENDOR_ID=1e2d
E: IFINDEX=4
E: INTERFACE=eth1
E: NM_UNMANAGED=1
E: SUBSYSTEM=net
E: USEC_INITIALIZED=23339186

(that mac is stable accross reboots)

afaiu, this modem generates a pointtopoint ethernet device and then
routes packets sent there to LTE.

I've just checked what my phone does when I do USB tethering and it's
pretty similar, the mac is also a local 02:xx mac (02:36:05) which is
also not in OUI lookup tables.
(the only difference is, I plugged it on my laptop which runs systemd,
so it got named enx023605xxxxxx and none of this matters..)

> > > +++ b/drivers/net/usb/usbnet.c
> > > @@ -1767,7 +1767,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))
> > > +		     /* somebody touched it*/
> > > +		     !is_zero_ether_addr(net->dev_addr)))
> > 
> > ... or actually now I'm looking at it again, perhaps is the check just
> > backwards, or am I getting this wrong?
> > previous check was rename if (mac[0] & 0x2 == 0), which reads to me as
> > "nobody set the 2nd bit"
> > new check now renames if !is_zero, so renames if it was set, which is
> > the opposite?...
> 
> The 2nd bit (aka mac[0] & 2) is the 'locally administered' bit.
> The intention of the standard was that all manufacturers would get
> a valid 14-bit OUI and the EEPROM (or equivalent) would contain an
> addresses with that bit clear, such addresses should be globally unique.
> Alternatively the local network administrator could assign an address
> with that bit set, required by protocols like DECnet.
> 
> This has never actually been strictly true, a few manufacturers used
> 'locally administered addresses' (02:cf:1f:xx:xx:xx comes to mind)
> and systems typically allow any (non-broadcast) be set.
> 
> So basing any decision on whether a MAC address is local or global
> is always going to be confusing.

Thank you for the explanation, I now understand the old check better --
point to point devices that are a local MAC addresses (0x2 set) would
get to keep the usb0 name.

The new check however no longer cares about the address globality, and
just basically always renames the interface if the driver provided a
mac ?

If that is what was intended, I am fine with this, but I think these
local ppp usb interfaces are rather common in the cheap modem world.


> Linux will allocate a random (locally administered) address if none
> is found - usually due to a corrupt eeprom.

Thanks,
-- 
Dominique

Re: [PATCH net] net: usb: usbnet: fix name regression

[Jakub Kicinski]

On Mon, 2 Dec 2024 17:36:51 +0900 'Dominique MARTINET' wrote:
> The new check however no longer cares about the address globality, and
> just basically always renames the interface if the driver provided a
> mac ?

Any way we can identify those devices and not read the address from 
the device? Reading a locally administered address from the device
seems rather pointless, we can generate one ourselves.

> If that is what was intended, I am fine with this, but I think these
> local ppp usb interfaces are rather common in the cheap modem world.

Which will work, as long as they are marked appropriately; that is
marked with FLAG_POINTTOPOINT.

Re: [PATCH net] net: usb: usbnet: fix name regression

['Dominique MARTINET']

Jakub Kicinski wrote on Mon, Dec 02, 2024 at 06:56:00AM -0800:
> On Mon, 2 Dec 2024 17:36:51 +0900 'Dominique MARTINET' wrote:
> > The new check however no longer cares about the address globality, and
> > just basically always renames the interface if the driver provided a
> > mac ?
> 
> Any way we can identify those devices and not read the address from 
> the device? Reading a locally administered address from the device
> seems rather pointless, we can generate one ourselves.

Would you want to regenerate a local address on every boot?

This might not have a properly allocated mac address range but this at
least has a constant mac, so the devices can be easily identified
(without looking at serial properties)
.. I guess the generation might be made to be a hash from ID_USB_SERIAL
or something like that, but if the device already provides something
stable I don't see any reason not to use it?

(With that said, I don't see anything in `udevadm info` that'd easily
point at this being a modem point to point device under the wraps..)

> > If that is what was intended, I am fine with this, but I think these
> > local ppp usb interfaces are rather common in the cheap modem world.
> 
> Which will work, as long as they are marked appropriately; that is
> marked with FLAG_POINTTOPOINT.

Hmm, but the check here was either FLAG_POINTTOPOINT being unset or not
locally administered address, so to keep the usb0 name we need both?

>             if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
>                 ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
> -                (net->dev_addr [0] & 0x02) == 0))
> +                /* somebody touched it*/
> +                !is_zero_ether_addr(net->dev_addr)))
>                       strscpy(net->name, "eth%d", sizeof(net->name));

i.e., something that didn't have FLAG_POINTTOPOINT in the first place
would not get into this mac consideration, so it must be set.

My problematic device here has FLAG_POINTTOPOINT and a (locally
admistered) mac address set, so it was not renamed up till now,
but the new check makes the locally admistered mac address being set
mean that it is no longer eligible to keep the usbX name.

... Would this check just be fine without any mac check at all?
e.g. anything that's not flagged as point to point will be renamed ethX,
and all non ethernet or point to point keep usbX.

-- 
Dominique

Re: [PATCH net] net: usb: usbnet: fix name regression

[Jakub Kicinski]

On Tue, 3 Dec 2024 08:39:47 +0900 'Dominique MARTINET' wrote:
> > > If that is what was intended, I am fine with this, but I think these
> > > local ppp usb interfaces are rather common in the cheap modem world.  
> > 
> > Which will work, as long as they are marked appropriately; that is
> > marked with FLAG_POINTTOPOINT.  
> 
> Hmm, but the check here was either FLAG_POINTTOPOINT being unset or not
> locally administered address, so to keep the usb0 name we need both?
> 
> >             if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
> >                 ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 ||
> > -                (net->dev_addr [0] & 0x02) == 0))
> > +                /* somebody touched it*/
> > +                !is_zero_ether_addr(net->dev_addr)))
> >                       strscpy(net->name, "eth%d", sizeof(net->name));  
> 
> i.e., something that didn't have FLAG_POINTTOPOINT in the first place
> would not get into this mac consideration, so it must be set.

Right! I missed the && plus ||

> My problematic device here has FLAG_POINTTOPOINT and a (locally
> admistered) mac address set, so it was not renamed up till now,
> but the new check makes the locally admistered mac address being set
> mean that it is no longer eligible to keep the usbX name.

Ideally, udev would be the best option, like Greg said.
This driver is already a fragile pile of workarounds.

If you really really want the old behavior tho, let's convert 
the zero check to  !is_zero_ether_addr() && !is_local_ether_addr().
Maybe factor out the P2P + address validation to a helper because
the && vs || is getting complicated.

Re: [PATCH net] net: usb: usbnet: fix name regression

['Dominique MARTINET']

Jakub Kicinski wrote on Mon, Dec 02, 2024 at 04:26:53PM -0800:
> > My problematic device here has FLAG_POINTTOPOINT and a (locally
> > admistered) mac address set, so it was not renamed up till now,
> > but the new check makes the locally admistered mac address being set
> > mean that it is no longer eligible to keep the usbX name.
> 
> Ideally, udev would be the best option, like Greg said.
> This driver is already a fragile pile of workarounds.

Right, as I replied to Greg I'm fine with this as long as it's what was
intended.

Half of the reason I sent the mail in the first place is I don't
understand what commit 8a7d12d674ac ("net: usb: usbnet: fix name
regression") actually fixes: the commit message desribes something about
mac address not being set before bind() but the code does not change
what address is looked at (net->dev_addr), just which bits of the
address is checked; and I don't see what which bytes are being looked at
changing has anything to do with the "fixed" commit bab8eb0dd4cb9 ("usbnet:
modern method to get random MAC")
... And now we've started discussing this and I understand the check
better, I also don't see what having a mac set by the previous driver
has to do with the link not being P2P either.


(The other half was I was wondering what kind of policy stable would have
for this kind of things, but that was made clear enough)


> If you really really want the old behavior tho, let's convert 
> the zero check to  !is_zero_ether_addr() && !is_local_ether_addr().

As far as I understand, !is_local_ether_addr (mac[0] & 0x2) implies
!is_zero_ether_addr (all bits of mac or'd), so that'd get us back to
exactly the old check.

> Maybe factor out the P2P + address validation to a helper because
> the && vs || is getting complicated.

... And I can definitely relate to this part :)

So:
- final behavior wise I have no strong feeling, we'll fix our userspace
(... and documentation) whatever is decided here
- let's improve the comment and factor the check anyway.
As said above I don't understand why having a mac set matters, if that
can be explained I'll be happy to send a patch.
Or if we go with the local address version, something like the
following?

----
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 44179f4e807f..240ae86adf08 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -178,6 +178,13 @@ int usbnet_get_ethernet_addr(struct usbnet *dev, int iMACAddress)
 }
 EXPORT_SYMBOL_GPL(usbnet_get_ethernet_addr);
 
+static bool usbnet_dev_is_two_host (struct usbnet *dev, struct net_device *net)
+{
+	/* device is marked point-to-point with a local mac address */
+	return (dev->driver_info->flags & FLAG_POINTTOPOINT) != 0 &&
+		is_local_ether_addr(net->dev_addr);
+}
+
 static void intr_complete (struct urb *urb)
 {
 	struct usbnet	*dev = urb->context;
@@ -1762,13 +1769,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_dev_is_two_host(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)
----

Thanks,
-- 
Dominique

Re: [PATCH net] net: usb: usbnet: fix name regression

[Jakub Kicinski]

On Tue, 3 Dec 2024 10:18:44 +0900 'Dominique MARTINET' wrote:
> Jakub Kicinski wrote on Mon, Dec 02, 2024 at 04:26:53PM -0800:
> > > My problematic device here has FLAG_POINTTOPOINT and a (locally
> > > admistered) mac address set, so it was not renamed up till now,
> > > but the new check makes the locally admistered mac address being set
> > > mean that it is no longer eligible to keep the usbX name.  
> > 
> > Ideally, udev would be the best option, like Greg said.
> > This driver is already a fragile pile of workarounds.  
> 
> Right, as I replied to Greg I'm fine with this as long as it's what was
> intended.

In theory unintentional != bug, but yes, its very likely unintentional.

> Half of the reason I sent the mail in the first place is I don't
> understand what commit 8a7d12d674ac ("net: usb: usbnet: fix name
> regression") actually fixes: the commit message desribes something about
> mac address not being set before bind() but the code does not change
> what address is looked at (net->dev_addr), just which bits of the
> address is checked; and I don't see what which bytes are being looked at
> changing has anything to do with the "fixed" commit bab8eb0dd4cb9 ("usbnet:
> modern method to get random MAC")

We moved where the random address is assigned, we used to assign random
(local) addr at init, now we assign it after calling ->bind().

Previously we checked "if local" as a shorthand for checking "if driver
updated". This check should really have been "if addr == node_id".

> ... And now we've started discussing this and I understand the check
> better, I also don't see what having a mac set by the previous driver
> has to do with the link not being P2P either.
> 
> 
> (The other half was I was wondering what kind of policy stable would have
> for this kind of things, but that was made clear enough)
> 
> 
> > If you really really want the old behavior tho, let's convert 
> > the zero check to  !is_zero_ether_addr() && !is_local_ether_addr().  
> 
> As far as I understand, !is_local_ether_addr (mac[0] & 0x2) implies
> !is_zero_ether_addr (all bits of mac or'd), so that'd get us back to
> exactly the old check.

Let the compiler discover that, this is control path code, so write
it for the human reader... The condition we want is "driver did not
initialize the MAC address, or it initialized it to a local MAC
address".

> > Maybe factor out the P2P + address validation to a helper because
> > the && vs || is getting complicated.  
> 
> ... And I can definitely relate to this part :)
> 
> So:
> - final behavior wise I have no strong feeling, we'll fix our userspace
> (... and documentation) whatever is decided here
> - let's improve the comment and factor the check anyway.
> As said above I don't understand why having a mac set matters, if that
> can be explained I'll be happy to send a patch.
> Or if we go with the local address version, something like the
> following?
> 
> ----
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 44179f4e807f..240ae86adf08 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -178,6 +178,13 @@ int usbnet_get_ethernet_addr(struct usbnet *dev, int iMACAddress)
>  }
>  EXPORT_SYMBOL_GPL(usbnet_get_ethernet_addr);
>  
> +static bool usbnet_dev_is_two_host (struct usbnet *dev, struct net_device *net)

static bool usenet_needs_usb_name_format(...

> +{
> +	/* device is marked point-to-point with a local mac address */

	/* 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_local_ether_addr(net->dev_addr);

	if ((dev->driver_info->flags & FLAG_POINTTOPOINT) &&
	    (is_zero_ether_addr(net->dev_addr) ||
	     is_local_ether_addr(net->dev_addr));

> +}

Up to you if you want to send this.

Re: [PATCH net] net: usb: usbnet: fix name regression

['Dominique MARTINET']

Jakub Kicinski wrote on Mon, Dec 02, 2024 at 06:29:35PM -0800:
> > Half of the reason I sent the mail in the first place is I don't
> > understand what commit 8a7d12d674ac ("net: usb: usbnet: fix name
> > regression") actually fixes: the commit message desribes something about
> > mac address not being set before bind() but the code does not change
> > what address is looked at (net->dev_addr), just which bits of the
> > address is checked; and I don't see what which bytes are being looked at
> > changing has anything to do with the "fixed" commit bab8eb0dd4cb9 ("usbnet:
> > modern method to get random MAC")
> 
> We moved where the random address is assigned, we used to assign random
> (local) addr at init, now we assign it after calling ->bind().
> 
> Previously we checked "if local" as a shorthand for checking "if driver
> updated". This check should really have been "if addr == node_id".

Ok, so a zero address here means a driver didn't set it, because the
ex-"node_id" address was no longer set at this point, and these would
fail the 0x2 check that worked previously...

The third time's a charm, the ordering part of the message just clicked
for me, thank you for putting up with me.


> > As far as I understand, !is_local_ether_addr (mac[0] & 0x2) implies
> > !is_zero_ether_addr (all bits of mac or'd), so that'd get us back to
> > exactly the old check.
> 
> Let the compiler discover that, this is control path code, so write
> it for the human reader... The condition we want is "driver did not
> initialize the MAC address, or it initialized it to a local MAC
> address".

(I was reading that '&& !' wrong here, having the check negated in the
helper is much more clear and it's required to keep the two anyway...)


> > Or if we go with the local address version, something like the
> > following?
> [...]
> 
> Up to you if you want to send this.

Thank you; after thinking it through today I think it won't hurt further
to send so I did.
Almost everyone involved is in Cc, but for follow-up it is here:
https://lore.kernel.org/r/20241203130457.904325-1-asmadeus@codewreck.org


Thanks,
-- 
Dominique

Re: [PATCH net] net: usb: usbnet: fix name regression

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

Found matching upstream commit: 8a7d12d674ac6f2147c18f36d1e15f1a48060edf


Status in newer kernel trees:
6.12.y | Present (exact SHA1)

Note: The patch differs from the upstream commit:
---
Failed to apply patch cleanly, falling back to interdiff...
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.12.y       |  Failed     |  N/A       |
| stable/linux-6.11.y       |  Failed     |  N/A       |
| stable/linux-6.6.y        |  Failed     |  N/A       |
| stable/linux-6.1.y        |  Failed     |  N/A       |
| stable/linux-5.15.y       |  Failed     |  N/A       |
| stable/linux-5.10.y       |  Failed     |  N/A       |
| stable/linux-5.4.y        |  Failed     |  N/A       |
| stable/linux-4.19.y       |  Failed     |  N/A       |