2022-01-05 15:14:51

by Aaron Ma

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

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 <[email protected]>
---
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



2022-01-05 15:15:03

by Aaron Ma

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

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

Signed-off-by: Aaron Ma <[email protected]>
---
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


2022-01-05 15:15:20

by Aaron Ma

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

Signed-off-by: Aaron Ma <[email protected]>
---
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


2022-01-05 15:31:58

by Greg KH

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

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 <[email protected]>
> ---
> 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

2022-01-05 15:34:03

by Greg KH

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

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 <[email protected]>
> ---
> 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

2022-01-05 15:34:25

by Greg KH

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

On Wed, Jan 05, 2022 at 11:14:27PM +0800, Aaron Ma wrote:
> Signed-off-by: Aaron Ma <[email protected]>

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

thanks,

greg k-h

2022-01-05 15:57:52

by Henning Schild

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

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 <[email protected]>:

> 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 <[email protected]>
> ---
> 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);


2022-01-05 17:30:46

by Andrew Lunn

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

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

2022-01-05 17:40:28

by Henning Schild

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

Am Wed, 5 Jan 2022 18:30:08 +0100
schrieb Andrew Lunn <[email protected]>:

> 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


2022-01-05 21:51:31

by Oliver Neukum

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


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


2022-01-05 22:27:57

by Andrew Lunn

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

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

2022-01-06 02:11:07

by Kai-Heng Feng

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

On Thu, Jan 6, 2022 at 6:27 AM Andrew Lunn <[email protected]> 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
>
>

2022-01-06 13:28:05

by Andrew Lunn

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

> 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

2022-01-07 02:01:54

by Kai-Heng Feng

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

On Thu, Jan 6, 2022 at 9:28 PM Andrew Lunn <[email protected]> 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

2022-01-07 02:31:50

by Jakub Kicinski

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

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?

2022-01-07 13:32:37

by Andrew Lunn

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

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

2022-01-10 03:32:33

by Kai-Heng Feng

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

On Fri, Jan 7, 2022 at 10:31 AM Jakub Kicinski <[email protected]> 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

2022-01-10 03:41:44

by Kai-Heng Feng

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

On Fri, Jan 7, 2022 at 9:32 PM Andrew Lunn <[email protected]> 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

2022-01-10 11:40:37

by Oliver Neukum

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


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