[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

[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 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 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

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

[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

[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 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

[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 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 91f4b2761f8e..3fbce3dbc04d 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"
 
@@ -9545,6 +9547,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