[RFC] r8152: pass through needs to be singular

[RFC] r8152: pass through needs to be singular

[Oliver Neukum]

If multiple devices are connected only one of them
is allowed to get the pass through MAC

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/r8152.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0f6efaabaa32..15f695398284 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1193,6 +1193,9 @@ static unsigned int agg_buf_sz = 16384;
 
 #define RTL_LIMITED_TSO_SIZE	(size_to_mtu(agg_buf_sz) - sizeof(struct tx_desc))
 
+static struct r8152 *holder_of_pass_through = NULL;
+static DEFINE_MUTEX(pass_through_lock);
+
 static
 int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 {
@@ -1587,8 +1590,19 @@ static int __rtl8152_set_mac_address(struct net_device *netdev, void *p,
 	return ret;
 }
 
+static void give_up_pass_through(struct r8152 *tp)
+{
+	mutex_lock(&pass_through_lock);
+	if (tp == holder_of_pass_through)
+		holder_of_pass_through = NULL;
+	mutex_unlock(&pass_through_lock);
+}
+
 static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
 {
+	struct r8152 *tp = netdev_priv(netdev);
+
+	give_up_pass_through(tp);
 	return __rtl8152_set_mac_address(netdev, p, false);
 }
 
@@ -1608,6 +1622,12 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	acpi_object_type mac_obj_type;
 	int mac_strlen;
 
+	mutex_lock(&pass_through_lock);
+
+	if (!holder_of_pass_through) {
+		ret = -EBUSY;
+		goto failout;
+	}
 	if (tp->lenovo_macpassthru) {
 		mac_obj_name = "\\MACA";
 		mac_obj_type = ACPI_TYPE_STRING;
@@ -1621,7 +1641,8 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 			if ((ocp_data & PASS_THRU_MASK) != 1) {
 				netif_dbg(tp, probe, tp->netdev,
 						"No efuse for RTL8153-AD MAC pass through\n");
-				return -ENODEV;
+				ret = -ENODEV;
+				goto failout;
 			}
 		} else {
 			/* test for RTL8153-BND and RTL8153-BD */
@@ -1629,7 +1650,8 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 			if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) {
 				netif_dbg(tp, probe, tp->netdev,
 						"Invalid variant for MAC pass through\n");
-				return -ENODEV;
+				ret = -ENODEV;
+				goto failout;
 			}
 		}
 
@@ -1641,8 +1663,10 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	/* returns _AUXMAC_#AABBCCDDEEFF# */
 	status = acpi_evaluate_object(NULL, mac_obj_name, NULL, &buffer);
 	obj = (union acpi_object *)buffer.pointer;
-	if (!ACPI_SUCCESS(status))
-		return -ENODEV;
+	if (!ACPI_SUCCESS(status)) {
+		ret = -ENODEV;
+		goto failout;
+	}
 	if (obj->type != mac_obj_type || obj->string.length != mac_strlen) {
 		netif_warn(tp, probe, tp->netdev,
 			   "Invalid buffer for pass-thru MAC addr: (%d, %d)\n",
@@ -1670,6 +1694,10 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 
 amacout:
 	kfree(obj);
+failout:
+	if (!ret)
+		holder_of_pass_through = tp;
+	mutex_unlock(&pass_through_lock);
 	return ret;
 }
 
@@ -8287,6 +8315,7 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
 		tp->rtl_ops.disable(tp);
 		mutex_unlock(&tp->control);
 	}
+	give_up_pass_through(tp);
 
 	return 0;
 }
@@ -9802,6 +9831,7 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 		cancel_delayed_work_sync(&tp->hw_phy_work);
 		if (tp->rtl_ops.unload)
 			tp->rtl_ops.unload(tp);
+		give_up_pass_through(tp);
 		rtl8152_release_firmware(tp);
 		free_netdev(tp->netdev);
 	}
-- 
2.35.3

Re: [RFC] r8152: pass through needs to be singular

[Andrew Lunn]

On Thu, Jul 28, 2022 at 09:18:51PM +0200, Oliver Neukum wrote:
> If multiple devices are connected only one of them
> is allowed to get the pass through MAC

Is that true? Ethernet switches often use the same MAC address on
multiple ports. It is not inherently broken to have the same MAC
address on multiple interfaces.

We also need to watch out for regressions. There could be users who do
have the same MAC address on multiple interfaces, and it works for
them. With this change it is a 50/50 chance they no longer get the MAC
address they expect, depending on probe order.

We know this implementation of pass through it very broken, but
unfortunately, it is there, and we have to follow the normal
regression rules, even if we really would like to throw it all out.

What exactly is your problem which you are trying to fix? 

     Andrew

Re: [RFC] r8152: pass through needs to be singular

[Oliver Neukum]

On 29.07.22 00:11, Andrew Lunn wrote:
> On Thu, Jul 28, 2022 at 09:18:51PM +0200, Oliver Neukum wrote:
>> If multiple devices are connected only one of them
>> is allowed to get the pass through MAC
> 
> Is that true? Ethernet switches often use the same MAC address on
> multiple ports. It is not inherently broken to have the same MAC
> address on multiple interfaces.

Well, yes, but for a host to assigning the same MAC by default
is a bug. The whole point of MACs is kind of that they be unique.
For the same reason I fixed usbnet to not hand out the same
random address. In theory I have broken setups which relied on
people reconnecting a device getting the same MAC.

> We know this implementation of pass through it very broken, but
> unfortunately, it is there, and we have to follow the normal
> regression rules, even if we really would like to throw it all out.

True. Nevertheless, do we really want to say that we dislike a design
so much that we are not fixing bugs?

> What exactly is your problem which you are trying to fix? 
Adressing the comment Hayes made when reset_resume() was fixed
from a deadlock, that it still assigns wrong MACs. I feel that
before I fix keeping the correct address I better make sure the
MAC is sane in the first place.

	Regards
		Oliver

Re: [RFC] r8152: pass through needs to be singular

[Andrew Lunn]

> True. Nevertheless, do we really want to say that we dislike a design
> so much that we are not fixing bugs?

I'm not sure we can fix it. Part of that long thread about why this
whole concept is broken is that we have no idea which interface is the
one which should give the MAC address to. If we change it to only give
out the MAC address once, all we really do is change it from one bug
to another bug.

> > What exactly is your problem which you are trying to fix? 
> Adressing the comment Hayes made when reset_resume() was fixed
> from a deadlock, that it still assigns wrong MACs. I feel that
> before I fix keeping the correct address I better make sure the
> MAC is sane in the first place.

I would say that reset_resume() should restore whatever the MAC
address was before the suspend. It does not matter if the MAC address
is not unique. As far as i know, the kernel never prevents the user
assigning the same MAC address on multiple interfaces via ip link set.
So it could actually be a user choice.

   Andrew

RE: [RFC] r8152: pass through needs to be singular

[Hayes Wang]

Oliver Neukum <oneukum@suse.com>
> Sent: Friday, July 29, 2022 3:19 AM
[...]
> @@ -1608,6 +1622,12 @@ static int vendor_mac_passthru_addr_read(struct
> r8152 *tp, struct sockaddr *sa)
>  	acpi_object_type mac_obj_type;
>  	int mac_strlen;
> 
> +	mutex_lock(&pass_through_lock);
> +
> +	if (!holder_of_pass_through) {
> +		ret = -EBUSY;
> +		goto failout;
> +	}

Excuse me. I have one question.
When is the holder_of_pass_through set?
The default value of holder_of_pass_through is NULL, so
it seems the holder_of_pass_through would never be set.

Best Regards,
Hayes

>  	if (tp->lenovo_macpassthru) {
>  		mac_obj_name = "\\MACA";
>  		mac_obj_type = ACPI_TYPE_STRING;
> @@ -1621,7 +1641,8 @@ static int vendor_mac_passthru_addr_read(struct
> r8152 *tp, struct sockaddr *sa)
>  			if ((ocp_data & PASS_THRU_MASK) != 1) {
>  				netif_dbg(tp, probe, tp->netdev,
>  						"No efuse for RTL8153-AD MAC pass through\n");
> -				return -ENODEV;
> +				ret = -ENODEV;
> +				goto failout;
>  			}
>  		} else {
>  			/* test for RTL8153-BND and RTL8153-BD */
> @@ -1629,7 +1650,8 @@ static int vendor_mac_passthru_addr_read(struct
> r8152 *tp, struct sockaddr *sa)
>  			if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK)
> == 0) {
>  				netif_dbg(tp, probe, tp->netdev,
>  						"Invalid variant for MAC pass through\n");
> -				return -ENODEV;
> +				ret = -ENODEV;
> +				goto failout;
>  			}
>  		}
> 
> @@ -1641,8 +1663,10 @@ static int vendor_mac_passthru_addr_read(struct
> r8152 *tp, struct sockaddr *sa)
>  	/* returns _AUXMAC_#AABBCCDDEEFF# */
>  	status = acpi_evaluate_object(NULL, mac_obj_name, NULL, &buffer);
>  	obj = (union acpi_object *)buffer.pointer;
> -	if (!ACPI_SUCCESS(status))
> -		return -ENODEV;
> +	if (!ACPI_SUCCESS(status)) {
> +		ret = -ENODEV;
> +		goto failout;
> +	}
>  	if (obj->type != mac_obj_type || obj->string.length != mac_strlen) {
>  		netif_warn(tp, probe, tp->netdev,
>  			   "Invalid buffer for pass-thru MAC addr: (%d, %d)\n",
> @@ -1670,6 +1694,10 @@ static int vendor_mac_passthru_addr_read(struct
> r8152 *tp, struct sockaddr *sa)
> 
>  amacout:
>  	kfree(obj);
> +failout:
> +	if (!ret)
> +		holder_of_pass_through = tp;
> +	mutex_unlock(&pass_through_lock);
>  	return ret;
>  }

Re: [RFC] r8152: pass through needs to be singular

[Oliver Neukum]

On 03.08.22 05:48, Hayes Wang wrote:
> Oliver Neukum <oneukum@suse.com>
>> Sent: Friday, July 29, 2022 3:19 AM
> [...]
>> @@ -1608,6 +1622,12 @@ static int vendor_mac_passthru_addr_read(struct
>> r8152 *tp, struct sockaddr *sa)
>>  	acpi_object_type mac_obj_type;
>>  	int mac_strlen;
>>
>> +	mutex_lock(&pass_through_lock);
>> +
>> +	if (!holder_of_pass_through) {
>> +		ret = -EBUSY;
>> +		goto failout;
>> +	}
> 
> Excuse me. I have one question.
> When is the holder_of_pass_through set?
> The default value of holder_of_pass_through is NULL, so
> it seems the holder_of_pass_through would never be set.


Hi,

here in vendor_mac_passthru_addr_read()

>>  amacout:
>>  	kfree(obj);
>> +failout:
>> +	if (!ret)
>> +		holder_of_pass_through = tp;
>> +	mutex_unlock(&pass_through_lock);
>>  	return ret;
>>  }

	Regards
		Oliver

Re: [RFC] r8152: pass through needs to be singular

[Oliver Neukum]

On 02.08.22 15:31, Andrew Lunn wrote:
>> True. Nevertheless, do we really want to say that we dislike a design
>> so much that we are not fixing bugs?
> 
> I'm not sure we can fix it. Part of that long thread about why this
> whole concept is broken is that we have no idea which interface is the
> one which should give the MAC address to. If we change it to only give
> out the MAC address once, all we really do is change it from one bug
> to another bug.

I am afraid I have to beg to differ.

We have a couple of related issues here. Obviously whoever designed
MAC pass-through did not consider the case of somebody using two
docking stations at the same time. While pass-through is used I agree
that this is unsolvable.

Yet connecting two (or even more) docking stations to a host is
within spec. They are USB devices (partially) and if they contain
a NIC it is clear what we have to do.
We would operate them with
1. a MAC contained on the device
2. if the device contains no MAC, we'd use a random MAC, but a
different one per device. User space can assign any MAC it wants to,
but we are talking defaults here.

Now, the question arises whether we let another feature interfere
with that. And then I must say, if we do that and we have decided
to take a feature that does so, we'll have to do it in a way that
stays within spec. Yet I am sorry, you cannot give out the same
MAC to two devices at the same time, if you want to do so.

So while the bug in the driver derives from a stupid design,
it nevertheless is a bug and my patch fixes it. Optimally?
No, of course not, the design is broken.

>>> What exactly is your problem which you are trying to fix? 
>> Adressing the comment Hayes made when reset_resume() was fixed
>> from a deadlock, that it still assigns wrong MACs. I feel that
>> before I fix keeping the correct address I better make sure the
>> MAC is sane in the first place.
> 
> I would say that reset_resume() should restore whatever the MAC
> address was before the suspend.

The problem is that these devices reread the passed through MAC
at post_reset(). Do you want reset_resume() to act differently?

> It does not matter if the MAC address
> is not unique. As far as i know, the kernel never prevents the user
> assigning the same MAC address on multiple interfaces via ip link set.
> So it could actually be a user choice.

Iff the user does so. Until then it is a kernel bug.

	Regards
		Oliver

Re: [RFC] r8152: pass through needs to be singular

[Andrew Lunn]

> I am afraid I have to beg to differ.
> 
> We have a couple of related issues here. Obviously whoever designed
> MAC pass-through did not consider the case of somebody using two
> docking stations at the same time. While pass-through is used I agree
> that this is unsolvable.
> 
> Yet connecting two (or even more) docking stations to a host is
> within spec. They are USB devices (partially) and if they contain
> a NIC it is clear what we have to do.
> We would operate them with
> 1. a MAC contained on the device
> 2. if the device contains no MAC, we'd use a random MAC, but a
> different one per device. User space can assign any MAC it wants to,
> but we are talking defaults here.
> 
> Now, the question arises whether we let another feature interfere
> with that. And then I must say, if we do that and we have decided
> to take a feature that does so, we'll have to do it in a way that
> stays within spec. Yet I am sorry, you cannot give out the same
> MAC to two devices at the same time, if you want to do so.
> 
> So while the bug in the driver derives from a stupid design,
> it nevertheless is a bug and my patch fixes it. Optimally?
> No, of course not, the design is broken.

The problem is regressions. Current code will put the MAC address on
both. I guess most users just have one dock with a cabled and the
second is unused. Both will get the same MAC address, the DHCP server
will recognise the MAC address and give out the expected IP address.

If you change it to only give the MAC address out once, you get into a
race condition, which device probes first, gets the MAC, and gives the
expected MAC to the DHCP server? You are going to see regressions.

> >>> What exactly is your problem which you are trying to fix? 
> >> Adressing the comment Hayes made when reset_resume() was fixed
> >> from a deadlock, that it still assigns wrong MACs. I feel that
> >> before I fix keeping the correct address I better make sure the
> >> MAC is sane in the first place.
> > 
> > I would say that reset_resume() should restore whatever the MAC
> > address was before the suspend.
> 
> The problem is that these devices reread the passed through MAC
> at post_reset(). Do you want reset_resume() to act differently?

I would expect whatever MAC address is in the netdev structure to be
put on the interface at resume. That should of been the MAC it was
using before suspend. And by doing that, you bypass all the discussion
about where it came from.

      Andrew

Re: [RFC] r8152: pass through needs to be singular

[Oliver Neukum]

On 04.08.22 15:24, Andrew Lunn wrote:

> The problem is regressions. Current code will put the MAC address on
> both. I guess most users just have one dock with a cabled and the
> second is unused. Both will get the same MAC address, the DHCP server
> will recognise the MAC address and give out the expected IP address.

1. That is a rather esoteric situation in which a bug is exploited
2. We are at a philosophical point where I need to argue that fixing
a bug is necessarily a change in behavior.

> I would expect whatever MAC address is in the netdev structure to be
> put on the interface at resume. That should of been the MAC it was
> using before suspend. And by doing that, you bypass all the discussion
> about where it came from.

Debatable, but that's not what the driver does. It reacquires the MAC
from the firmware. I want to change that. Though obviously that changes
behavior.

I am sorry, but at some point a bug is a bug, even if some people like
it.

	Regards
		Oliver

Re: [RFC] r8152: pass through needs to be singular

[Andrew Lunn]

> I am sorry, but at some point a bug is a bug, even if some people like
> it.

So i guess we go with what we said last time. We can merge a change,
but if anybody reports a regression, it will be reverted. And if i
remember correctly, we did revert some stuff in this area.

But i think it would be better to consider RTL8153-BND, RTL8153-BD and
RTL8153-AD as poisoned, and make new designs with a different device.

	   Andrew

RE: [RFC] r8152: pass through needs to be singular

[Hayes Wang]

Oliver Neukum <oneukum@suse.com>
> Sent: Thursday, August 4, 2022 4:58 PM
[...]
> >> +	if (!holder_of_pass_through) {
> >> +		ret = -EBUSY;
> >> +		goto failout;
> >> +	}
> >
> > Excuse me. I have one question.
> > When is the holder_of_pass_through set?
> > The default value of holder_of_pass_through is NULL, so
> > it seems the holder_of_pass_through would never be set.
> 
> 
> Hi,
> 
> here in vendor_mac_passthru_addr_read()

I mean that holder_of_pass_through is NULL, so you set ret = -EBUSY and goto failout.
However, holder_of_pass_through is set only if ret == 0. That is,
holder_of_pass_through = tp would never occur, because ret is equal -EBUSY.
The default value of holder_of_pass_through is NULL, so it has no chance to be set.
Right?

Best Regards,
Hayes

> >>  amacout:
> >>  	kfree(obj);
> >> +failout:
> >> +	if (!ret)
> >> +		holder_of_pass_through = tp;
> >> +	mutex_unlock(&pass_through_lock);
> >>  	return ret;
> >>  }

Re: [RFC] r8152: pass through needs to be singular

[Oliver Neukum]

On 08.08.22 09:04, Hayes Wang wrote:
> Oliver Neukum <oneukum@suse.com>
>> Sent: Thursday, August 4, 2022 4:58 PM
> [...]
>>>> +	if (!holder_of_pass_through) {
>>>> +		ret = -EBUSY;
>>>> +		goto failout;
>>>> +	}
>>>
>>> Excuse me. I have one question.
>>> When is the holder_of_pass_through set?
>>> The default value of holder_of_pass_through is NULL, so
>>> it seems the holder_of_pass_through would never be set.
>>
>>
>> Hi,
>>
>> here in vendor_mac_passthru_addr_read()
> 
> I mean that holder_of_pass_through is NULL, so you set ret = -EBUSY and goto failout.
> However, holder_of_pass_through is set only if ret == 0. That is,
> holder_of_pass_through = tp would never occur, because ret is equal -EBUSY.
> The default value of holder_of_pass_through is NULL, so it has no chance to be set.
> Right?
>

Hi,

sorry, you are right. I made a "!" in error.
I'll correct that.

	Regards
		Oliver