Date:   Fri, 28 Jan 2022 22:07:14 +0100
From:   Henning Schild <henning.schild@...mens.com>
To:     "Limonciello, Mario" <Mario.Limonciello@....com>
Cc:     Andrew Lunn <andrew@...n.ch>, Aaron Ma <aaron.ma@...onical.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "hayeswang@...ltek.com" <hayeswang@...ltek.com>,
        "tiwai@...e.de" <tiwai@...e.de>
Subject: Re: [PATCH v3] net: usb: r8152: Add MAC passthrough support for
 RTL8153BL

Am Fri, 28 Jan 2022 20:29:58 +0000
schrieb "Limonciello, Mario" <Mario.Limonciello@....com>:

> [Public]
> 
> > > > I've not yet been convinced by replies that the proposed code
> > > > really does only match the given dock, and not random USB
> > > > dongles.  
> > >
> > > Didn't Realtek confirm this bit is used to identify the Lenovo
> > > devices?  
> > 
> > The question is really why we do all that and that answer really
> > seems missing. Lenovo might have a very different answer than Dell.
> > All i saw was Lenovo docks that suggest PXE. While that could be a
> > good case, bootloaders and OSs should then inherit the MAC of a NIC
> > that is already up. But sure not try and guess what to do and spoof
> > on their own. (based on weird bits and topology guesses) I am
> > willing to bet that grub/pxelinux/uboot/systemd-boot would all fail
> > a PXE run even if the OS to boot would spoof/inherit.
> > 
> > PXE uses spoofed MAC and bootloader will dhcp again and not spoof.
> > dhcp would time out or fall back to somewhere (MAC not known),
> > Linux kernel would not come, PXE boot failed.
> > 
> > In fact i have seen first hand how PXE Windows install failed on a
> > Lenovo dock and the real NIC worked. That was within my company and
> > i sure do not know the BIOS settings or what bootloader was
> > involved and if that Windows installer did active spoofing. But i
> > would say that the PXE story probably does not really "work" for
> > Windows either. In fact i am willing to bet the BIOS setting for
> > spoofing was turned on, because it seems to be the factory default.
> > 
> > And all stories beyond PXE-bootstrap should probably be answered
> > with "a MAC does not identify a machine". So people that care to
> > ident a machine should use something like x509, or allow network
> > access in any other way not relying on a MAC. If "Linux" cares to
> > spoof for the lazy ones, udev is a better place than the kernel.
> >   
> > > > To be
> > > > convinced i would probably like to see code which positively
> > > > identifies the dock, and that the USB device is on the correct
> > > > port of the USB hub within the dock. I doubt you can actually
> > > > do that in a sane way inside an Ethernet driver. As you say, it
> > > > will likely lead to unmaintainable spaghetti-code.
> > > >
> > > > I also don't really think the vendor would be keen on adding
> > > > code which they know will get reverted as soon as it is shown
> > > > to cause a regression.
> > > >
> > > > So i would prefer to NACK this, and push it to udev rules where
> > > > you have a complete picture of the hardware and really can
> > > > identify with 100% certainty it really is the docks NIC.  
> > >
> > > I remember when I did the Dell implementation I tried userspace
> > > first.
> > >
> > > Pushing this out to udev has a few other implications I remember
> > > hitting: 1) You need to also get the value you're supposed to use
> > > from ACPI BIOS exported some way in userland too.  
> > 
> > Sounds like a problem with ACPI in userspace. So the kernel could
> > expose ACPI in a better shape. Or you will simply need to see what
> > systemd thinks about a funny "sed | grep | awk | bc" patch to parse
> > binary. DMI might contain bits, but without clear vendor
> > instructions we would be guessing (like on ACPI?).  
> 
> Yeah I think if this is to be reverted, step 1 is going to be to
> export that data into sysfs from some Dell and Lenovo drivers so
> userspace can get it. No funny sed/grep/awk to parse binary.
> 
> DMI doesn't contain it (at least for Dell).

Would still cause a nasty vendor-lockin where passthrough would only
happen if peripherals match the laptop. In the end users would end up
very confused or Linux would cater for vendors selling their docks.

> >   
> > > 2) You can run into race conditions with other device or MAC
> > > renaming rules. My first try I did it with NM and hit that
> > > continually.  So you would probably need to land this in systemd
> > > or so.  
> > 
> > For sure you would end up in systemd (udev). NM is just one of many
> > options and would be the wrong place. You might quickly find
> > yourself in mdev (busybox) as well because of that PXE case or
> > because of an initrd.
> > 
> > If it was my call i would revert all mac passthough patches and
> > request Lenovo/Dell and others to present their case first hand. (no
> > canonical/amd/suse proxies)
> > The "feature" causes MAC collisions and is not well
> > understood/argued by anyone.  
> 
> Reverting it has the possibility to really mess up machines people
> have in the field that have behavior built around it.  I think a
> clear set of rules for what is allow to use it is the only safe way
> forward.  You need to "clearly identify the device" or something.

Very true, i was kind of hardlining it. But IMHO the clear rule is that
a MAC does not ident a machine. And while that might break already
deployed setups, it will make clear that passthrough is a clear no-go.
If you do not draw that line, many more patches might follow. Claiming
their rightful place next to Dell hacks.

The revert that did already happen actually has potential to "break"
already deployed setups. It was still done because i brought up the
case and people agreed. I can not speak for the Dell patches, they are
in there for much longer and will be more widely deployed. So a revert
will potentially affect for people. But they sure seem equally
questionable and not well explained.

> Just FYI I'm not intentionally acting as a proxy to anyone at Dell on
> behalf of AMD, my only involvement here is because I did the original
> implementation for Dell when I was there and so I can speak the how
> things were and thought processes at that time.

I bet nobody wanted to act as a proxy, it just happened somehow. If
that proxy can explain in detail why and how that is fine.

Since you have been at Dell and started that, do you happen to know the
real story? Like real ... not "i had to solve jira ticket 42"

> If the consensus is to revert this then someone from Dell probably
> does need to speak up.

If you do not cater for Dell any longer, maybe you would be just the
person to revert and tell them to reveal what "jira 42" was about.
Merge windows are wide open and distros will be slow to follow, so
enough time to react after a revert.

regards,
Henning

> > 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikip
> > edia.org%2Fwiki%2FMAC_address&amp;data=04%7C01%7CMario.Limonciello%
> > 40amd.com%7C385b6dd02672490749d208d9e29ba192%7C3dd8961fe4884e60
> > 8e11a82d994e183d%7C0%7C0%7C637789980357612909%7CUnknown%7CTWF
> > pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> > Mn0%3D%7C3000&amp;sdata=9vmPi1%2FF%2BxrkqpjoYoLmmupAJ1vBMckLLo
> > 5hDMqTuZA%3D&amp;reserved=0
> >   
> > > A media access control address (MAC address) is a unique
> > > identifier assigned to a network interface controller (NIC) ...  
> > 
> > unique and NIC, as opposed to colliding and machine
> > 
> > Henning
> >   
> > > >
> > > >    Andre  

Powered by blists - more mailing lists