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

[PATCH] 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.

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

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f9877a3e83ac..77f11b3f847b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1605,6 +1605,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 +1663,15 @@ 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 (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

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Hi Aaron,

if this or something similar goes in, please add another patch to
remove the left-over defines.

Am Wed,  5 Jan 2022 14:17:47 +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.
> 
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index f9877a3e83ac..77f11b3f847b 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
> +			rcu_read_unlock();
> +			goto amacout;

Since the original PCI netdev will always be there, that would disable
inheritance would it not?
I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME)) is
needed as well.

Maybe leave here with
netif_info()

And move the whole block up, we can skip the whole ACPI story if we
find the MAC busy.

> +		}
> +	}
> +	rcu_read_unlock();

Not sure if this function is guaranteed to only run once at a time,
otherwise i think that is a race. Multiple instances could make it to
this very point at the same time.

Henning

>  	memcpy(sa->sa_data, buf, 6);
>  	netif_info(tp, probe, tp->netdev,
>  		   "Using pass-thru MAC addr %pM\n", sa->sa_data);

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Am Wed, 5 Jan 2022 08:23:55 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Hi Aaron,
> 
> if this or something similar goes in, please add another patch to
> remove the left-over defines.
> 
> Am Wed,  5 Jan 2022 14:17:47 +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.
> > 
> > Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> > ---
> >  drivers/net/usb/r8152.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index f9877a3e83ac..77f11b3f847b 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
> > +			rcu_read_unlock();
> > +			goto amacout;  
> 
> Since the original PCI netdev will always be there, that would disable
> inheritance would it not?
> I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME)) is
> needed as well.
> 
> Maybe leave here with
> netif_info()
> 
> And move the whole block up, we can skip the whole ACPI story if we
> find the MAC busy.

That is wrong, need to know that MAC so can not move up too much. But
maybe above the is_valid_ether_addr

Henning

> > +		}
> > +	}
> > +	rcu_read_unlock();  
> 
> Not sure if this function is guaranteed to only run once at a time,
> otherwise i think that is a race. Multiple instances could make it to
> this very point at the same time.
> 
> Henning
> 
> >  	memcpy(sa->sa_data, buf, 6);
> >  	netif_info(tp, probe, tp->netdev,
> >  		   "Using pass-thru MAC addr %pM\n", sa->sa_data);
> >  
> 

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

On 1/5/22 15:32, Henning Schild wrote:
> Am Wed, 5 Jan 2022 08:23:55 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
>> Hi Aaron,
>>
>> if this or something similar goes in, please add another patch to
>> remove the left-over defines.
>>

Sure, I will do it.

>> Am Wed,  5 Jan 2022 14:17:47 +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.
>>>
>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>>> ---
>>>   drivers/net/usb/r8152.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>>> index f9877a3e83ac..77f11b3f847b 100644
>>> --- a/drivers/net/usb/r8152.c
>>> +++ b/drivers/net/usb/r8152.c
>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
>>> +			rcu_read_unlock();
>>> +			goto amacout;
>>
>> Since the original PCI netdev will always be there, that would disable
>> inheritance would it not?
>> I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME)) is
>> needed as well.
>>

PCI ethernet could be a builtin one on dock since there will be TBT4 dock.

>> Maybe leave here with
>> netif_info()
>>

Not good to print in rcu lock.

>> And move the whole block up, we can skip the whole ACPI story if we
>> find the MAC busy.
> 
> That is wrong, need to know that MAC so can not move up too much. But
> maybe above the is_valid_ether_addr

The MAC passthough address is read from ACPI.
ACPI read only happens once during r8152 driver probe.
To keep the lock less time, do it after is_valid_ether_addr.

> 
> Henning
> 
>>> +		}
>>> +	}
>>> +	rcu_read_unlock();
>>
>> Not sure if this function is guaranteed to only run once at a time,
>> otherwise i think that is a race. Multiple instances could make it to
>> this very point at the same time.
>>

Run once for one device.
So add a safe lock.

Aaron

>> Henning
>>
>>>   	memcpy(sa->sa_data, buf, 6);
>>>   	netif_info(tp, probe, tp->netdev,
>>>   		   "Using pass-thru MAC addr %pM\n", sa->sa_data);
>>>   
>>
> 

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

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

> On 1/5/22 15:32, Henning Schild wrote:
> > Am Wed, 5 Jan 2022 08:23:55 +0100
> > schrieb Henning Schild <henning.schild@siemens.com>:
> >   
> >> Hi Aaron,
> >>
> >> if this or something similar goes in, please add another patch to
> >> remove the left-over defines.
> >>  
> 
> Sure, I will do it.
> 
> >> Am Wed,  5 Jan 2022 14:17:47 +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.
> >>>
> >>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> >>> ---
> >>>   drivers/net/usb/r8152.c | 10 ++++++++++
> >>>   1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> >>> index f9877a3e83ac..77f11b3f847b 100644
> >>> --- a/drivers/net/usb/r8152.c
> >>> +++ b/drivers/net/usb/r8152.c
> >>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
> >>> +			rcu_read_unlock();
> >>> +			goto amacout;  
> >>
> >> Since the original PCI netdev will always be there, that would
> >> disable inheritance would it not?
> >> I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME))
> >> is needed as well.
> >>  
> 
> PCI ethernet could be a builtin one on dock since there will be TBT4
> dock.

In my X280 there is a PCI device in the laptop, always there. And its
MAC is the one found in ACPI. Did not try but i think for such devices
there would never be inheritance even if one wanted and used a Lenovo
dock that is supposed to do it.

Maybe i should try the patch but it seems like it defeats inheritance
completely. Well depending on probe order ...

regards,
Henning


> >> Maybe leave here with
> >> netif_info()
> >>  
> 
> Not good to print in rcu lock.
> 
> >> And move the whole block up, we can skip the whole ACPI story if we
> >> find the MAC busy.  
> > 
> > That is wrong, need to know that MAC so can not move up too much.
> > But maybe above the is_valid_ether_addr  
> 
> The MAC passthough address is read from ACPI.
> ACPI read only happens once during r8152 driver probe.
> To keep the lock less time, do it after is_valid_ether_addr.
> 
> > 
> > Henning
> >   
> >>> +		}
> >>> +	}
> >>> +	rcu_read_unlock();  
> >>
> >> Not sure if this function is guaranteed to only run once at a time,
> >> otherwise i think that is a race. Multiple instances could make it
> >> to this very point at the same time.
> >>  
> 
> Run once for one device.
> So add a safe lock.
> 
> Aaron
> 
> >> Henning
> >>  
> >>>   	memcpy(sa->sa_data, buf, 6);
> >>>   	netif_info(tp, probe, tp->netdev,
> >>>   		   "Using pass-thru MAC addr %pM\n",
> >>> sa->sa_data); 
> >>  
> >   

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

On 1/5/22 15:55, Henning Schild wrote:
> Am Wed, 5 Jan 2022 15:38:51 +0800
> schrieb Aaron Ma <aaron.ma@canonical.com>:
> 
>> On 1/5/22 15:32, Henning Schild wrote:
>>> Am Wed, 5 Jan 2022 08:23:55 +0100
>>> schrieb Henning Schild <henning.schild@siemens.com>:
>>>    
>>>> Hi Aaron,
>>>>
>>>> if this or something similar goes in, please add another patch to
>>>> remove the left-over defines.
>>>>   
>>
>> Sure, I will do it.
>>
>>>> Am Wed,  5 Jan 2022 14:17:47 +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.
>>>>>
>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>>>>> ---
>>>>>    drivers/net/usb/r8152.c | 10 ++++++++++
>>>>>    1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>>>>> index f9877a3e83ac..77f11b3f847b 100644
>>>>> --- a/drivers/net/usb/r8152.c
>>>>> +++ b/drivers/net/usb/r8152.c
>>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
>>>>> +			rcu_read_unlock();
>>>>> +			goto amacout;
>>>>
>>>> Since the original PCI netdev will always be there, that would
>>>> disable inheritance would it not?
>>>> I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME))
>>>> is needed as well.
>>>>   
>>
>> PCI ethernet could be a builtin one on dock since there will be TBT4
>> dock.
> 
> In my X280 there is a PCI device in the laptop, always there. And its
> MAC is the one found in ACPI. Did not try but i think for such devices
> there would never be inheritance even if one wanted and used a Lenovo
> dock that is supposed to do it.
> 

There will more TBT4 docks in market, the new ethernet is just the same as
PCI device, connected by thunderbolt.

For exmaple, connect a TBT4 dock which uses i225 pcie base ethernet,
then connect another TBT3 dock which uses r8152.
If skip PCI check, then i225 and r8152 will use the same MAC.

Aaron

> Maybe i should try the patch but it seems like it defeats inheritance
> completely. Well depending on probe order ...
> 
> regards,
> Henning
> 
> 
>>>> Maybe leave here with
>>>> netif_info()
>>>>   
>>
>> Not good to print in rcu lock.
>>
>>>> And move the whole block up, we can skip the whole ACPI story if we
>>>> find the MAC busy.
>>>
>>> That is wrong, need to know that MAC so can not move up too much.
>>> But maybe above the is_valid_ether_addr
>>
>> The MAC passthough address is read from ACPI.
>> ACPI read only happens once during r8152 driver probe.
>> To keep the lock less time, do it after is_valid_ether_addr.
>>
>>>
>>> Henning
>>>    
>>>>> +		}
>>>>> +	}
>>>>> +	rcu_read_unlock();
>>>>
>>>> Not sure if this function is guaranteed to only run once at a time,
>>>> otherwise i think that is a race. Multiple instances could make it
>>>> to this very point at the same time.
>>>>   
>>
>> Run once for one device.
>> So add a safe lock.
>>
>> Aaron
>>
>>>> Henning
>>>>   
>>>>>    	memcpy(sa->sa_data, buf, 6);
>>>>>    	netif_info(tp, probe, tp->netdev,
>>>>>    		   "Using pass-thru MAC addr %pM\n",
>>>>> sa->sa_data);
>>>>   
>>>    
> 

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Am Wed, 5 Jan 2022 16:01:24 +0800
schrieb Aaron Ma <aaron.ma@canonical.com>:

> On 1/5/22 15:55, Henning Schild wrote:
> > Am Wed, 5 Jan 2022 15:38:51 +0800
> > schrieb Aaron Ma <aaron.ma@canonical.com>:
> >   
> >> On 1/5/22 15:32, Henning Schild wrote:  
> >>> Am Wed, 5 Jan 2022 08:23:55 +0100
> >>> schrieb Henning Schild <henning.schild@siemens.com>:
> >>>      
> >>>> Hi Aaron,
> >>>>
> >>>> if this or something similar goes in, please add another patch to
> >>>> remove the left-over defines.
> >>>>     
> >>
> >> Sure, I will do it.
> >>  
> >>>> Am Wed,  5 Jan 2022 14:17:47 +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.
> >>>>>
> >>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> >>>>> ---
> >>>>>    drivers/net/usb/r8152.c | 10 ++++++++++
> >>>>>    1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> >>>>> index f9877a3e83ac..77f11b3f847b 100644
> >>>>> --- a/drivers/net/usb/r8152.c
> >>>>> +++ b/drivers/net/usb/r8152.c
> >>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
> >>>>> +			rcu_read_unlock();
> >>>>> +			goto amacout;  
> >>>>
> >>>> Since the original PCI netdev will always be there, that would
> >>>> disable inheritance would it not?
> >>>> I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME))
> >>>> is needed as well.
> >>>>     
> >>
> >> PCI ethernet could be a builtin one on dock since there will be
> >> TBT4 dock.  
> > 
> > In my X280 there is a PCI device in the laptop, always there. And
> > its MAC is the one found in ACPI. Did not try but i think for such
> > devices there would never be inheritance even if one wanted and
> > used a Lenovo dock that is supposed to do it.
> >   
> 
> There will more TBT4 docks in market, the new ethernet is just the
> same as PCI device, connected by thunderbolt.
> 
> For exmaple, connect a TBT4 dock which uses i225 pcie base ethernet,
> then connect another TBT3 dock which uses r8152.
> If skip PCI check, then i225 and r8152 will use the same MAC.

In current 5.15 i have that sort of collision already. All r8152s will
happily grab the MAC of the I219. In fact i have only ever seen it with
one r8152 at a time but while the I219 was actively in use.
While this patch will probably solve that, i bet it would defeat MAC
pass-thru altogether. Even when turned on in the BIOS.
Or does that iterator take "up"/"down" state into consideration? But
even if, the I219 could become "up" any time later.

These collisions are simply bound to happen and probably very hard to
avoid once you have set your mind on allowing pass-thru in the first
place. Not sure whether that even has potential to disturb network
equipment like switches.

Henning

> Aaron
> 
> > Maybe i should try the patch but it seems like it defeats
> > inheritance completely. Well depending on probe order ...
> > 
> > regards,
> > Henning
> > 
> >   
> >>>> Maybe leave here with
> >>>> netif_info()
> >>>>     
> >>
> >> Not good to print in rcu lock.
> >>  
> >>>> And move the whole block up, we can skip the whole ACPI story if
> >>>> we find the MAC busy.  
> >>>
> >>> That is wrong, need to know that MAC so can not move up too much.
> >>> But maybe above the is_valid_ether_addr  
> >>
> >> The MAC passthough address is read from ACPI.
> >> ACPI read only happens once during r8152 driver probe.
> >> To keep the lock less time, do it after is_valid_ether_addr.
> >>  
> >>>
> >>> Henning
> >>>      
> >>>>> +		}
> >>>>> +	}
> >>>>> +	rcu_read_unlock();  
> >>>>
> >>>> Not sure if this function is guaranteed to only run once at a
> >>>> time, otherwise i think that is a race. Multiple instances could
> >>>> make it to this very point at the same time.
> >>>>     
> >>
> >> Run once for one device.
> >> So add a safe lock.
> >>
> >> Aaron
> >>  
> >>>> Henning
> >>>>     
> >>>>>    	memcpy(sa->sa_data, buf, 6);
> >>>>>    	netif_info(tp, probe, tp->netdev,
> >>>>>    		   "Using pass-thru MAC addr %pM\n",
> >>>>> sa->sa_data);  
> >>>>     
> >>>      
> >   

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

On 1/5/22 16:32, Henning Schild wrote:
> Am Wed, 5 Jan 2022 16:01:24 +0800
> schrieb Aaron Ma <aaron.ma@canonical.com>:
> 
>> On 1/5/22 15:55, Henning Schild wrote:
>>> Am Wed, 5 Jan 2022 15:38:51 +0800
>>> schrieb Aaron Ma <aaron.ma@canonical.com>:
>>>    
>>>> On 1/5/22 15:32, Henning Schild wrote:
>>>>> Am Wed, 5 Jan 2022 08:23:55 +0100
>>>>> schrieb Henning Schild <henning.schild@siemens.com>:
>>>>>       
>>>>>> Hi Aaron,
>>>>>>
>>>>>> if this or something similar goes in, please add another patch to
>>>>>> remove the left-over defines.
>>>>>>      
>>>>
>>>> Sure, I will do it.
>>>>   
>>>>>> Am Wed,  5 Jan 2022 14:17:47 +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.
>>>>>>>
>>>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>>>>>>> ---
>>>>>>>     drivers/net/usb/r8152.c | 10 ++++++++++
>>>>>>>     1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>>>>>>> index f9877a3e83ac..77f11b3f847b 100644
>>>>>>> --- a/drivers/net/usb/r8152.c
>>>>>>> +++ b/drivers/net/usb/r8152.c
>>>>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
>>>>>>> +			rcu_read_unlock();
>>>>>>> +			goto amacout;
>>>>>>
>>>>>> Since the original PCI netdev will always be there, that would
>>>>>> disable inheritance would it not?
>>>>>> I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME))
>>>>>> is needed as well.
>>>>>>      
>>>>
>>>> PCI ethernet could be a builtin one on dock since there will be
>>>> TBT4 dock.
>>>
>>> In my X280 there is a PCI device in the laptop, always there. And
>>> its MAC is the one found in ACPI. Did not try but i think for such
>>> devices there would never be inheritance even if one wanted and
>>> used a Lenovo dock that is supposed to do it.
>>>    
>>
>> There will more TBT4 docks in market, the new ethernet is just the
>> same as PCI device, connected by thunderbolt.
>>
>> For exmaple, connect a TBT4 dock which uses i225 pcie base ethernet,
>> then connect another TBT3 dock which uses r8152.
>> If skip PCI check, then i225 and r8152 will use the same MAC.
> 
> In current 5.15 i have that sort of collision already. All r8152s will
> happily grab the MAC of the I219. In fact i have only ever seen it with
> one r8152 at a time but while the I219 was actively in use.
> While this patch will probably solve that, i bet it would defeat MAC
> pass-thru altogether. Even when turned on in the BIOS.
> Or does that iterator take "up"/"down" state into consideration? But
> even if, the I219 could become "up" any time later.
> 

No, that's different, I219 got MAC from their own space.
MAC passthrough got MAC from ACPI "\MACA".

> These collisions are simply bound to happen and probably very hard to
> avoid once you have set your mind on allowing pass-thru in the first
> place. Not sure whether that even has potential to disturb network
> equipment like switches.
> 

After check MAC address, it will be more safe.

Aaron

> Henning
> 
>> Aaron
>>
>>> Maybe i should try the patch but it seems like it defeats
>>> inheritance completely. Well depending on probe order ...
>>>
>>> regards,
>>> Henning
>>>
>>>    
>>>>>> Maybe leave here with
>>>>>> netif_info()
>>>>>>      
>>>>
>>>> Not good to print in rcu lock.
>>>>   
>>>>>> And move the whole block up, we can skip the whole ACPI story if
>>>>>> we find the MAC busy.
>>>>>
>>>>> That is wrong, need to know that MAC so can not move up too much.
>>>>> But maybe above the is_valid_ether_addr
>>>>
>>>> The MAC passthough address is read from ACPI.
>>>> ACPI read only happens once during r8152 driver probe.
>>>> To keep the lock less time, do it after is_valid_ether_addr.
>>>>   
>>>>>
>>>>> Henning
>>>>>       
>>>>>>> +		}
>>>>>>> +	}
>>>>>>> +	rcu_read_unlock();
>>>>>>
>>>>>> Not sure if this function is guaranteed to only run once at a
>>>>>> time, otherwise i think that is a race. Multiple instances could
>>>>>> make it to this very point at the same time.
>>>>>>      
>>>>
>>>> Run once for one device.
>>>> So add a safe lock.
>>>>
>>>> Aaron
>>>>   
>>>>>> Henning
>>>>>>      
>>>>>>>     	memcpy(sa->sa_data, buf, 6);
>>>>>>>     	netif_info(tp, probe, tp->netdev,
>>>>>>>     		   "Using pass-thru MAC addr %pM\n",
>>>>>>> sa->sa_data);
>>>>>>      
>>>>>       
>>>    
> 

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Am Wed, 5 Jan 2022 16:37:11 +0800
schrieb Aaron Ma <aaron.ma@canonical.com>:

> On 1/5/22 16:32, Henning Schild wrote:
> > Am Wed, 5 Jan 2022 16:01:24 +0800
> > schrieb Aaron Ma <aaron.ma@canonical.com>:
> >   
> >> On 1/5/22 15:55, Henning Schild wrote:  
> >>> Am Wed, 5 Jan 2022 15:38:51 +0800
> >>> schrieb Aaron Ma <aaron.ma@canonical.com>:
> >>>      
> >>>> On 1/5/22 15:32, Henning Schild wrote:  
> >>>>> Am Wed, 5 Jan 2022 08:23:55 +0100
> >>>>> schrieb Henning Schild <henning.schild@siemens.com>:
> >>>>>         
> >>>>>> Hi Aaron,
> >>>>>>
> >>>>>> if this or something similar goes in, please add another patch
> >>>>>> to remove the left-over defines.
> >>>>>>        
> >>>>
> >>>> Sure, I will do it.
> >>>>     
> >>>>>> Am Wed,  5 Jan 2022 14:17:47 +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.
> >>>>>>>
> >>>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> >>>>>>> ---
> >>>>>>>     drivers/net/usb/r8152.c | 10 ++++++++++
> >>>>>>>     1 file changed, 10 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> >>>>>>> index f9877a3e83ac..77f11b3f847b 100644
> >>>>>>> --- a/drivers/net/usb/r8152.c
> >>>>>>> +++ b/drivers/net/usb/r8152.c
> >>>>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
> >>>>>>> +			rcu_read_unlock();
> >>>>>>> +			goto amacout;  
> >>>>>>
> >>>>>> Since the original PCI netdev will always be there, that would
> >>>>>> disable inheritance would it not?
> >>>>>> I guess a strncmp(MODULE_NAME, info->driver,
> >>>>>> strlen(MODULE_NAME)) is needed as well.
> >>>>>>        
> >>>>
> >>>> PCI ethernet could be a builtin one on dock since there will be
> >>>> TBT4 dock.  
> >>>
> >>> In my X280 there is a PCI device in the laptop, always there. And
> >>> its MAC is the one found in ACPI. Did not try but i think for such
> >>> devices there would never be inheritance even if one wanted and
> >>> used a Lenovo dock that is supposed to do it.
> >>>      
> >>
> >> There will more TBT4 docks in market, the new ethernet is just the
> >> same as PCI device, connected by thunderbolt.
> >>
> >> For exmaple, connect a TBT4 dock which uses i225 pcie base
> >> ethernet, then connect another TBT3 dock which uses r8152.
> >> If skip PCI check, then i225 and r8152 will use the same MAC.  
> > 
> > In current 5.15 i have that sort of collision already. All r8152s
> > will happily grab the MAC of the I219. In fact i have only ever
> > seen it with one r8152 at a time but while the I219 was actively in
> > use. While this patch will probably solve that, i bet it would
> > defeat MAC pass-thru altogether. Even when turned on in the BIOS.
> > Or does that iterator take "up"/"down" state into consideration? But
> > even if, the I219 could become "up" any time later.
> >   
> 
> No, that's different, I219 got MAC from their own space.
> MAC passthrough got MAC from ACPI "\MACA".

On my machine "\MACA" and I219 are the same, likely "\MACA" was
populated by looking at that I219 by the BIOS.
Not sure if "\MACA" can change when plugging docks, but probably not
since the BIOS is also trying to implement inheritance of the main MAC.

Let me try this patch, maybe i do not get it.

Henning

> > These collisions are simply bound to happen and probably very hard
> > to avoid once you have set your mind on allowing pass-thru in the
> > first place. Not sure whether that even has potential to disturb
> > network equipment like switches.
> >   
> 
> After check MAC address, it will be more safe.
> 
> Aaron
> 
> > Henning
> >   
> >> Aaron
> >>  
> >>> Maybe i should try the patch but it seems like it defeats
> >>> inheritance completely. Well depending on probe order ...
> >>>
> >>> regards,
> >>> Henning
> >>>
> >>>      
> >>>>>> Maybe leave here with
> >>>>>> netif_info()
> >>>>>>        
> >>>>
> >>>> Not good to print in rcu lock.
> >>>>     
> >>>>>> And move the whole block up, we can skip the whole ACPI story
> >>>>>> if we find the MAC busy.  
> >>>>>
> >>>>> That is wrong, need to know that MAC so can not move up too
> >>>>> much. But maybe above the is_valid_ether_addr  
> >>>>
> >>>> The MAC passthough address is read from ACPI.
> >>>> ACPI read only happens once during r8152 driver probe.
> >>>> To keep the lock less time, do it after is_valid_ether_addr.
> >>>>     
> >>>>>
> >>>>> Henning
> >>>>>         
> >>>>>>> +		}
> >>>>>>> +	}
> >>>>>>> +	rcu_read_unlock();  
> >>>>>>
> >>>>>> Not sure if this function is guaranteed to only run once at a
> >>>>>> time, otherwise i think that is a race. Multiple instances
> >>>>>> could make it to this very point at the same time.
> >>>>>>        
> >>>>
> >>>> Run once for one device.
> >>>> So add a safe lock.
> >>>>
> >>>> Aaron
> >>>>     
> >>>>>> Henning
> >>>>>>        
> >>>>>>>     	memcpy(sa->sa_data, buf, 6);
> >>>>>>>     	netif_info(tp, probe, tp->netdev,
> >>>>>>>     		   "Using pass-thru MAC addr %pM\n",
> >>>>>>> sa->sa_data);  
> >>>>>>        
> >>>>>         
> >>>      
> >   

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Aaron Ma]

On 1/5/22 16:47, Henning Schild wrote:
> Am Wed, 5 Jan 2022 16:37:11 +0800
> schrieb Aaron Ma <aaron.ma@canonical.com>:
> 
>> On 1/5/22 16:32, Henning Schild wrote:
>>> Am Wed, 5 Jan 2022 16:01:24 +0800
>>> schrieb Aaron Ma <aaron.ma@canonical.com>:
>>>    
>>>> On 1/5/22 15:55, Henning Schild wrote:
>>>>> Am Wed, 5 Jan 2022 15:38:51 +0800
>>>>> schrieb Aaron Ma <aaron.ma@canonical.com>:
>>>>>       
>>>>>> On 1/5/22 15:32, Henning Schild wrote:
>>>>>>> Am Wed, 5 Jan 2022 08:23:55 +0100
>>>>>>> schrieb Henning Schild <henning.schild@siemens.com>:
>>>>>>>          
>>>>>>>> Hi Aaron,
>>>>>>>>
>>>>>>>> if this or something similar goes in, please add another patch
>>>>>>>> to remove the left-over defines.
>>>>>>>>         
>>>>>>
>>>>>> Sure, I will do it.
>>>>>>      
>>>>>>>> Am Wed,  5 Jan 2022 14:17:47 +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.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>>>>>>>>> ---
>>>>>>>>>      drivers/net/usb/r8152.c | 10 ++++++++++
>>>>>>>>>      1 file changed, 10 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>>>>>>>>> index f9877a3e83ac..77f11b3f847b 100644
>>>>>>>>> --- a/drivers/net/usb/r8152.c
>>>>>>>>> +++ b/drivers/net/usb/r8152.c
>>>>>>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
>>>>>>>>> +			rcu_read_unlock();
>>>>>>>>> +			goto amacout;
>>>>>>>>
>>>>>>>> Since the original PCI netdev will always be there, that would
>>>>>>>> disable inheritance would it not?
>>>>>>>> I guess a strncmp(MODULE_NAME, info->driver,
>>>>>>>> strlen(MODULE_NAME)) is needed as well.
>>>>>>>>         
>>>>>>
>>>>>> PCI ethernet could be a builtin one on dock since there will be
>>>>>> TBT4 dock.
>>>>>
>>>>> In my X280 there is a PCI device in the laptop, always there. And
>>>>> its MAC is the one found in ACPI. Did not try but i think for such
>>>>> devices there would never be inheritance even if one wanted and
>>>>> used a Lenovo dock that is supposed to do it.
>>>>>       
>>>>
>>>> There will more TBT4 docks in market, the new ethernet is just the
>>>> same as PCI device, connected by thunderbolt.
>>>>
>>>> For exmaple, connect a TBT4 dock which uses i225 pcie base
>>>> ethernet, then connect another TBT3 dock which uses r8152.
>>>> If skip PCI check, then i225 and r8152 will use the same MAC.
>>>
>>> In current 5.15 i have that sort of collision already. All r8152s
>>> will happily grab the MAC of the I219. In fact i have only ever
>>> seen it with one r8152 at a time but while the I219 was actively in
>>> use. While this patch will probably solve that, i bet it would
>>> defeat MAC pass-thru altogether. Even when turned on in the BIOS.
>>> Or does that iterator take "up"/"down" state into consideration? But
>>> even if, the I219 could become "up" any time later.
>>>    
>>
>> No, that's different, I219 got MAC from their own space.
>> MAC passthrough got MAC from ACPI "\MACA".
> 
> On my machine "\MACA" and I219 are the same, likely "\MACA" was
> populated by looking at that I219 by the BIOS.
> Not sure if "\MACA" can change when plugging docks, but probably not
> since the BIOS is also trying to implement inheritance of the main MAC.
> 
> Let me try this patch, maybe i do not get it.

That's expected, MAC passthrough is intended to replace I219.
This feature allows the MAC address of the native Ethernet network device on the system to be used
on dock.
The company network only see one MAC for your laptop.

Aaron

> 
> Henning
> 
>>> These collisions are simply bound to happen and probably very hard
>>> to avoid once you have set your mind on allowing pass-thru in the
>>> first place. Not sure whether that even has potential to disturb
>>> network equipment like switches.
>>>    
>>
>> After check MAC address, it will be more safe.
>>
>> Aaron
>>
>>> Henning
>>>    
>>>> Aaron
>>>>   
>>>>> Maybe i should try the patch but it seems like it defeats
>>>>> inheritance completely. Well depending on probe order ...
>>>>>
>>>>> regards,
>>>>> Henning
>>>>>
>>>>>       
>>>>>>>> Maybe leave here with
>>>>>>>> netif_info()
>>>>>>>>         
>>>>>>
>>>>>> Not good to print in rcu lock.
>>>>>>      
>>>>>>>> And move the whole block up, we can skip the whole ACPI story
>>>>>>>> if we find the MAC busy.
>>>>>>>
>>>>>>> That is wrong, need to know that MAC so can not move up too
>>>>>>> much. But maybe above the is_valid_ether_addr
>>>>>>
>>>>>> The MAC passthough address is read from ACPI.
>>>>>> ACPI read only happens once during r8152 driver probe.
>>>>>> To keep the lock less time, do it after is_valid_ether_addr.
>>>>>>      
>>>>>>>
>>>>>>> Henning
>>>>>>>          
>>>>>>>>> +		}
>>>>>>>>> +	}
>>>>>>>>> +	rcu_read_unlock();
>>>>>>>>
>>>>>>>> Not sure if this function is guaranteed to only run once at a
>>>>>>>> time, otherwise i think that is a race. Multiple instances
>>>>>>>> could make it to this very point at the same time.
>>>>>>>>         
>>>>>>
>>>>>> Run once for one device.
>>>>>> So add a safe lock.
>>>>>>
>>>>>> Aaron
>>>>>>      
>>>>>>>> Henning
>>>>>>>>         
>>>>>>>>>      	memcpy(sa->sa_data, buf, 6);
>>>>>>>>>      	netif_info(tp, probe, tp->netdev,
>>>>>>>>>      		   "Using pass-thru MAC addr %pM\n",
>>>>>>>>> sa->sa_data);
>>>>>>>>         
>>>>>>>          
>>>>>       
>>>    
> 

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Am Wed, 5 Jan 2022 09:47:02 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Wed, 5 Jan 2022 16:37:11 +0800
> schrieb Aaron Ma <aaron.ma@canonical.com>:
> 
> > On 1/5/22 16:32, Henning Schild wrote:  
> > > Am Wed, 5 Jan 2022 16:01:24 +0800
> > > schrieb Aaron Ma <aaron.ma@canonical.com>:
> > >     
> > >> On 1/5/22 15:55, Henning Schild wrote:    
> > >>> Am Wed, 5 Jan 2022 15:38:51 +0800
> > >>> schrieb Aaron Ma <aaron.ma@canonical.com>:
> > >>>        
> > >>>> On 1/5/22 15:32, Henning Schild wrote:    
> > >>>>> Am Wed, 5 Jan 2022 08:23:55 +0100
> > >>>>> schrieb Henning Schild <henning.schild@siemens.com>:
> > >>>>>           
> > >>>>>> Hi Aaron,
> > >>>>>>
> > >>>>>> if this or something similar goes in, please add another
> > >>>>>> patch to remove the left-over defines.
> > >>>>>>          
> > >>>>
> > >>>> Sure, I will do it.
> > >>>>       
> > >>>>>> Am Wed,  5 Jan 2022 14:17:47 +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.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> > >>>>>>> ---
> > >>>>>>>     drivers/net/usb/r8152.c | 10 ++++++++++
> > >>>>>>>     1 file changed, 10 insertions(+)
> > >>>>>>>
> > >>>>>>> diff --git a/drivers/net/usb/r8152.c
> > >>>>>>> b/drivers/net/usb/r8152.c index f9877a3e83ac..77f11b3f847b
> > >>>>>>> 100644 --- a/drivers/net/usb/r8152.c
> > >>>>>>> +++ b/drivers/net/usb/r8152.c
> > >>>>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
> > >>>>>>> +			rcu_read_unlock();
> > >>>>>>> +			goto amacout;    
> > >>>>>>
> > >>>>>> Since the original PCI netdev will always be there, that
> > >>>>>> would disable inheritance would it not?
> > >>>>>> I guess a strncmp(MODULE_NAME, info->driver,
> > >>>>>> strlen(MODULE_NAME)) is needed as well.
> > >>>>>>          
> > >>>>
> > >>>> PCI ethernet could be a builtin one on dock since there will be
> > >>>> TBT4 dock.    
> > >>>
> > >>> In my X280 there is a PCI device in the laptop, always there.
> > >>> And its MAC is the one found in ACPI. Did not try but i think
> > >>> for such devices there would never be inheritance even if one
> > >>> wanted and used a Lenovo dock that is supposed to do it.
> > >>>        
> > >>
> > >> There will more TBT4 docks in market, the new ethernet is just
> > >> the same as PCI device, connected by thunderbolt.
> > >>
> > >> For exmaple, connect a TBT4 dock which uses i225 pcie base
> > >> ethernet, then connect another TBT3 dock which uses r8152.
> > >> If skip PCI check, then i225 and r8152 will use the same MAC.    
> > > 
> > > In current 5.15 i have that sort of collision already. All r8152s
> > > will happily grab the MAC of the I219. In fact i have only ever
> > > seen it with one r8152 at a time but while the I219 was actively
> > > in use. While this patch will probably solve that, i bet it would
> > > defeat MAC pass-thru altogether. Even when turned on in the BIOS.
> > > Or does that iterator take "up"/"down" state into consideration?
> > > But even if, the I219 could become "up" any time later.
> > >     
> > 
> > No, that's different, I219 got MAC from their own space.
> > MAC passthrough got MAC from ACPI "\MACA".  
> 
> On my machine "\MACA" and I219 are the same, likely "\MACA" was
> populated by looking at that I219 by the BIOS.
> Not sure if "\MACA" can change when plugging docks, but probably not
> since the BIOS is also trying to implement inheritance of the main
> MAC.
> 
> Let me try this patch, maybe i do not get it.

So i tried it and inheritance now is killed as expected because that
I219 has that MAC. So for devices that have a PCI device built-in ...
this patch is like removing pass-thru altogether. And very likely not
what you intended.

On top the device will receive a random MAC because there is no error
code before "goto amacount;" some randomness from the stack of the
caller of determine_ethernet_addr where the "sa"s come from.

I now have an "ret = -EBUSY" in mine ... but still there can never be
pass-thru because that I219 is always there.

This patch is very wrong! And i am still not sure about the race, you
did not say anything about that.

Henning

> Henning
> 
> > > These collisions are simply bound to happen and probably very hard
> > > to avoid once you have set your mind on allowing pass-thru in the
> > > first place. Not sure whether that even has potential to disturb
> > > network equipment like switches.
> > >     
> > 
> > After check MAC address, it will be more safe.
> > 
> > Aaron
> >   
> > > Henning
> > >     
> > >> Aaron
> > >>    
> > >>> Maybe i should try the patch but it seems like it defeats
> > >>> inheritance completely. Well depending on probe order ...
> > >>>
> > >>> regards,
> > >>> Henning
> > >>>
> > >>>        
> > >>>>>> Maybe leave here with
> > >>>>>> netif_info()
> > >>>>>>          
> > >>>>
> > >>>> Not good to print in rcu lock.
> > >>>>       
> > >>>>>> And move the whole block up, we can skip the whole ACPI story
> > >>>>>> if we find the MAC busy.    
> > >>>>>
> > >>>>> That is wrong, need to know that MAC so can not move up too
> > >>>>> much. But maybe above the is_valid_ether_addr    
> > >>>>
> > >>>> The MAC passthough address is read from ACPI.
> > >>>> ACPI read only happens once during r8152 driver probe.
> > >>>> To keep the lock less time, do it after is_valid_ether_addr.
> > >>>>       
> > >>>>>
> > >>>>> Henning
> > >>>>>           
> > >>>>>>> +		}
> > >>>>>>> +	}
> > >>>>>>> +	rcu_read_unlock();    
> > >>>>>>
> > >>>>>> Not sure if this function is guaranteed to only run once at a
> > >>>>>> time, otherwise i think that is a race. Multiple instances
> > >>>>>> could make it to this very point at the same time.
> > >>>>>>          
> > >>>>
> > >>>> Run once for one device.
> > >>>> So add a safe lock.
> > >>>>
> > >>>> Aaron
> > >>>>       
> > >>>>>> Henning
> > >>>>>>          
> > >>>>>>>     	memcpy(sa->sa_data, buf, 6);
> > >>>>>>>     	netif_info(tp, probe, tp->netdev,
> > >>>>>>>     		   "Using pass-thru MAC addr %pM\n",
> > >>>>>>> sa->sa_data);    
> > >>>>>>          
> > >>>>>           
> > >>>        
> > >     
> 

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Florian Fainelli]

On 1/5/22 12:37 AM, Aaron Ma wrote:
> 
> 
> On 1/5/22 16:32, Henning Schild wrote:
>> Am Wed, 5 Jan 2022 16:01:24 +0800
>> schrieb Aaron Ma <aaron.ma@canonical.com>:
>>
>>> On 1/5/22 15:55, Henning Schild wrote:
>>>> Am Wed, 5 Jan 2022 15:38:51 +0800
>>>> schrieb Aaron Ma <aaron.ma@canonical.com>:
>>>>   
>>>>> On 1/5/22 15:32, Henning Schild wrote:
>>>>>> Am Wed, 5 Jan 2022 08:23:55 +0100
>>>>>> schrieb Henning Schild <henning.schild@siemens.com>:
>>>>>>      
>>>>>>> Hi Aaron,
>>>>>>>
>>>>>>> if this or something similar goes in, please add another patch to
>>>>>>> remove the left-over defines.
>>>>>>>      
>>>>>
>>>>> Sure, I will do it.
>>>>>  
>>>>>>> Am Wed,  5 Jan 2022 14:17:47 +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.
>>>>>>>>
>>>>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>>>>>>>> ---
>>>>>>>>     drivers/net/usb/r8152.c | 10 ++++++++++
>>>>>>>>     1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>>>>>>>> index f9877a3e83ac..77f11b3f847b 100644
>>>>>>>> --- a/drivers/net/usb/r8152.c
>>>>>>>> +++ b/drivers/net/usb/r8152.c
>>>>>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
>>>>>>>> +            rcu_read_unlock();
>>>>>>>> +            goto amacout;
>>>>>>>
>>>>>>> Since the original PCI netdev will always be there, that would
>>>>>>> disable inheritance would it not?
>>>>>>> I guess a strncmp(MODULE_NAME, info->driver, strlen(MODULE_NAME))
>>>>>>> is needed as well.
>>>>>>>      
>>>>>
>>>>> PCI ethernet could be a builtin one on dock since there will be
>>>>> TBT4 dock.
>>>>
>>>> In my X280 there is a PCI device in the laptop, always there. And
>>>> its MAC is the one found in ACPI. Did not try but i think for such
>>>> devices there would never be inheritance even if one wanted and
>>>> used a Lenovo dock that is supposed to do it.
>>>>    
>>>
>>> There will more TBT4 docks in market, the new ethernet is just the
>>> same as PCI device, connected by thunderbolt.
>>>
>>> For exmaple, connect a TBT4 dock which uses i225 pcie base ethernet,
>>> then connect another TBT3 dock which uses r8152.
>>> If skip PCI check, then i225 and r8152 will use the same MAC.
>>
>> In current 5.15 i have that sort of collision already. All r8152s will
>> happily grab the MAC of the I219. In fact i have only ever seen it with
>> one r8152 at a time but while the I219 was actively in use.
>> While this patch will probably solve that, i bet it would defeat MAC
>> pass-thru altogether. Even when turned on in the BIOS.
>> Or does that iterator take "up"/"down" state into consideration? But
>> even if, the I219 could become "up" any time later.
>>
> 
> No, that's different, I219 got MAC from their own space.
> MAC passthrough got MAC from ACPI "\MACA".
> 
>> These collisions are simply bound to happen and probably very hard to
>> avoid once you have set your mind on allowing pass-thru in the first
>> place. Not sure whether that even has potential to disturb network
>> equipment like switches.
>>
> 
> After check MAC address, it will be more safe.

Sorry to just do a drive by review here, but why is passing through the
MAC a kernel problem and not something that you punt to user-space entirely?
-- 
Florian

Re: [PATCH] net: usb: r8152: Check used MAC passthrough address

[Henning Schild]

Am Wed, 5 Jan 2022 11:55:06 -0800
schrieb Florian Fainelli <f.fainelli@gmail.com>:

> On 1/5/22 12:37 AM, Aaron Ma wrote:
> > 
> > 
> > On 1/5/22 16:32, Henning Schild wrote:  
> >> Am Wed, 5 Jan 2022 16:01:24 +0800
> >> schrieb Aaron Ma <aaron.ma@canonical.com>:
> >>  
> >>> On 1/5/22 15:55, Henning Schild wrote:  
> >>>> Am Wed, 5 Jan 2022 15:38:51 +0800
> >>>> schrieb Aaron Ma <aaron.ma@canonical.com>:
> >>>>     
> >>>>> On 1/5/22 15:32, Henning Schild wrote:  
> >>>>>> Am Wed, 5 Jan 2022 08:23:55 +0100
> >>>>>> schrieb Henning Schild <henning.schild@siemens.com>:
> >>>>>>        
> >>>>>>> Hi Aaron,
> >>>>>>>
> >>>>>>> if this or something similar goes in, please add another
> >>>>>>> patch to remove the left-over defines.
> >>>>>>>        
> >>>>>
> >>>>> Sure, I will do it.
> >>>>>    
> >>>>>>> Am Wed,  5 Jan 2022 14:17:47 +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.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> >>>>>>>> ---
> >>>>>>>>     drivers/net/usb/r8152.c | 10 ++++++++++
> >>>>>>>>     1 file changed, 10 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/net/usb/r8152.c
> >>>>>>>> b/drivers/net/usb/r8152.c index f9877a3e83ac..77f11b3f847b
> >>>>>>>> 100644 --- a/drivers/net/usb/r8152.c
> >>>>>>>> +++ b/drivers/net/usb/r8152.c
> >>>>>>>> @@ -1605,6 +1605,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 +1663,15 @@ 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 (strncmp(buf, ndev->dev_addr, 6) == 0) {
> >>>>>>>> +            rcu_read_unlock();
> >>>>>>>> +            goto amacout;  
> >>>>>>>
> >>>>>>> Since the original PCI netdev will always be there, that would
> >>>>>>> disable inheritance would it not?
> >>>>>>> I guess a strncmp(MODULE_NAME, info->driver,
> >>>>>>> strlen(MODULE_NAME)) is needed as well.
> >>>>>>>        
> >>>>>
> >>>>> PCI ethernet could be a builtin one on dock since there will be
> >>>>> TBT4 dock.  
> >>>>
> >>>> In my X280 there is a PCI device in the laptop, always there. And
> >>>> its MAC is the one found in ACPI. Did not try but i think for
> >>>> such devices there would never be inheritance even if one wanted
> >>>> and used a Lenovo dock that is supposed to do it.
> >>>>      
> >>>
> >>> There will more TBT4 docks in market, the new ethernet is just the
> >>> same as PCI device, connected by thunderbolt.
> >>>
> >>> For exmaple, connect a TBT4 dock which uses i225 pcie base
> >>> ethernet, then connect another TBT3 dock which uses r8152.
> >>> If skip PCI check, then i225 and r8152 will use the same MAC.  
> >>
> >> In current 5.15 i have that sort of collision already. All r8152s
> >> will happily grab the MAC of the I219. In fact i have only ever
> >> seen it with one r8152 at a time but while the I219 was actively
> >> in use. While this patch will probably solve that, i bet it would
> >> defeat MAC pass-thru altogether. Even when turned on in the BIOS.
> >> Or does that iterator take "up"/"down" state into consideration?
> >> But even if, the I219 could become "up" any time later.
> >>  
> > 
> > No, that's different, I219 got MAC from their own space.
> > MAC passthrough got MAC from ACPI "\MACA".
> >   
> >> These collisions are simply bound to happen and probably very hard
> >> to avoid once you have set your mind on allowing pass-thru in the
> >> first place. Not sure whether that even has potential to disturb
> >> network equipment like switches.
> >>  
> > 
> > After check MAC address, it will be more safe.  
> 
> Sorry to just do a drive by review here, but why is passing through
> the MAC a kernel problem and not something that you punt to
> user-space entirely?

Agreed and several other people seem to feel the same way about
pass-thru not deserving a place in the kernel.

This all dates back to 34ee32c9a5696247be405bb0c21f3d1fc6cb5729
and some other patches that came later

9647722befbedcd6735e00655ffec392c05f0c56
c286909fe5458f69e533c845b757fd2c35064d26
8e29d23e28ee7fb995a00c1ca7e1a4caf5070b12
9c27369f4a1393452c17e8708c1b0beb8ac59501

Maybe other drivers are affected as well.

All of the patches should probably be reverted. If people care enough
they can try and get it into udev.

All patches put policy into the kernel, do weird ACPI lookups and cause
MAC conflicts with NICs that might be up and running. And will claim too
many r8512 devices in case there are multiple.

I propose to revert all of this or maybe add a module param (which
should probably default to "off") just to give people a way to preserve
their hacks.

If the BIOS did spoof we could try to keep that, but spoofing in the OS
(at least in the kernel) sound very wrong and caused me to start the
whole discussion after all my r8521 dongles all of a sudden had the
same (already busy) MAC when moving to v5.15. That was on a Lenovo
laptop but i am pretty sure Dell and HP would be affected as well.

When using another NIC you get another MAC, it is that simple. If that
causes issues with DHCP/PXE deal with it. A MAC does not id a machine,
maybe x509 radius does. Not a kernel story!

Henning