[PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

When plugin multiple r8152 ethernet dongles to Lenovo Docks
or USB hub, MAC passthrough address from BIOS should be
checked if it had been used to avoid using on other dongles.

Currently builtin r8152 on Dock still can't be identified.
First detected r8152 will use the MAC passthrough address.

v2:
Skip builtin PCI MAC address which is share MAC address with
passthrough MAC.
Check thunderbolt based ethernet.

v3:
Add return value.

Fixes: f77b83b5bbab ("net: usb: r8152: Add MAC passthrough support for
more Lenovo Docks")
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/net/usb/r8152.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f9877a3e83ac..2483dc421dff 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -25,6 +25,7 @@
 #include <linux/atomic.h>
 #include <linux/acpi.h>
 #include <linux/firmware.h>
+#include <linux/pci.h>
 #include <crypto/hash.h>
 #include <linux/usb/r8152.h>
 
@@ -1605,6 +1606,7 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	char *mac_obj_name;
 	acpi_object_type mac_obj_type;
 	int mac_strlen;
+	struct net_device *ndev;
 
 	if (tp->lenovo_macpassthru) {
 		mac_obj_name = "\\MACA";
@@ -1662,6 +1664,19 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 		ret = -EINVAL;
 		goto amacout;
 	}
+	rcu_read_lock();
+	for_each_netdev_rcu(&init_net, ndev) {
+		if (ndev->dev.parent && dev_is_pci(ndev->dev.parent) &&
+				!pci_is_thunderbolt_attached(to_pci_dev(ndev->dev.parent)))
+			continue;
+		if (strncmp(buf, ndev->dev_addr, 6) == 0) {
+			ret = -EINVAL;
+			rcu_read_unlock();
+			goto amacout;
+		}
+	}
+	rcu_read_unlock();
+
 	memcpy(sa->sa_data, buf, 6);
 	netif_info(tp, probe, tp->netdev,
 		   "Using pass-thru MAC addr %pM\n", sa->sa_data);
-- 
2.30.2

[PATCH 2/3] net: usb: r8152: Set probe mode to sync

[Aaron Ma]

To avoid the race of get passthrough MAC,
set probe mode to sync to check the used MAC address.

Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/net/usb/r8152.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2483dc421dff..7cf2faf8d088 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -29,6 +29,8 @@
 #include <crypto/hash.h>
 #include <linux/usb/r8152.h>
 
+static struct usb_driver rtl8152_driver;
+
 /* Information for net-next */
 #define NETNEXT_VERSION		"12"
 
@@ -9546,6 +9548,9 @@ static int rtl8152_probe(struct usb_interface *intf,
 	struct r8152 *tp;
 	struct net_device *netdev;
 	int ret;
+	struct device_driver *rtl8152_drv = &rtl8152_driver.drvwrap.driver;
+
+	rtl8152_drv->probe_type = PROBE_FORCE_SYNCHRONOUS;
 
 	if (version == RTL_VER_UNKNOWN)
 		return -ENODEV;
-- 
2.30.2

Re: [PATCH 2/3] net: usb: r8152: Set probe mode to sync

[Greg KH]

On Wed, Jan 05, 2022 at 11:14:26PM +0800, Aaron Ma wrote:
> To avoid the race of get passthrough MAC,
> set probe mode to sync to check the used MAC address.
> 
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 2483dc421dff..7cf2faf8d088 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -29,6 +29,8 @@
>  #include <crypto/hash.h>
>  #include <linux/usb/r8152.h>
>  
> +static struct usb_driver rtl8152_driver;
> +
>  /* Information for net-next */
>  #define NETNEXT_VERSION		"12"
>  
> @@ -9546,6 +9548,9 @@ static int rtl8152_probe(struct usb_interface *intf,
>  	struct r8152 *tp;
>  	struct net_device *netdev;
>  	int ret;
> +	struct device_driver *rtl8152_drv = &rtl8152_driver.drvwrap.driver;
> +
> +	rtl8152_drv->probe_type = PROBE_FORCE_SYNCHRONOUS;

If you really need to set this type of thing then set BEFORE you
register the driver.  After-the-fact like this is way too late, sorry.
You are already in the probe function which is after the driver core
checked this flag :(

How did you test this?

thanks,

greg k-h

[PATCH 3/3] net: usb: r8152: remove unused definition

[Aaron Ma]

Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/net/usb/r8152.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 7cf2faf8d088..7cd3b1db062a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -773,9 +773,6 @@ enum rtl8152_flags {
 	RX_EPROTO,
 };
 
-#define DEVICE_ID_THINKPAD_THUNDERBOLT3_DOCK_GEN2	0x3082
-#define DEVICE_ID_THINKPAD_USB_C_DOCK_GEN2		0xa387
-
 struct tally_counter {
 	__le64	tx_packets;
 	__le64	rx_packets;
-- 
2.30.2

Re: [PATCH 3/3] net: usb: r8152: remove unused definition

[Greg KH]

On Wed, Jan 05, 2022 at 11:14:27PM +0800, Aaron Ma wrote:
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>

Again, not good, you know better than to not provide a changelog text.

thanks,

greg k-h

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Greg KH]

On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> or USB hub, MAC passthrough address from BIOS should be
> checked if it had been used to avoid using on other dongles.
> 
> Currently builtin r8152 on Dock still can't be identified.
> First detected r8152 will use the MAC passthrough address.
> 
> v2:
> Skip builtin PCI MAC address which is share MAC address with
> passthrough MAC.
> Check thunderbolt based ethernet.
> 
> v3:
> Add return value.

All of this goes below the --- line.

You have read the kernel documentation for how to do all of this, right?
If not, please re-read it.

> 
> Fixes: f77b83b5bbab ("net: usb: r8152: Add MAC passthrough support for
> more Lenovo Docks")

This line should not be wrapped.



> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index f9877a3e83ac..2483dc421dff 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -25,6 +25,7 @@
>  #include <linux/atomic.h>
>  #include <linux/acpi.h>
>  #include <linux/firmware.h>
> +#include <linux/pci.h>

Why does a USB driver care about PCI stuff?

>  #include <crypto/hash.h>
>  #include <linux/usb/r8152.h>
>  
> @@ -1605,6 +1606,7 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
>  	char *mac_obj_name;
>  	acpi_object_type mac_obj_type;
>  	int mac_strlen;
> +	struct net_device *ndev;
>  
>  	if (tp->lenovo_macpassthru) {
>  		mac_obj_name = "\\MACA";
> @@ -1662,6 +1664,19 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
>  		ret = -EINVAL;
>  		goto amacout;
>  	}
> +	rcu_read_lock();
> +	for_each_netdev_rcu(&init_net, ndev) {
> +		if (ndev->dev.parent && dev_is_pci(ndev->dev.parent) &&

Ick ick ick.

No, don't go poking around in random parent devices of a USB device,
that is a sure way to break things.

> +				!pci_is_thunderbolt_attached(to_pci_dev(ndev->dev.parent)))

So thunderbolt USB hubs are a problem here?

No, this is not the correct way to handle this at all.  There should be
some sort of identifier on the USB device itself to say that it is in a
docking station and needs to have special handling.  If not, then the
docking station is broken and needs to be returned.

Can you please revert the offending patch first so that people's systems
go back to working properly first?  Then worry about trying to uniquely
identify these crazy devices.

Again, this is not a way to do it, sorry.

greg k-h

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Ok this now will claim only the first plugged in dongle. Probably as
you wanted it. But still breaking my always two ethernet cables and no
"lenovo dock" anywhere.

Still feels very wrong to me, but i have no clue how it is supposed to
work. Lenovo documentation talks about PXE use-cases ... which means
BIOS is spoofing and not OS. OS should probably just take the active
MAC instead of the one from EEPROM.
But it also has a link to one "dock" that is really just a dongle. And
the whole thing is super dated.

https://support.lenovo.com/ie/en/solutions/ht503574

Henning

Am Wed,  5 Jan 2022 23:14:25 +0800
schrieb Aaron Ma <aaron.ma@canonical.com>:

> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> or USB hub, MAC passthrough address from BIOS should be
> checked if it had been used to avoid using on other dongles.
> 
> Currently builtin r8152 on Dock still can't be identified.
> First detected r8152 will use the MAC passthrough address.
> 
> v2:
> Skip builtin PCI MAC address which is share MAC address with
> passthrough MAC.
> Check thunderbolt based ethernet.
> 
> v3:
> Add return value.
> 
> Fixes: f77b83b5bbab ("net: usb: r8152: Add MAC passthrough support for
> more Lenovo Docks")
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index f9877a3e83ac..2483dc421dff 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -25,6 +25,7 @@
>  #include <linux/atomic.h>
>  #include <linux/acpi.h>
>  #include <linux/firmware.h>
> +#include <linux/pci.h>
>  #include <crypto/hash.h>
>  #include <linux/usb/r8152.h>
>  
> @@ -1605,6 +1606,7 @@ static int vendor_mac_passthru_addr_read(struct
> r8152 *tp, struct sockaddr *sa) char *mac_obj_name;
>  	acpi_object_type mac_obj_type;
>  	int mac_strlen;
> +	struct net_device *ndev;
>  
>  	if (tp->lenovo_macpassthru) {
>  		mac_obj_name = "\\MACA";
> @@ -1662,6 +1664,19 @@ static int
> vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
> ret = -EINVAL; goto amacout;
>  	}
> +	rcu_read_lock();
> +	for_each_netdev_rcu(&init_net, ndev) {
> +		if (ndev->dev.parent && dev_is_pci(ndev->dev.parent)
> &&
> +
> !pci_is_thunderbolt_attached(to_pci_dev(ndev->dev.parent)))
> +			continue;
> +		if (strncmp(buf, ndev->dev_addr, 6) == 0) {
> +			ret = -EINVAL;
> +			rcu_read_unlock();
> +			goto amacout;
> +		}
> +	}
> +	rcu_read_unlock();
> +
>  	memcpy(sa->sa_data, buf, 6);
>  	netif_info(tp, probe, tp->netdev,
>  		   "Using pass-thru MAC addr %pM\n", sa->sa_data);

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Andrew Lunn]

On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> or USB hub, MAC passthrough address from BIOS should be
> checked if it had been used to avoid using on other dongles.
> 
> Currently builtin r8152 on Dock still can't be identified.
> First detected r8152 will use the MAC passthrough address.

I do have to wonder why you are doing this in the kernel, and not
using a udev rule? This seems to be policy, and policy does not belong
in the kernel.

   Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Am Wed, 5 Jan 2022 18:30:08 +0100
schrieb Andrew Lunn <andrew@lunn.ch>:

> On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> > When plugin multiple r8152 ethernet dongles to Lenovo Docks
> > or USB hub, MAC passthrough address from BIOS should be
> > checked if it had been used to avoid using on other dongles.
> > 
> > Currently builtin r8152 on Dock still can't be identified.
> > First detected r8152 will use the MAC passthrough address.  
> 
> I do have to wonder why you are doing this in the kernel, and not
> using a udev rule? This seems to be policy, and policy does not belong
> in the kernel.

Yes, the whole pass-thru story should not be a kernel feature in the
first place, i could not agree more, udev would be the way better place!
But it is part of the driver and Aaron did not introduce it but just
extend it.

Henning

>    Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Oliver Neukum]

On 05.01.22 18:30, Andrew Lunn wrote:
> On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
>> When plugin multiple r8152 ethernet dongles to Lenovo Docks
>> or USB hub, MAC passthrough address from BIOS should be
>> checked if it had been used to avoid using on other dongles.
>>
>> Currently builtin r8152 on Dock still can't be identified.
>> First detected r8152 will use the MAC passthrough address.
> I do have to wonder why you are doing this in the kernel, and not
> using a udev rule? This seems to be policy, and policy does not belong
> in the kernel.
Debatable. An ethernet NIC has to have a MAC. The kernel must
provide one. That we should always take the one found in the
device's ROM rather than the host's ROM is already a policy decision.

In fact we make up a MAC for devices that do not have one.

    Regards
        Oliver

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Andrew Lunn]

On Wed, Jan 05, 2022 at 10:49:56PM +0100, Oliver Neukum wrote:
> 
> On 05.01.22 18:30, Andrew Lunn wrote:
> > On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> >> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> >> or USB hub, MAC passthrough address from BIOS should be
> >> checked if it had been used to avoid using on other dongles.
> >>
> >> Currently builtin r8152 on Dock still can't be identified.
> >> First detected r8152 will use the MAC passthrough address.
> > I do have to wonder why you are doing this in the kernel, and not
> > using a udev rule? This seems to be policy, and policy does not belong
> > in the kernel.
> Debatable. An ethernet NIC has to have a MAC. The kernel must
> provide one. That we should always take the one found in the
> device's ROM rather than the host's ROM is already a policy decision.

In general, it is a much longer list of places to find the MAC address
from. It could be in three different places in device tree, it could
be in ACPI in a similar number of places, it could be in NVMEM,
pointed to by device tree, the bootloader might of already programmed
the controller with its MAC address, etc, or if everything else fails,
it could be random.

So yes, the kernel will give it one. But if you want the interface to
have a specific MAC address, you probably should not be trusting the
kernel, given it has so many options.

	Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Kai-Heng Feng]

On Thu, Jan 6, 2022 at 6:27 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Jan 05, 2022 at 10:49:56PM +0100, Oliver Neukum wrote:
> >
> > On 05.01.22 18:30, Andrew Lunn wrote:
> > > On Wed, Jan 05, 2022 at 11:14:25PM +0800, Aaron Ma wrote:
> > >> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> > >> or USB hub, MAC passthrough address from BIOS should be
> > >> checked if it had been used to avoid using on other dongles.
> > >>
> > >> Currently builtin r8152 on Dock still can't be identified.
> > >> First detected r8152 will use the MAC passthrough address.
> > > I do have to wonder why you are doing this in the kernel, and not
> > > using a udev rule? This seems to be policy, and policy does not belong
> > > in the kernel.
> > Debatable. An ethernet NIC has to have a MAC. The kernel must
> > provide one. That we should always take the one found in the
> > device's ROM rather than the host's ROM is already a policy decision.
>
> In general, it is a much longer list of places to find the MAC address
> from. It could be in three different places in device tree, it could
> be in ACPI in a similar number of places, it could be in NVMEM,
> pointed to by device tree, the bootloader might of already programmed
> the controller with its MAC address, etc, or if everything else fails,
> it could be random.
>
> So yes, the kernel will give it one. But if you want the interface to
> have a specific MAC address, you probably should not be trusting the
> kernel, given it has so many options.

Can udev in current form really handle the MAC race? Unless there's a
new uevent right before ndo_open() so udev can decide which MAC to
assign? Even with that, the race can still happen...

So what if we keep the existing behavior (i.e. copy MAC from ACPI),
and let userspace daemon like NetworkManager to give the second NIC
(r8152 in this case) a random MAC if the main NIC (I219 in this case)
is already in use? Considering the userspace daemon has the all the
information required and it's the policy maker here.

Kai-Heng

>
>         Andrew
>
>

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Andrew Lunn]

> Can udev in current form really handle the MAC race? Unless there's a
> new uevent right before ndo_open() so udev can decide which MAC to
> assign? Even with that, the race can still happen...

There will always be a race, since the kernel can start using the
interface before register_netdev() has even finished, before user
space is running. Take a look at how NFS root works.

But in general, you can change the MAC address at any time. Some MAC
drivers will return -EBUSY if the interface is up, but that is
generally artificial. After a change of MAC address ARP will time out
after a while and the link peers will get the new MAC address.

> 
> So what if we keep the existing behavior (i.e. copy MAC from ACPI),
> and let userspace daemon like NetworkManager to give the second NIC
> (r8152 in this case) a random MAC if the main NIC (I219 in this case)
> is already in use? Considering the userspace daemon has the all the
> information required and it's the policy maker here.

You should be thinking of this in more general terms. You want to
design a system that will work for any vendors laptop and dock.

You need to describe the two interfaces using some sort of bus
address, be it PCIe, USB, or a platform device address as used by
device tree etc.

Let the kernel do whatever it wants with MAC addresses for these two
interfaces. The only requirement you have is that the laptop internal
interface gets the vendor allocated MAC address, and that the dock get
some sort of MAC address, even if it is random.

On device creation, udev can check if it now has both interfaces? If
the internal interface is up, it is probably in use. Otherwise, take
its MAC address and assign it to the dock interface, and give the
internal interface a random MAC address, just in case.

You probably need to delay NetworkManager, systemd-networkkd,
/etc/network/interfaces etc, so that they don't do anything until
after udevd has settled, indicating all devices have probably been
found.

I suspect you will never get a 100% robust design, but you can
probably get it working enough for the cleaning staff and the CEO, who
have very simple setups. Power users are going to find all the corner
cases and will want to disable the udev rule.

     Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Kai-Heng Feng]

On Thu, Jan 6, 2022 at 9:28 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Can udev in current form really handle the MAC race? Unless there's a
> > new uevent right before ndo_open() so udev can decide which MAC to
> > assign? Even with that, the race can still happen...
>
> There will always be a race, since the kernel can start using the
> interface before register_netdev() has even finished, before user
> space is running. Take a look at how NFS root works.

Didn't know that, thanks for the insight.

>
> But in general, you can change the MAC address at any time. Some MAC
> drivers will return -EBUSY if the interface is up, but that is
> generally artificial. After a change of MAC address ARP will time out
> after a while and the link peers will get the new MAC address.

I think this makes the whole situation even more complex.

>
> >
> > So what if we keep the existing behavior (i.e. copy MAC from ACPI),
> > and let userspace daemon like NetworkManager to give the second NIC
> > (r8152 in this case) a random MAC if the main NIC (I219 in this case)
> > is already in use? Considering the userspace daemon has the all the
> > information required and it's the policy maker here.
>
> You should be thinking of this in more general terms. You want to
> design a system that will work for any vendors laptop and dock.
>
> You need to describe the two interfaces using some sort of bus
> address, be it PCIe, USB, or a platform device address as used by
> device tree etc.
>
> Let the kernel do whatever it wants with MAC addresses for these two
> interfaces. The only requirement you have is that the laptop internal
> interface gets the vendor allocated MAC address, and that the dock get
> some sort of MAC address, even if it is random.

Those laptops and docks are designed to have duplicated MACs. I don't
understand why but that's why Dell/HP/Lenovo did.
What if the kernel just abstract the hardware/firmware as intended, no
matter how stupid it is, and let userspace to make the right policy?

>
> On device creation, udev can check if it now has both interfaces? If
> the internal interface is up, it is probably in use. Otherwise, take
> its MAC address and assign it to the dock interface, and give the
> internal interface a random MAC address, just in case.
>
> You probably need to delay NetworkManager, systemd-networkkd,
> /etc/network/interfaces etc, so that they don't do anything until
> after udevd has settled, indicating all devices have probably been
> found.

I don't think it's a good idea. On my laptop,
systemd-udev-settle.service can add extra 5~10 seconds boot time
delay.
Furthermore, the external NIC in question is in a USB/Thunderbolt
dock, it can present pre-boot, or it can be hotplugged at any time.

>
> I suspect you will never get a 100% robust design, but you can
> probably get it working enough for the cleaning staff and the CEO, who
> have very simple setups. Power users are going to find all the corner
> cases and will want to disable the udev rule.

But power users may also need to use corporate network to work as
Aaron mentioned.
Packets from unregistered MAC can be filtered under corporate network,
and that's why MAC pass-through is a useful feature that many business
laptops have.

>
>      Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Jakub Kicinski]

On Fri, 7 Jan 2022 10:01:33 +0800 Kai-Heng Feng wrote:
> > On device creation, udev can check if it now has both interfaces? If
> > the internal interface is up, it is probably in use. Otherwise, take
> > its MAC address and assign it to the dock interface, and give the
> > internal interface a random MAC address, just in case.
> >
> > You probably need to delay NetworkManager, systemd-networkkd,
> > /etc/network/interfaces etc, so that they don't do anything until
> > after udevd has settled, indicating all devices have probably been
> > found.  
> 
> I don't think it's a good idea. On my laptop,
> systemd-udev-settle.service can add extra 5~10 seconds boot time
> delay.
> Furthermore, the external NIC in question is in a USB/Thunderbolt
> dock, it can present pre-boot, or it can be hotplugged at any time.

IIUC our guess is that this feature used for NAC and IEEE 802.1X.
In that case someone is already provisioning certificates to all 
the machines, and must provide a config for all its interfaces.
It should be pretty simple to also put the right MAC address override
in the NetworkManager/systemd-networkd/whatever config, no?

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Kai-Heng Feng]

On Fri, Jan 7, 2022 at 10:31 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 7 Jan 2022 10:01:33 +0800 Kai-Heng Feng wrote:
> > > On device creation, udev can check if it now has both interfaces? If
> > > the internal interface is up, it is probably in use. Otherwise, take
> > > its MAC address and assign it to the dock interface, and give the
> > > internal interface a random MAC address, just in case.
> > >
> > > You probably need to delay NetworkManager, systemd-networkkd,
> > > /etc/network/interfaces etc, so that they don't do anything until
> > > after udevd has settled, indicating all devices have probably been
> > > found.
> >
> > I don't think it's a good idea. On my laptop,
> > systemd-udev-settle.service can add extra 5~10 seconds boot time
> > delay.
> > Furthermore, the external NIC in question is in a USB/Thunderbolt
> > dock, it can present pre-boot, or it can be hotplugged at any time.
>
> IIUC our guess is that this feature used for NAC and IEEE 802.1X.
> In that case someone is already provisioning certificates to all
> the machines, and must provide a config for all its interfaces.
> It should be pretty simple to also put the right MAC address override
> in the NetworkManager/systemd-networkd/whatever config, no?

If that's really the case, why do major OEMs came up with MAC
pass-through? Stupid may it be, I don't think it's a solution looking
for problem.

Kai-Heng

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Jakub Kicinski]

On Mon, 10 Jan 2022 11:32:16 +0800 Kai-Heng Feng wrote:
> > > I don't think it's a good idea. On my laptop,
> > > systemd-udev-settle.service can add extra 5~10 seconds boot time
> > > delay.
> > > Furthermore, the external NIC in question is in a USB/Thunderbolt
> > > dock, it can present pre-boot, or it can be hotplugged at any time.  
> >
> > IIUC our guess is that this feature used for NAC and IEEE 802.1X.
> > In that case someone is already provisioning certificates to all
> > the machines, and must provide a config for all its interfaces.
> > It should be pretty simple to also put the right MAC address override
> > in the NetworkManager/systemd-networkd/whatever config, no?  
> 
> If that's really the case, why do major OEMs came up with MAC
> pass-through? Stupid may it be, I don't think it's a solution looking
> for problem.

I don't know. Maybe due to a limitation in Windows? Maybe it's hard to
do in network manager, too, and we're not seeing something. Or perhaps
simply because they want to convince corporations to buy their
unreasonably expensive docks.

What I do know is that we need to gain a good understanding of the
motivation before we push any more of such magic into the kernel.

I may be able to do some testing myself after the Omicron surge is over
in the US.

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Kai-Heng Feng]

[+Cc Mario Limonciello, the original author on MAC pass-through]

On Tue, Jan 11, 2022 at 12:51 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 10 Jan 2022 11:32:16 +0800 Kai-Heng Feng wrote:
> > > > I don't think it's a good idea. On my laptop,
> > > > systemd-udev-settle.service can add extra 5~10 seconds boot time
> > > > delay.
> > > > Furthermore, the external NIC in question is in a USB/Thunderbolt
> > > > dock, it can present pre-boot, or it can be hotplugged at any time.
> > >
> > > IIUC our guess is that this feature used for NAC and IEEE 802.1X.
> > > In that case someone is already provisioning certificates to all
> > > the machines, and must provide a config for all its interfaces.
> > > It should be pretty simple to also put the right MAC address override
> > > in the NetworkManager/systemd-networkd/whatever config, no?
> >
> > If that's really the case, why do major OEMs came up with MAC
> > pass-through? Stupid may it be, I don't think it's a solution looking
> > for problem.
>
> I don't know. Maybe due to a limitation in Windows? Maybe it's hard to
> do in network manager, too, and we're not seeing something. Or perhaps
> simply because they want to convince corporations to buy their
> unreasonably expensive docks.
>
> What I do know is that we need to gain a good understanding of the
> motivation before we push any more of such magic into the kernel.

Mario, do you know how corporate network and other OS handle MAC
pass-through, so we can come up with a more robust design?

Kai-Heng

>
> I may be able to do some testing myself after the Omicron surge is over
> in the US.

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Limonciello, Mario]

On 1/10/2022 19:51, Kai-Heng Feng wrote:
> [+Cc Mario Limonciello, the original author on MAC pass-through]
> 
> On Tue, Jan 11, 2022 at 12:51 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Mon, 10 Jan 2022 11:32:16 +0800 Kai-Heng Feng wrote:
>>>>> I don't think it's a good idea. On my laptop,
>>>>> systemd-udev-settle.service can add extra 5~10 seconds boot time
>>>>> delay.
>>>>> Furthermore, the external NIC in question is in a USB/Thunderbolt
>>>>> dock, it can present pre-boot, or it can be hotplugged at any time.
>>>>
>>>> IIUC our guess is that this feature used for NAC and IEEE 802.1X.
>>>> In that case someone is already provisioning certificates to all
>>>> the machines, and must provide a config for all its interfaces.
>>>> It should be pretty simple to also put the right MAC address override
>>>> in the NetworkManager/systemd-networkd/whatever config, no?
>>>
>>> If that's really the case, why do major OEMs came up with MAC
>>> pass-through? Stupid may it be, I don't think it's a solution looking
>>> for problem.
>>
>> I don't know. Maybe due to a limitation in Windows? Maybe it's hard to
>> do in network manager, too, and we're not seeing something. Or perhaps
>> simply because they want to convince corporations to buy their
>> unreasonably expensive docks.
>>
>> What I do know is that we need to gain a good understanding of the
>> motivation before we push any more of such magic into the kernel.
> 
> Mario, do you know how corporate network and other OS handle MAC
> pass-through, so we can come up with a more robust design?

The important thing to remember is that many of these machines *don't*
have in-built network controller and rely upon a USB-c network adapter.

I recall a few reasons.

1) Consistency with the UEFI network stack and dual booting Windows when 
using the machine.  IOW 1 DHCP lease to one network controller, not one OS.

2) A (small) part of an onion that is network security.  It allows 
administrators to allow-list or block-list controllers.

The example I recall hearing is someone has their laptop stolen and 
notifies I/T.  I/T removes the MAC address of the pass through address
from the allow-list and now that laptop can't use any hotel cubes for 
accessing network resources.

3) Resource planning and management of hoteling resources.

For example allow facilities to monitor whether users are reserving and 
using the hoteling cubes they reserved.

> 
> Kai-Heng
> 
>>
>> I may be able to do some testing myself after the Omicron surge is over
>> in the US.

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Jakub Kicinski]

On Tue, 11 Jan 2022 08:57:39 -0600 Limonciello, Mario wrote:
> The important thing to remember is that many of these machines *don't*
> have in-built network controller and rely upon a USB-c network adapter.
> 
> I recall a few reasons.
> 
> 1) Consistency with the UEFI network stack and dual booting Windows when 
> using the machine.  IOW 1 DHCP lease to one network controller, not one OS.
> 
> 2) A (small) part of an onion that is network security.  It allows 
> administrators to allow-list or block-list controllers.
> 
> The example I recall hearing is someone has their laptop stolen and 
> notifies I/T.  I/T removes the MAC address of the pass through address
> from the allow-list and now that laptop can't use any hotel cubes for 
> accessing network resources.
> 
> 3) Resource planning and management of hoteling resources.
> 
> For example allow facilities to monitor whether users are reserving and 
> using the hoteling cubes they reserved.

Interesting, I haven't thought about use case (3).

Do you know how this is implemented on other platforms?

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Limonciello, Mario]

On 1/11/2022 10:26, Jakub Kicinski wrote:
> On Tue, 11 Jan 2022 08:57:39 -0600 Limonciello, Mario wrote:
>> The important thing to remember is that many of these machines *don't*
>> have in-built network controller and rely upon a USB-c network adapter.
>>
>> I recall a few reasons.
>>
>> 1) Consistency with the UEFI network stack and dual booting Windows when
>> using the machine.  IOW 1 DHCP lease to one network controller, not one OS.
>>
>> 2) A (small) part of an onion that is network security.  It allows
>> administrators to allow-list or block-list controllers.
>>
>> The example I recall hearing is someone has their laptop stolen and
>> notifies I/T.  I/T removes the MAC address of the pass through address
>> from the allow-list and now that laptop can't use any hotel cubes for
>> accessing network resources.
>>
>> 3) Resource planning and management of hoteling resources.
>>
>> For example allow facilities to monitor whether users are reserving and
>> using the hoteling cubes they reserved.
> 
> Interesting, I haven't thought about use case (3).

These are just the cases I have from my memory when we kicked this off. 
  There may be others that are now used too.

> 
> Do you know how this is implemented on other platforms?


It's entirely OS independent - but presumes that there is a mapping of 
the pass through MAC address of the HW to a user account in the hoteling 
cube reservation software.

If you end up having only your pass through MAC used for Windows and 
UEFI your hoteling system might not work properly if your corporation 
also supports employees to use Linux and this feature was removed from 
the kernel.

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Jakub Kicinski]

On Tue, 11 Jan 2022 10:33:39 -0600 Limonciello, Mario wrote:
> If you end up having only your pass through MAC used for Windows and 
> UEFI your hoteling system might not work properly if your corporation 
> also supports employees to use Linux and this feature was removed from 
> the kernel.

Right, I think the utility of the feature is clear now. Let me clarify
what I was after - I was wondering which component is responsible for
the address inheritance in Windows or UEFI. Is it also hardcoded into
the realtek driver or is there a way to export the ACPI information to
the network management component?

Also knowing how those OSes handle the new docks which don't have
unique device IDs would obviously be great..

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Limonciello, Mario]

On 1/11/2022 10:43, Jakub Kicinski wrote:
> On Tue, 11 Jan 2022 10:33:39 -0600 Limonciello, Mario wrote:
>> If you end up having only your pass through MAC used for Windows and
>> UEFI your hoteling system might not work properly if your corporation
>> also supports employees to use Linux and this feature was removed from
>> the kernel.
> 
> Right, I think the utility of the feature is clear now. Let me clarify
> what I was after - 

Thanks, I was looped into this thread late so I didn't have any context.

You prompted me to look at this patch series on patchwork.

We talked about this when the initial patch series was developed and the 
intention was to align what Windows does.  That means that all dongles 
or docks with the appropriate effuse blown take the same pass through 
address.

Anything else and you lose the element of predictability and all those 
use cases I mentioned stop working.

> I was wondering which component is responsible for
> the address inheritance in Windows or UEFI. 

The Realtek driver for Windows and the Realtek DXE for UEFI.

> Is it also hardcoded into
> the realtek driver or is there a way to export the ACPI information to
> the network management component?

On Windows there is no indication outside of the driver that this 
feature has been used.  It's "invisible" to the user.

> 
> Also knowing how those OSes handle the new docks which don't have
> unique device IDs would obviously be great..

I'm sorry, can you give me some more context on this?  What unique 
device IDs?

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Jakub Kicinski]

On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:
> > Also knowing how those OSes handle the new docks which don't have
> > unique device IDs would obviously be great..  
> 
> I'm sorry, can you give me some more context on this?  What unique 
> device IDs?

We used to match the NICs based on their device ID. The USB NICs
present in docks had lenovo as manufacturer and a unique device ID.
Now reportedly the new docks are using generic realtek IDs so we have
no way to differentiate "blessed" dock NICs from random USB dongles,
and inheriting the address to all devices with the generic relatek IDs
breaks setups with multiple dongles, e.g. Henning's setup.

If we know of a fuse that can be read on new docks that'd put us back
in more comfortable position. If we need to execute random heuristics
to find the "right NIC" we'd much rather have udev or NetworkManager 
or some other user space do that according to whatever policy it
chooses.

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Limonciello, Mario]

On 1/11/2022 11:06, Jakub Kicinski wrote:
> On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:
>>> Also knowing how those OSes handle the new docks which don't have
>>> unique device IDs would obviously be great..
>>
>> I'm sorry, can you give me some more context on this?  What unique
>> device IDs?
> 
> We used to match the NICs based on their device ID. The USB NICs
> present in docks had lenovo as manufacturer and a unique device ID.
> Now reportedly the new docks are using generic realtek IDs so we have
> no way to differentiate "blessed" dock NICs from random USB dongles,
> and inheriting the address to all devices with the generic relatek IDs
> breaks setups with multiple dongles, e.g. Henning's setup. >
> If we know of a fuse that can be read on new docks that'd put us back
> in more comfortable position. If we need to execute random heuristics
> to find the "right NIC" we'd much rather have udev or NetworkManager
> or some other user space do that according to whatever policy it
> chooses.

I agree - this stuff in the kernel isn't supposed to be applying to 
anything other than the OEM dongles or docks.  If you can't identify 
them they shouldn't be in here.

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Am Tue, 11 Jan 2022 11:10:52 -0600
schrieb "Limonciello, Mario" <mario.limonciello@amd.com>:

> On 1/11/2022 11:06, Jakub Kicinski wrote:
> > On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:  
> >>> Also knowing how those OSes handle the new docks which don't have
> >>> unique device IDs would obviously be great..  
> >>
> >> I'm sorry, can you give me some more context on this?  What unique
> >> device IDs?  
> > 
> > We used to match the NICs based on their device ID. The USB NICs
> > present in docks had lenovo as manufacturer and a unique device ID.
> > Now reportedly the new docks are using generic realtek IDs so we
> > have no way to differentiate "blessed" dock NICs from random USB
> > dongles, and inheriting the address to all devices with the generic
> > relatek IDs breaks setups with multiple dongles, e.g. Henning's
> > setup. > If we know of a fuse that can be read on new docks that'd
> > put us back in more comfortable position. If we need to execute
> > random heuristics to find the "right NIC" we'd much rather have
> > udev or NetworkManager or some other user space do that according
> > to whatever policy it chooses.  
> 
> I agree - this stuff in the kernel isn't supposed to be applying to 
> anything other than the OEM dongles or docks.  If you can't identify 
> them they shouldn't be in here.

Not sure we can really get to a proper solution here. The one revert
for Lenovo will solve my very issue. And on top i was lucky enough to
being able to disable pass-thru in the BIOS.

From what the Lenovo vendor docs seem to suggest it is about PXE ...
meaning the BIOS will do the spoofing, how it does that is unclear. Now
an OS can try to keep it up but probably should not try to do anything
on its own ... or do the additional bits in user-space and not the
kernel.

Thinking about PXE we do not just have the kernel, but also multiple
potential bootloaders. All would need to inherit the spoofed MAC from a
potential predecessor having applied spoofing, and support USB and
network ... that is not realistic!

Going back to PXE ... says nothing about OS operation really. Say a
BIOS decides to spoof when booting ... that say nothing on how to deal
with hot-plugged devices which die BIOS did not even see. But we have
code for such hot-plug spoofing in the kernel .. where vendors like
Lenovo talk about PXE (only?)

The whole story seems too complicated and not really explained or
throught through. If the vendors can explain stuff the kernel can
probably cater ... but user-land would still be the better place.

I will not push for more reverts. But more patches in the direction
should be questioned really hard! And even if we get "proper device
matching" we will only cater for "vendor lock-in". It is not clear why
that strange feature will only apply if the dock if from the same
vendor as the laptop. Applying it on all USB NICs is clearly going too
far, that will only work with IT department highlander policies like
"there must only be one NIC".

So from my point it is solved with the one Lenovo-related revert. Any
future pass-thru patches get a NACK and any reverts targeting other
vendors get an ACK. But feel free to Cc me when such things happen in
the future.

regards,
Henning

RE: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Limonciello, Mario]

[Public]

> -----Original Message-----
> From: Henning Schild <henning.schild@siemens.com>
> Sent: Wednesday, January 12, 2022 13:21
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; Kai-Heng Feng
> <kai.heng.feng@canonical.com>; Andrew Lunn <andrew@lunn.ch>; Oliver
> Neukum <oneukum@suse.com>; Aaron Ma <aaron.ma@canonical.com>; linux-
> usb@vger.kernel.org; netdev@vger.kernel.org; davem@davemloft.net;
> hayeswang@realtek.com; tiwai@suse.de
> Subject: Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough
> address
> 
> Am Tue, 11 Jan 2022 11:10:52 -0600
> schrieb "Limonciello, Mario" <mario.limonciello@amd.com>:
> 
> > On 1/11/2022 11:06, Jakub Kicinski wrote:
> > > On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:
> > >>> Also knowing how those OSes handle the new docks which don't have
> > >>> unique device IDs would obviously be great..
> > >>
> > >> I'm sorry, can you give me some more context on this?  What unique
> > >> device IDs?
> > >
> > > We used to match the NICs based on their device ID. The USB NICs
> > > present in docks had lenovo as manufacturer and a unique device ID.
> > > Now reportedly the new docks are using generic realtek IDs so we
> > > have no way to differentiate "blessed" dock NICs from random USB
> > > dongles, and inheriting the address to all devices with the generic
> > > relatek IDs breaks setups with multiple dongles, e.g. Henning's
> > > setup. > If we know of a fuse that can be read on new docks that'd
> > > put us back in more comfortable position. If we need to execute
> > > random heuristics to find the "right NIC" we'd much rather have
> > > udev or NetworkManager or some other user space do that according
> > > to whatever policy it chooses.
> >
> > I agree - this stuff in the kernel isn't supposed to be applying to
> > anything other than the OEM dongles or docks.  If you can't identify
> > them they shouldn't be in here.
> 
> Not sure we can really get to a proper solution here. The one revert
> for Lenovo will solve my very issue. And on top i was lucky enough to
> being able to disable pass-thru in the BIOS.
> 
> From what the Lenovo vendor docs seem to suggest it is about PXE ...
> meaning the BIOS will do the spoofing, how it does that is unclear. Now
> an OS can try to keep it up but probably should not try to do anything
> on its own ... or do the additional bits in user-space and not the
> kernel.
> 
> Thinking about PXE we do not just have the kernel, but also multiple
> potential bootloaders. All would need to inherit the spoofed MAC from a
> potential predecessor having applied spoofing, and support USB and
> network ... that is not realistic!
> 
> Going back to PXE ... says nothing about OS operation really. Say a
> BIOS decides to spoof when booting ... that say nothing on how to deal
> with hot-plugged devices which die BIOS did not even see. But we have
> code for such hot-plug spoofing in the kernel .. where vendors like
> Lenovo talk about PXE (only?)

Something that would probably be interesting to check is whether the
BIOS uses pass through MAC on anything as well or it has some criteria
that decides to apply it that the kernel doesn't know about.

> 
> The whole story seems too complicated and not really explained or
> throught through. If the vendors can explain stuff the kernel can
> probably cater ... but user-land would still be the better place.
> 
> I will not push for more reverts. But more patches in the direction
> should be questioned really hard! And even if we get "proper device
> matching" we will only cater for "vendor lock-in". It is not clear why
> that strange feature will only apply if the dock if from the same
> vendor as the laptop. Applying it on all USB NICs is clearly going too
> far, that will only work with IT department highlander policies like
> "there must only be one NIC".
> 
> So from my point it is solved with the one Lenovo-related revert. Any
> future pass-thru patches get a NACK and any reverts targeting other
> vendors get an ACK. But feel free to Cc me when such things happen in
> the future.
> 

KH & Aaron - can you please talk to Lenovo about making sure that there
is a way to distinguish between devices that should get pass through or
shouldn't and to document that?  

I think that a policy that it should be a NACK for anything else general
makes sense.

> regards,
> Henning

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

On 1/13/22 03:27, Limonciello, Mario wrote:
> [Public]
>
>> -----Original Message-----
>> From: Henning Schild <henning.schild@siemens.com>
>> Sent: Wednesday, January 12, 2022 13:21
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>; Kai-Heng Feng
>> <kai.heng.feng@canonical.com>; Andrew Lunn <andrew@lunn.ch>; Oliver
>> Neukum <oneukum@suse.com>; Aaron Ma <aaron.ma@canonical.com>; linux-
>> usb@vger.kernel.org; netdev@vger.kernel.org; davem@davemloft.net;
>> hayeswang@realtek.com; tiwai@suse.de
>> Subject: Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough
>> address
>>
>> Am Tue, 11 Jan 2022 11:10:52 -0600
>> schrieb "Limonciello, Mario" <mario.limonciello@amd.com>:
>>
>>> On 1/11/2022 11:06, Jakub Kicinski wrote:
>>>> On Tue, 11 Jan 2022 10:54:50 -0600 Limonciello, Mario wrote:
>>>>>> Also knowing how those OSes handle the new docks which don't have
>>>>>> unique device IDs would obviously be great..
>>>>> I'm sorry, can you give me some more context on this?  What unique
>>>>> device IDs?
>>>> We used to match the NICs based on their device ID. The USB NICs
>>>> present in docks had lenovo as manufacturer and a unique device ID.
>>>> Now reportedly the new docks are using generic realtek IDs so we
>>>> have no way to differentiate "blessed" dock NICs from random USB
>>>> dongles, and inheriting the address to all devices with the generic
>>>> relatek IDs breaks setups with multiple dongles, e.g. Henning's
>>>> setup. > If we know of a fuse that can be read on new docks that'd
>>>> put us back in more comfortable position. If we need to execute
>>>> random heuristics to find the "right NIC" we'd much rather have
>>>> udev or NetworkManager or some other user space do that according
>>>> to whatever policy it chooses.
>>> I agree - this stuff in the kernel isn't supposed to be applying to
>>> anything other than the OEM dongles or docks.  If you can't identify
>>> them they shouldn't be in here.
>> Not sure we can really get to a proper solution here. The one revert
>> for Lenovo will solve my very issue. And on top i was lucky enough to
>> being able to disable pass-thru in the BIOS.
>>
>>  From what the Lenovo vendor docs seem to suggest it is about PXE ...
>> meaning the BIOS will do the spoofing, how it does that is unclear. Now
>> an OS can try to keep it up but probably should not try to do anything
>> on its own ... or do the additional bits in user-space and not the
>> kernel.
>>
>> Thinking about PXE we do not just have the kernel, but also multiple
>> potential bootloaders. All would need to inherit the spoofed MAC from a
>> potential predecessor having applied spoofing, and support USB and
>> network ... that is not realistic!
>>
>> Going back to PXE ... says nothing about OS operation really. Say a
>> BIOS decides to spoof when booting ... that say nothing on how to deal
>> with hot-plugged devices which die BIOS did not even see. But we have
>> code for such hot-plug spoofing in the kernel .. where vendors like
>> Lenovo talk about PXE (only?)
> Something that would probably be interesting to check is whether the
> BIOS uses pass through MAC on anything as well or it has some criteria
> that decides to apply it that the kernel doesn't know about.
>
>> The whole story seems too complicated and not really explained or
>> throught through. If the vendors can explain stuff the kernel can
>> probably cater ... but user-land would still be the better place.
>>
>> I will not push for more reverts. But more patches in the direction
>> should be questioned really hard! And even if we get "proper device
>> matching" we will only cater for "vendor lock-in". It is not clear why
>> that strange feature will only apply if the dock if from the same
>> vendor as the laptop. Applying it on all USB NICs is clearly going too
>> far, that will only work with IT department highlander policies like
>> "there must only be one NIC".
>>
>> So from my point it is solved with the one Lenovo-related revert. Any
>> future pass-thru patches get a NACK and any reverts targeting other
>> vendors get an ACK. But feel free to Cc me when such things happen in
>> the future.
>>
> KH & Aaron - can you please talk to Lenovo about making sure that there
> is a way to distinguish between devices that should get pass through or
> shouldn't and to document that?
>
> I think that a policy that it should be a NACK for anything else general
> makes sense.

Sorry for my previous patch.
Before made that patch I already discussed with Lenovo.
And didn't get any other opinion. The solution is from a discussion with them.

This info had been forward to them too.

Aaron

>
>> regards,
>> Henning

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

Hi all,

Realtek 8153BL can be identified by the following code from Realtek Outbox driver:
} else if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {

I will suggest Realtek to send out this change for review.

Thanks,
Aaron

On 1/13/22 11:23, Aaron Ma wrote:
> Before made that patch I already discussed with Lenovo.
> And didn't get any other opinion. The solution is from a discussion with them.
> 
> This info had been forward to them too.

RE: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Hayes Wang]

Aaron Ma <aaron.ma@canonical.com>
> Sent: Thursday, January 27, 2022 10:52 AM
[...]
> Hi all,
> 
> Realtek 8153BL can be identified by the following code from Realtek Outbox
> driver:
> } else if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {
> 
> I will suggest Realtek to send out this change for review.

I don't think the feature of MAC passthrough address is maintained
by Realtek. Especially, there is no uniform way about it. The
different companies have to maintain their own ways by themselves.

Realtek could provide the method of finding out the specific device
for Lenovo. You could check USB OCP 0xD81F bit 3. For example,

	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
		/* This is the RTL8153B for Lenovo. */
	}

Best Regards,
Hayes

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

On 1/27/22 16:06, Hayes Wang wrote:
> I don't think the feature of MAC passthrough address is maintained
> by Realtek. Especially, there is no uniform way about it. The
> different companies have to maintain their own ways by themselves.
> 
> Realtek could provide the method of finding out the specific device
> for Lenovo. You could check USB OCP 0xD81F bit 3. For example,
> 
> 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> 	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
> 		/* This is the RTL8153B for Lenovo. */
> 	}
> 

May I use the code from Realtek Outbox driver to implement the MAPT?

If so, allow me to write a patch and send here to review.

Thanks,
Aaron


> Best Regards,
> Hayes

RE: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Hayes Wang]

Aaron Ma <aaron.ma@canonical.com>
> Sent: Thursday, January 27, 2022 4:13 PM
[...]
> > I don't think the feature of MAC passthrough address is maintained
> > by Realtek. Especially, there is no uniform way about it. The
> > different companies have to maintain their own ways by themselves.
> >
> > Realtek could provide the method of finding out the specific device
> > for Lenovo. You could check USB OCP 0xD81F bit 3. For example,
> >
> > 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> > 	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
> > 		/* This is the RTL8153B for Lenovo. */
> > 	}
> >
> 
> May I use the code from Realtek Outbox driver to implement the MAPT?
> 
> If so, allow me to write a patch and send here to review.

Sure.

However, the outbox driver has a mistake.
The mac_obj_name with "\\_SB.AMAC" is used by Dell.
I think the device of Lenovo should use "\\MACA" only. Right?

The easiest way is to set tp->lenovo_macpassthru for RTL8153BL.
For example,

	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3)))
		tp->lenovo_macpassthru = 1;

Best Regards,
Hayes

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Andrew Lunn]

On Thu, Jan 27, 2022 at 08:06:20AM +0000, Hayes Wang wrote:
> Aaron Ma <aaron.ma@canonical.com>
> > Sent: Thursday, January 27, 2022 10:52 AM
> [...]
> > Hi all,
> > 
> > Realtek 8153BL can be identified by the following code from Realtek Outbox
> > driver:
> > } else if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {
> > 
> > I will suggest Realtek to send out this change for review.
> 
> I don't think the feature of MAC passthrough address is maintained
> by Realtek. Especially, there is no uniform way about it. The
> different companies have to maintain their own ways by themselves.
> 
> Realtek could provide the method of finding out the specific device
> for Lenovo. You could check USB OCP 0xD81F bit 3. For example,
> 
> 	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> 	if (tp->version == RTL_VER_09 && (ocp_data & BIT(3))) {
> 		/* This is the RTL8153B for Lenovo. */
> 	}

Is there a documented meaning for this bit? Realtek have allocated
this bit to Lenovo?

Why is it guaranteed that no other devices manufactured by anybody
else will not have this bit set? That is the real question here. Does
this give false positive where you break the networking for some
random USB dongle.

     Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Andrew Lunn]

> > You should be thinking of this in more general terms. You want to
> > design a system that will work for any vendors laptop and dock.
> >
> > You need to describe the two interfaces using some sort of bus
> > address, be it PCIe, USB, or a platform device address as used by
> > device tree etc.
> >
> > Let the kernel do whatever it wants with MAC addresses for these two
> > interfaces. The only requirement you have is that the laptop internal
> > interface gets the vendor allocated MAC address, and that the dock get
> > some sort of MAC address, even if it is random.
> 
> Those laptops and docks are designed to have duplicated MACs. I don't
> understand why but that's why Dell/HP/Lenovo did.

But it also sounds like the design is broken. So the question is, is
it possible to actually implement it correctly, without breaking
networking for others with sane laptop/docks/USB dongles.

> What if the kernel just abstract the hardware/firmware as intended, no
> matter how stupid it is, and let userspace to make the right policy?

Which is exactly what is being suggested here. The kernel gives the
laptop internal interface its MAC address from ACPI or where ever, and
the dock which has no MAC address gets a random MAC address. That is
the normal kernel abstract. Userspace, in the form of udev, can then
change the MAC addresses in whatever way it wants.

> But power users may also need to use corporate network to work as
> Aaron mentioned.
> Packets from unregistered MAC can be filtered under corporate network,
> and that's why MAC pass-through is a useful feature that many business
> laptops have.

Depends on the cooperate network, but power users generally know more
than the IT department, and will just make their machine work, copying
the 802.3x certificate where ever it needs to go, us ebtables to
mangle the MAC address, build their own little network with an RPi
acting as a gateway doing NAT and MAC address translation, etc.

       Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Kai-Heng Feng]

On Fri, Jan 7, 2022 at 9:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > You should be thinking of this in more general terms. You want to
> > > design a system that will work for any vendors laptop and dock.
> > >
> > > You need to describe the two interfaces using some sort of bus
> > > address, be it PCIe, USB, or a platform device address as used by
> > > device tree etc.
> > >
> > > Let the kernel do whatever it wants with MAC addresses for these two
> > > interfaces. The only requirement you have is that the laptop internal
> > > interface gets the vendor allocated MAC address, and that the dock get
> > > some sort of MAC address, even if it is random.
> >
> > Those laptops and docks are designed to have duplicated MACs. I don't
> > understand why but that's why Dell/HP/Lenovo did.
>
> But it also sounds like the design is broken. So the question is, is
> it possible to actually implement it correctly, without breaking
> networking for others with sane laptop/docks/USB dongles.

It's possible, just stick to whitelist and never over generalize the
device matching rule.

>
> > What if the kernel just abstract the hardware/firmware as intended, no
> > matter how stupid it is, and let userspace to make the right policy?
>
> Which is exactly what is being suggested here. The kernel gives the
> laptop internal interface its MAC address from ACPI or where ever, and
> the dock which has no MAC address gets a random MAC address. That is
> the normal kernel abstract. Userspace, in the form of udev, can then
> change the MAC addresses in whatever way it wants.

That's not what I mean. I mean the kernel should do what
firmware/hardware expects kernel should do - copy the MAC from ACPI to
the external NIC in the dock.
Then the userspace can assign a random MAC to external interface if
internal interface is already up.

>
> > But power users may also need to use corporate network to work as
> > Aaron mentioned.
> > Packets from unregistered MAC can be filtered under corporate network,
> > and that's why MAC pass-through is a useful feature that many business
> > laptops have.
>
> Depends on the cooperate network, but power users generally know more
> than the IT department, and will just make their machine work, copying
> the 802.3x certificate where ever it needs to go, us ebtables to
> mangle the MAC address, build their own little network with an RPi
> acting as a gateway doing NAT and MAC address translation, etc.

That's true, but as someone who work closely with other Distro folks,
we really should make this feature works for (hopefully) everyone.

Kai-Heng

>
>        Andrew

Re: [PATCH 1/3 v3] net: usb: r8152: Check used MAC passthrough address

[Oliver Neukum]

On 05.01.22 16:14, Aaron Ma wrote:
> When plugin multiple r8152 ethernet dongles to Lenovo Docks
> or USB hub, MAC passthrough address from BIOS should be
> checked if it had been used to avoid using on other dongles.
>
> Currently builtin r8152 on Dock still can't be identified.
> First detected r8152 will use the MAC passthrough address.
>
> v2:
> Skip builtin PCI MAC address which is share MAC address with
> passthrough MAC.
> Check thunderbolt based ethernet.
I am sorry and I may be dense, why are you comparing
MACs? The pass-through is done per machine. All you
nee is a global flag, or if you want to be even more
formal, a reference count.

    Regards
        Oliver

[PATCH 1/3] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

When plugin multiple r8152 ethernet dongles to Lenovo Docks
or USB hub, MAC passthrough address from BIOS should be
checked if it had been used to avoid using on other dongles.

Skip builtin PCI MAC address which is share MAC address with
passthrough MAC.
Check thunderbolt based ethernet.

Currently builtin r8152 on Dock still can't be identified.
First detected r8152 will use the MAC passthrough address.

Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/net/usb/r8152.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f9877a3e83ac..91f4b2761f8e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -25,6 +25,7 @@
 #include <linux/atomic.h>
 #include <linux/acpi.h>
 #include <linux/firmware.h>
+#include <linux/pci.h>
 #include <crypto/hash.h>
 #include <linux/usb/r8152.h>
 
@@ -1605,6 +1606,7 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	char *mac_obj_name;
 	acpi_object_type mac_obj_type;
 	int mac_strlen;
+	struct net_device *ndev;
 
 	if (tp->lenovo_macpassthru) {
 		mac_obj_name = "\\MACA";
@@ -1662,6 +1664,18 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 		ret = -EINVAL;
 		goto amacout;
 	}
+	rcu_read_lock();
+	for_each_netdev_rcu(&init_net, ndev) {
+		if (ndev->dev.parent && dev_is_pci(ndev->dev.parent) &&
+				!pci_is_thunderbolt_attached(to_pci_dev(ndev->dev.parent)))
+			continue;
+		if (strncmp(buf, ndev->dev_addr, 6) == 0) {
+			rcu_read_unlock();
+			goto amacout;
+		}
+	}
+	rcu_read_unlock();
+
 	memcpy(sa->sa_data, buf, 6);
 	netif_info(tp, probe, tp->netdev,
 		   "Using pass-thru MAC addr %pM\n", sa->sa_data);
-- 
2.30.2

[PATCH 3/3] net: usb: r8152: remove unused definition

[Aaron Ma]

Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/net/usb/r8152.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3fbce3dbc04d..be2a6a2c2445 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -773,9 +773,6 @@ enum rtl8152_flags {
 	RX_EPROTO,
 };
 
-#define DEVICE_ID_THINKPAD_THUNDERBOLT3_DOCK_GEN2	0x3082
-#define DEVICE_ID_THINKPAD_USB_C_DOCK_GEN2		0xa387
-
 struct tally_counter {
 	__le64	tx_packets;
 	__le64	rx_packets;
-- 
2.30.2

Re: [PATCH 3/3] net: usb: r8152: remove unused definition

[Henning Schild]

Am Wed,  5 Jan 2022 22:23:51 +0800
schrieb Aaron Ma <aaron.ma@canonical.com>:

Maybe add a 
Fixes: f77b83b5bbab ("net: usb: r8152: Add MAC passthrough support for more Lenovo Docks")

> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 3fbce3dbc04d..be2a6a2c2445 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -773,9 +773,6 @@ enum rtl8152_flags {
>  	RX_EPROTO,
>  };
>  
> -#define DEVICE_ID_THINKPAD_THUNDERBOLT3_DOCK_GEN2	0x3082
> -#define DEVICE_ID_THINKPAD_USB_C_DOCK_GEN2		0xa387
> -
>  struct tally_counter {
>  	__le64	tx_packets;
>  	__le64	rx_packets;

Re: [PATCH 3/3] net: usb: r8152: remove unused definition

[Greg KH]

On Wed, Jan 05, 2022 at 03:51:06PM +0100, Henning Schild wrote:
> Am Wed,  5 Jan 2022 22:23:51 +0800
> schrieb Aaron Ma <aaron.ma@canonical.com>:
> 
> Maybe add a 
> Fixes: f77b83b5bbab ("net: usb: r8152: Add MAC passthrough support for more Lenovo Docks")

How can removing an unused #define fix anything?

Re: [PATCH 3/3] net: usb: r8152: remove unused definition

[Greg KH]

On Wed, Jan 05, 2022 at 10:23:51PM +0800, Aaron Ma wrote:
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 3 ---
>  1 file changed, 3 deletions(-)

I know I do not take patches without any changelog text in it, maybe
other maintainers are more lax :)

Please fix.

thanks,

greg k-h