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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z08EKwZ3DNb11x_B@atmark-techno.com>
Date: Tue, 3 Dec 2024 22:14:19 +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 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