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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z05cdCEgqyea-qBD@atmark-techno.com>
Date: Tue, 3 Dec 2024 10:18:44 +0900
From: 'Dominique MARTINET' <dominique.martinet@...ark-techno.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: David Laight <David.Laight@...lab.com>,
	Oliver Neukum <oneukum@...e.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Greg Thelen <gthelen@...gle.com>,
	John Sperbeck <jsperbeck@...gle.com>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH net] net: usb: usbnet: fix name regression

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