Message-ID: <20220104193455.6b8a21fc@md1za8fc.ad001.siemens.net>
Date:   Tue, 4 Jan 2022 19:34:55 +0100
From:   Henning Schild <henning.schild@...mens.com>
To:     Aaron Ma <aaron.ma@...onical.com>
CC:     Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>,
        <hayeswang@...ltek.com>, <tiwai@...e.de>,
        <linux-usb@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: usb: r8152: Add MAC passthrough support for more
 Lenovo Docks

Am Wed, 5 Jan 2022 01:40:42 +0800
schrieb Aaron Ma <aaron.ma@...onical.com>:

> On 1/5/22 01:07, Henning Schild wrote:
> > Am Tue, 4 Jan 2022 06:53:26 -0800
> > schrieb Jakub Kicinski <kuba@...nel.org>:
> >   
> >> On Tue, 4 Jan 2022 12:38:14 +0100 Henning Schild wrote:  
> >>> This patch is wrong and taking the MAC inheritance way too far.
> >>> Now any USB Ethernet dongle connected to a Lenovo USB Hub will go
> >>> into inheritance (which is meant for docks).
> >>>
> >>> It means that such dongles plugged directly into the laptop will
> >>> do that, or travel adaptors/hubs which are not "active docks".
> >>>
> >>> I have USB-Ethernet dongles on two desks and both stopped working
> >>> as expected because they took the main MAC, even with it being
> >>> used at the same time. The inheritance should (if at all) only be
> >>> done for clearly identified docks and only for one r8152 instance
> >>> ... not all. Maybe even double checking if that main PHY is
> >>> "plugged" and monitoring it to back off as soon as it is.
> >>>
> >>> With this patch applied users can not use multiple ethernet
> >>> devices anymore ... if some of them are r8152 and connected to
> >>> "Lenovo" ... which is more than likely!
> >>>
> >>> Reverting that patch solved my problem, but i later went to
> >>> disabling that very questionable BIOS feature to disable things
> >>> for good without having to patch my kernel.
> >>>
> >>> I strongly suggest to revert that. And if not please drop the
> >>> defines of  
> >>>> -		case DEVICE_ID_THINKPAD_THUNDERBOLT3_DOCK_GEN2:
> >>>> -		case DEVICE_ID_THINKPAD_USB_C_DOCK_GEN2:  
> >>>
> >>> And instead of crapping out with "(unnamed net_device)
> >>> (uninitialized): Invalid header when reading pass-thru MAC addr"
> >>> when the BIOS feature is turned off, one might want to check
> >>> DSDT/WMT1/ITEM/"MACAddressPassThrough" which is my best for asking
> >>> the BIOS if the feature is wanted.  
> >>
> >> Thank you for the report!
> >>
> >> Aaron, will you be able to fix this quickly? 5.16 is about to be
> >> released.  
> > 
> > If you guys agree with a revert and potentially other actions, i
> > would be willing to help. In any case it is not super-urgent since
> > we can maybe agree an regression and push it back into stable
> > kernels.
> > 
> > I first wanted to place the report and see how people would react
> > ... if you guys agree that this is a bug and the inheritance is
> > going "way too far".
> > 
> > But i would only do some repairs on the surface, the feature itself
> > is horrific to say the least and i am very happy with that BIOS
> > switch to ditch it for good. Giving the MAC out is something a dock
> > physically blocking the original PHY could do ... but year ... only
> > once and it might be pretty hard to say which r8152 is built-in
> > from the hub and which is plugged in additionally in that very hub.
> > Not to mention multiple hubs of the same type ... in a nice USB-C
> > chain. 
> 
> Yes, it's expected to be a mess if multiple r8152 are attached to
> Lenovo USB-C/TBT docks. The issue had been discussed for several
> times in LKML. Either lose this feature or add potential risk for
> multiple r8152.
> 
> The idea is to make the Dock work which only ship with one r8152.
> It's really hard to say r8152 is from dock or another plugin one.
> 
> If revert this patch, then most users with the original shipped dock
> may lose this feature. That's the problem this patch try to fix.

I understand that. But i would say people can not expect such a crap
feature on Linux, or we really need very good reasoning to cause MAC
collisions with the real PHY and on top claim ETOOMANY of the dongles.

The other vendors seem to check bits of the "golden" dongle. At least
that is how i understand BD/AD/BND_MASK

How about making it a module param and default to off, and dev_warn if
BIOS has it turned on. That sounds like a reasonable compromise and
whoever turns it on twice probably really wants it. (note that BIOS
defaults to on ... so that was never intended by users, and corporate
users might not be allowed/able to turn that off)

MACs change ... all the time, people should use radius x509. The
request is probably coming from corporate users, and they are all on a
zero trust journey and will eventually stop relying on MACs anyways.

And if ubuntu wants to cater by default, there can always be an udev
rule or setting that module param to "on".

> For now I suggest to disable it in BIOS if you got multiple r8152.
> 
> Let me try to make some changes to limit this feature in one r8152.

Which one? ;) And how to deal with the real NIC once you picked one?
Looking forward, please Cc me.

Henning

> Aaron
> 
> 
> > MAC spoofing is something NetworkManager and others can take care
> > of, or udev ... doing that in the driver is ... spooky.
> > 
> > regards,
> > Henning  

Powered by blists - more mailing lists