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: <20241202182935.75e8850c@kernel.org>
Date: Mon, 2 Dec 2024 18:29:35 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: 'Dominique MARTINET' <dominique.martinet@...ark-techno.com>
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

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.