linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses
@ 2020-10-10  6:44 Anant Thazhemadam
  2020-10-10 16:59 ` Jakub Kicinski
  2020-10-11 17:30 ` [PATCH v2] " Anant Thazhemadam
  0 siblings, 2 replies; 16+ messages in thread
From: Anant Thazhemadam @ 2020-10-10  6:44 UTC (permalink / raw)
  Cc: linux-kernel-mentees, Anant Thazhemadam, Petko Manolov,
	David S. Miller, Jakub Kicinski, linux-usb, netdev, linux-kernel

get_registers() directly returns the return value of
usb_control_msg_recv() - 0 if successful, and negative error number 
otherwise.
However, in set_ethernet_addr(), this return value is incorrectly 
checked.

Since this return value will never be equal to sizeof(node_id), a 
random MAC address will always be generated and assigned to the 
device; even in cases when get_registers() is successful.

Correctly modifying the condition that checks if get_registers() was 
successful or not fixes this problem, and copies the ethernet address
appropriately.

Fixes: f45a4248ea4c ("set random MAC address when set_ethernet_addr() fails")
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
 drivers/net/usb/rtl8150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index f020401adf04..bf8a60533f3e 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
 
 	ret = get_registers(dev, IDR, sizeof(node_id), node_id);
 
-	if (ret == sizeof(node_id)) {
+	if (!ret) {
 		ether_addr_copy(dev->netdev->dev_addr, node_id);
 	} else {
 		eth_hw_addr_random(dev->netdev);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-10  6:44 [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses Anant Thazhemadam
@ 2020-10-10 16:59 ` Jakub Kicinski
  2020-10-10 18:04   ` Anant Thazhemadam
  2020-10-11 17:30 ` [PATCH v2] " Anant Thazhemadam
  1 sibling, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2020-10-10 16:59 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: linux-kernel-mentees, Petko Manolov, David S. Miller, linux-usb,
	netdev, linux-kernel

On Sat, 10 Oct 2020 12:14:59 +0530 Anant Thazhemadam wrote:
> get_registers() directly returns the return value of
> usb_control_msg_recv() - 0 if successful, and negative error number 
> otherwise.

Are you expecting Greg to take this as a part of some USB subsystem
changes? I don't see usb_control_msg_recv() in my tree, and the
semantics of usb_control_msg() are not what you described.

> However, in set_ethernet_addr(), this return value is incorrectly 
> checked.
> 
> Since this return value will never be equal to sizeof(node_id), a 
> random MAC address will always be generated and assigned to the 
> device; even in cases when get_registers() is successful.
> 
> Correctly modifying the condition that checks if get_registers() was 
> successful or not fixes this problem, and copies the ethernet address
> appropriately.
> 
> Fixes: f45a4248ea4c ("set random MAC address when set_ethernet_addr() fails")
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>

The fixes tag does not follow the standard format:

Fixes tag: Fixes: f45a4248ea4c ("set random MAC address when set_ethernet_addr() fails")
Has these problem(s):
	- Subject does not match target commit subject
	  Just use
		git log -1 --format='Fixes: %h ("%s")'


Please put the relevant maintainer in the To: field of the email, and
even better - also mark the patch as [PATCH net], since it's a
networking fix.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-10 16:59 ` Jakub Kicinski
@ 2020-10-10 18:04   ` Anant Thazhemadam
  2020-10-10 18:16     ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Anant Thazhemadam @ 2020-10-10 18:04 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-kernel-mentees, Petko Manolov, David S. Miller, linux-usb,
	netdev, linux-kernel

Hi,

On 10/10/20 10:29 pm, Jakub Kicinski wrote:
> On Sat, 10 Oct 2020 12:14:59 +0530 Anant Thazhemadam wrote:
>> get_registers() directly returns the return value of
>> usb_control_msg_recv() - 0 if successful, and negative error number 
>> otherwise.
> Are you expecting Greg to take this as a part of some USB subsystem
> changes? I don't see usb_control_msg_recv() in my tree, and the
> semantics of usb_control_msg() are not what you described.

No, I'm not. usb_control_msg_recv() is an API that was recently
introduced, and get_registers() in rtl8150.c was also modified to
use it in order to prevent partial reads.

By your tree, I assume you mean
    https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/
(it was the only one I could find).

I don't see the commit that this patch is supposed to fix in your
tree either... :/

Nonetheless, this commit fixes an issue that was applied to the
networking tree, and has made its way into linux-next as well, if
I'm not mistaken.

>> However, in set_ethernet_addr(), this return value is incorrectly 
>> checked.
>>
>> Since this return value will never be equal to sizeof(node_id), a 
>> random MAC address will always be generated and assigned to the 
>> device; even in cases when get_registers() is successful.
>>
>> Correctly modifying the condition that checks if get_registers() was 
>> successful or not fixes this problem, and copies the ethernet address
>> appropriately.
>>
>> Fixes: f45a4248ea4c ("set random MAC address when set_ethernet_addr() fails")
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> The fixes tag does not follow the standard format:
>
> Fixes tag: Fixes: f45a4248ea4c ("set random MAC address when set_ethernet_addr() fails")
> Has these problem(s):
> 	- Subject does not match target commit subject
> 	  Just use
> 		git log -1 --format='Fixes: %h ("%s")'
>
>
> Please put the relevant maintainer in the To: field of the email, and
> even better - also mark the patch as [PATCH net], since it's a
> networking fix.

The script I've been using for sending patches in had been configured to CC
the maintainer(s) and respective mailing list(s). Sorry about that.

I will put the relevant maintainer in the To: field, fix the Fixes tag, and
mark the patch as [PATCH net] as well and send in a v2.

Thank you for your time.

Thanks,
Anant

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-10 18:04   ` Anant Thazhemadam
@ 2020-10-10 18:16     ` Jakub Kicinski
  2020-10-10 18:44       ` Anant Thazhemadam
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2020-10-10 18:16 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: linux-kernel-mentees, Petko Manolov, David S. Miller, linux-usb,
	netdev, linux-kernel

On Sat, 10 Oct 2020 23:34:51 +0530 Anant Thazhemadam wrote:
> On 10/10/20 10:29 pm, Jakub Kicinski wrote:
> > On Sat, 10 Oct 2020 12:14:59 +0530 Anant Thazhemadam wrote:  
> >> get_registers() directly returns the return value of
> >> usb_control_msg_recv() - 0 if successful, and negative error number 
> >> otherwise.  
> > Are you expecting Greg to take this as a part of some USB subsystem
> > changes? I don't see usb_control_msg_recv() in my tree, and the
> > semantics of usb_control_msg() are not what you described.  
> 
> No, I'm not. usb_control_msg_recv() is an API that was recently
> introduced, and get_registers() in rtl8150.c was also modified to
> use it in order to prevent partial reads.
> 
> By your tree, I assume you mean
>     https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/
> (it was the only one I could find).
> 
> I don't see the commit that this patch is supposed to fix in your
> tree either... :/
> 
> Nonetheless, this commit fixes an issue that was applied to the
> networking tree, and has made its way into linux-next as well, if
> I'm not mistaken.

I mean the networking tree, what's the commit ID in linux-next?

Your fixes tag points to f45a4248ea4c, but looks like the code was
quite correct at that point.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-10 18:16     ` Jakub Kicinski
@ 2020-10-10 18:44       ` Anant Thazhemadam
  2020-10-10 19:20         ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Anant Thazhemadam @ 2020-10-10 18:44 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-kernel-mentees, Petko Manolov, David S. Miller, linux-usb,
	netdev, linux-kernel


On 10/10/20 11:46 pm, Jakub Kicinski wrote:
> On Sat, 10 Oct 2020 23:34:51 +0530 Anant Thazhemadam wrote:
>> On 10/10/20 10:29 pm, Jakub Kicinski wrote:
>>> On Sat, 10 Oct 2020 12:14:59 +0530 Anant Thazhemadam wrote:  
>>>> get_registers() directly returns the return value of
>>>> usb_control_msg_recv() - 0 if successful, and negative error number 
>>>> otherwise.  
>>> Are you expecting Greg to take this as a part of some USB subsystem
>>> changes? I don't see usb_control_msg_recv() in my tree, and the
>>> semantics of usb_control_msg() are not what you described.  
>> No, I'm not. usb_control_msg_recv() is an API that was recently
>> introduced, and get_registers() in rtl8150.c was also modified to
>> use it in order to prevent partial reads.
>>
>> By your tree, I assume you mean
>>     https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/
>> (it was the only one I could find).
>>
>> I don't see the commit that this patch is supposed to fix in your
>> tree either... :/
>>
>> Nonetheless, this commit fixes an issue that was applied to the
>> networking tree, and has made its way into linux-next as well, if
>> I'm not mistaken.
> I mean the networking tree, what's the commit ID in linux-next?
>
> Your fixes tag points to f45a4248ea4c, but looks like the code was
> quite correct at that point.


Ah, my apologies. You're right. It doesn't look like those helpers have made
their way into the networking tree yet.

(This gets mentioned here as well,
    https://www.mail-archive.com/netdev@vger.kernel.org/msg357843.html)

The commit ID pointed to by the fixes tag is correct.
The change introduced by said commit looks right, but is logically incorrect.

get_registers() directly returns the return value of usb_control_msg_recv(),
and usb_control_msg_recv() returns 0 on success and negative error number
otherwise.

(You can find more about the new helpers here
    https://lore.kernel.org/alsa-devel/20200914153756.3412156-1-gregkh@linuxfoundation.org/ )

The commit ID mentioned introduces a change that is supposed to copy over
the ethernet only when get_registers() succeeds, i.e., a complete read occurs,
and generate and set a random ethernet address otherwise (reading the
commit message should give some more insight).

The condition that checks if get_registers() succeeds (as specified in f45a4248ea4c)
was,
    ret == sizeof(node_id)
where ret is the return value of get_registers().

However, ret will never equal sizeof(node_id), since ret can only be equal to 0
or a negative number.

Thus, even in case where get_registers() succeeds, a randomly generated MAC
address would get copied over, instead of copying the appropriate ethernet
address, which is logically incorrect and not optimal.

Hence, we need to modify this to check if (ret == 0), and copy over the correct
ethernet address in that case, instead of randomly generating one and assigning
that.
Hope this helps.

Thanks,
Anant



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-10 18:44       ` Anant Thazhemadam
@ 2020-10-10 19:20         ` Jakub Kicinski
  0 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2020-10-10 19:20 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: linux-kernel-mentees, Petko Manolov, David S. Miller, linux-usb,
	netdev, linux-kernel, Stephen Rothwell, Linux Next Mailing List

On Sun, 11 Oct 2020 00:14:05 +0530 Anant Thazhemadam wrote:
> Ah, my apologies. You're right. It doesn't look like those helpers have made
> their way into the networking tree yet.
> 
> (This gets mentioned here as well,
>     https://www.mail-archive.com/netdev@vger.kernel.org/msg357843.html)
> 
> The commit ID pointed to by the fixes tag is correct.
> The change introduced by said commit looks right, but is logically incorrect.
> 
> get_registers() directly returns the return value of usb_control_msg_recv(),
> and usb_control_msg_recv() returns 0 on success and negative error number
> otherwise.
> 
> (You can find more about the new helpers here
>     https://lore.kernel.org/alsa-devel/20200914153756.3412156-1-gregkh@linuxfoundation.org/ )
> 
> The commit ID mentioned introduces a change that is supposed to copy over
> the ethernet only when get_registers() succeeds, i.e., a complete read occurs,
> and generate and set a random ethernet address otherwise (reading the
> commit message should give some more insight).
> 
> The condition that checks if get_registers() succeeds (as specified in f45a4248ea4c)
> was,
>     ret == sizeof(node_id)
> where ret is the return value of get_registers().
> 
> However, ret will never equal sizeof(node_id), since ret can only be equal to 0
> or a negative number.
> 
> Thus, even in case where get_registers() succeeds, a randomly generated MAC
> address would get copied over, instead of copying the appropriate ethernet
> address, which is logically incorrect and not optimal.
> 
> Hence, we need to modify this to check if (ret == 0), and copy over the correct
> ethernet address in that case, instead of randomly generating one and assigning
> that.

I see... so we ended up with your fix applied to net, and Petko's
rework applied to the usb/usb-next tree.

What you're actually fixing is the improper resolution of the resulting
conflict in linux-next!

CCing Stephen and linux-next.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-10  6:44 [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses Anant Thazhemadam
  2020-10-10 16:59 ` Jakub Kicinski
@ 2020-10-11 17:30 ` Anant Thazhemadam
  2020-10-11 17:59   ` Jakub Kicinski
  2020-10-11 22:14   ` Stephen Rothwell
  1 sibling, 2 replies; 16+ messages in thread
From: Anant Thazhemadam @ 2020-10-11 17:30 UTC (permalink / raw)
  To: petkan, davem, kuba
  Cc: Anant Thazhemadam, linux-usb, netdev, linux-kernel,
	linux-kernel-mentees, linux-next, sfr

In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
that was read must be copied over. Otherwise, a random ethernet address
must be assigned.

get_registers() returns 0 if successful, and negative error number
otherwise. However, in set_ethernet_addr(), this return value is
incorrectly checked.

Since this return value will never be equal to sizeof(node_id), a
random MAC address will always be generated and assigned to the
device; even in cases when get_registers() is successful.

Correctly modifying the condition that checks if get_registers() was
successful or not fixes this problem, and copies the ethernet address
appropriately.

Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
Changes in v2:
        * Fixed the format of the Fixes tag
        * Modified the commit message to better describe the issue being 
          fixed

+CCing Stephen and linux-next, since the commit fixed isn't in the networking
tree, but is present in linux-next.

 drivers/net/usb/rtl8150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index f020401adf04..bf8a60533f3e 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
 
 	ret = get_registers(dev, IDR, sizeof(node_id), node_id);
 
-	if (ret == sizeof(node_id)) {
+	if (!ret) {
 		ether_addr_copy(dev->netdev->dev_addr, node_id);
 	} else {
 		eth_hw_addr_random(dev->netdev);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-11 17:30 ` [PATCH v2] " Anant Thazhemadam
@ 2020-10-11 17:59   ` Jakub Kicinski
  2020-10-11 18:33     ` Joe Perches
  2020-10-11 22:14   ` Stephen Rothwell
  1 sibling, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2020-10-11 17:59 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: petkan, davem, linux-usb, netdev, linux-kernel,
	linux-kernel-mentees, linux-next, sfr

On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> that was read must be copied over. Otherwise, a random ethernet address
> must be assigned.
> 
> get_registers() returns 0 if successful, and negative error number
> otherwise. However, in set_ethernet_addr(), this return value is
> incorrectly checked.
> 
> Since this return value will never be equal to sizeof(node_id), a
> random MAC address will always be generated and assigned to the
> device; even in cases when get_registers() is successful.
> 
> Correctly modifying the condition that checks if get_registers() was
> successful or not fixes this problem, and copies the ethernet address
> appropriately.
> 
> Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>

This patch is a fix to a conflict resolution in linux-next.

linux-next is not a "real" tree, it's an integration tree used to
figure out conflicts early.

We had let Stephen know about the problem already. Please wait one
week, and if the problem is still present resend this.

Thank you.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-11 17:59   ` Jakub Kicinski
@ 2020-10-11 18:33     ` Joe Perches
  2020-10-11 19:31       ` Petko Manolov
  0 siblings, 1 reply; 16+ messages in thread
From: Joe Perches @ 2020-10-11 18:33 UTC (permalink / raw)
  To: Jakub Kicinski, Anant Thazhemadam
  Cc: petkan, davem, linux-usb, netdev, linux-kernel,
	linux-kernel-mentees, linux-next, sfr

On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote:
> On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > that was read must be copied over. Otherwise, a random ethernet address
> > must be assigned.
> > 
> > get_registers() returns 0 if successful, and negative error number
> > otherwise. However, in set_ethernet_addr(), this return value is
> > incorrectly checked.
> > 
> > Since this return value will never be equal to sizeof(node_id), a
> > random MAC address will always be generated and assigned to the
> > device; even in cases when get_registers() is successful.
> > 
> > Correctly modifying the condition that checks if get_registers() was
> > successful or not fixes this problem, and copies the ethernet address
> > appropriately.

There are many unchecked uses of set_registers and get_registers
 in this file.

If failures are really expected, then it might be better to fix
them up too.

$ git grep -w '[gs]et_registers' drivers/net/usb/rtl8150.c
drivers/net/usb/rtl8150.c:static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
drivers/net/usb/rtl8150.c:static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
drivers/net/usb/rtl8150.c:              get_registers(dev, PHYDAT, 2, data);
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
drivers/net/usb/rtl8150.c:      ret = get_registers(dev, IDR, sizeof(node_id), node_id);
drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, netdev->addr_len, netdev->dev_addr);
drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:              set_registers(dev, IDR_EEPROM + (i * 2), 2,
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &data);
drivers/net/usb/rtl8150.c:              get_registers(dev, CR, 1, &data);
drivers/net/usb/rtl8150.c:      set_registers(dev, RCR, 1, &rcr);
drivers/net/usb/rtl8150.c:      set_registers(dev, TCR, 1, &tcr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      get_registers(dev, MSR, 1, &msr);
drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      get_registers(dev, CSCR, 2, &tmp);
drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, 6, netdev->dev_addr);
drivers/net/usb/rtl8150.c:      get_registers(dev, BMCR, 2, &bmcr);
drivers/net/usb/rtl8150.c:      get_registers(dev, ANLP, 2, &lpa);



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-11 18:33     ` Joe Perches
@ 2020-10-11 19:31       ` Petko Manolov
  2020-10-11 20:14         ` Joe Perches
  0 siblings, 1 reply; 16+ messages in thread
From: Petko Manolov @ 2020-10-11 19:31 UTC (permalink / raw)
  To: Joe Perches
  Cc: Jakub Kicinski, Anant Thazhemadam, davem, linux-usb, netdev,
	linux-kernel, linux-kernel-mentees, linux-next, sfr

On 20-10-11 11:33:00, Joe Perches wrote:
> On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote:
> > On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> > > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > > that was read must be copied over. Otherwise, a random ethernet address
> > > must be assigned.
> > > 
> > > get_registers() returns 0 if successful, and negative error number
> > > otherwise. However, in set_ethernet_addr(), this return value is
> > > incorrectly checked.
> > > 
> > > Since this return value will never be equal to sizeof(node_id), a
> > > random MAC address will always be generated and assigned to the
> > > device; even in cases when get_registers() is successful.
> > > 
> > > Correctly modifying the condition that checks if get_registers() was
> > > successful or not fixes this problem, and copies the ethernet address
> > > appropriately.
> 
> There are many unchecked uses of set_registers and get_registers
>  in this file.
> 
> If failures are really expected, then it might be better to fix
> them up too.

Checking the return value of each get/set_registers() is going to be a PITA and
not very helpful.  Doing so when setting the MAC address _does_ make sense as in
that case it is not a hard error.

In almost all other occasions if usb_control_msg_send/recv() return an error i'd
rather dump an error message (from within get/set_registers()) and let the user
decide whether to get rid of this adapter or start debugging it.


cheers,
Petko


> $ git grep -w '[gs]et_registers' drivers/net/usb/rtl8150.c
> drivers/net/usb/rtl8150.c:static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> drivers/net/usb/rtl8150.c:static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
> drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
> drivers/net/usb/rtl8150.c:              get_registers(dev, PHYDAT, 2, data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
> drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
> drivers/net/usb/rtl8150.c:      ret = get_registers(dev, IDR, sizeof(node_id), node_id);
> drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, netdev->addr_len, netdev->dev_addr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:              set_registers(dev, IDR_EEPROM + (i * 2), 2,
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &data);
> drivers/net/usb/rtl8150.c:              get_registers(dev, CR, 1, &data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, RCR, 1, &rcr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, TCR, 1, &tcr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, MSR, 1, &msr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, CSCR, 2, &tmp);
> drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, 6, netdev->dev_addr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, BMCR, 2, &bmcr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, ANLP, 2, &lpa);
> 
> 
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-11 19:31       ` Petko Manolov
@ 2020-10-11 20:14         ` Joe Perches
  0 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2020-10-11 20:14 UTC (permalink / raw)
  To: Petko Manolov
  Cc: Jakub Kicinski, Anant Thazhemadam, davem, linux-usb, netdev,
	linux-kernel, linux-kernel-mentees, linux-next, sfr

On Sun, 2020-10-11 at 22:31 +0300, Petko Manolov wrote:
> On 20-10-11 11:33:00, Joe Perches wrote:
> > On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote:
> > > On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> > > > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > > > that was read must be copied over. Otherwise, a random ethernet address
> > > > must be assigned.
> > > > 
> > > > get_registers() returns 0 if successful, and negative error number
> > > > otherwise. However, in set_ethernet_addr(), this return value is
> > > > incorrectly checked.
> > > > 
> > > > Since this return value will never be equal to sizeof(node_id), a
> > > > random MAC address will always be generated and assigned to the
> > > > device; even in cases when get_registers() is successful.
> > > > 
> > > > Correctly modifying the condition that checks if get_registers() was
> > > > successful or not fixes this problem, and copies the ethernet address
> > > > appropriately.
> > 
> > There are many unchecked uses of set_registers and get_registers
> >  in this file.
> > 
> > If failures are really expected, then it might be better to fix
> > them up too.
> 
> Checking the return value of each get/set_registers() is going to be a PITA and
> not very helpful.  Doing so when setting the MAC address _does_ make sense as in
> that case it is not a hard error.
> 
> In almost all other occasions if usb_control_msg_send/recv() return an error i'd
> rather dump an error message (from within get/set_registers()) and let the user
> decide whether to get rid of this adapter or start debugging it.

Your code, your choices...

Consider using _once or _ratelimited output too.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-11 17:30 ` [PATCH v2] " Anant Thazhemadam
  2020-10-11 17:59   ` Jakub Kicinski
@ 2020-10-11 22:14   ` Stephen Rothwell
  2020-10-15 21:59     ` Stephen Rothwell
  1 sibling, 1 reply; 16+ messages in thread
From: Stephen Rothwell @ 2020-10-11 22:14 UTC (permalink / raw)
  To: Greg KH
  Cc: Anant Thazhemadam, petkan, davem, kuba, linux-usb, netdev,
	linux-kernel, linux-kernel-mentees, linux-next, Jakub Kicinski

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

Hi Greg,

On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam <anant.thazhemadam@gmail.com> wrote:
>
> In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> that was read must be copied over. Otherwise, a random ethernet address
> must be assigned.
> 
> get_registers() returns 0 if successful, and negative error number
> otherwise. However, in set_ethernet_addr(), this return value is
> incorrectly checked.
> 
> Since this return value will never be equal to sizeof(node_id), a
> random MAC address will always be generated and assigned to the
> device; even in cases when get_registers() is successful.
> 
> Correctly modifying the condition that checks if get_registers() was
> successful or not fixes this problem, and copies the ethernet address
> appropriately.
> 
> Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
> Changes in v2:
>         * Fixed the format of the Fixes tag
>         * Modified the commit message to better describe the issue being 
>           fixed
> 
> +CCing Stephen and linux-next, since the commit fixed isn't in the networking
> tree, but is present in linux-next.
> 
>  drivers/net/usb/rtl8150.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index f020401adf04..bf8a60533f3e 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
>  
>  	ret = get_registers(dev, IDR, sizeof(node_id), node_id);
>  
> -	if (ret == sizeof(node_id)) {
> +	if (!ret) {
>  		ether_addr_copy(dev->netdev->dev_addr, node_id);
>  	} else {
>  		eth_hw_addr_random(dev->netdev);
> -- 
> 2.25.1
> 

I will apply the above patch to the merge of the usb tree today to fix
up a semantic conflict between the usb tree and Linus' tree.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-11 22:14   ` Stephen Rothwell
@ 2020-10-15 21:59     ` Stephen Rothwell
  2020-10-15 22:24       ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Stephen Rothwell @ 2020-10-15 21:59 UTC (permalink / raw)
  To: Greg KH
  Cc: Anant Thazhemadam, petkan, davem, kuba, linux-usb, netdev,
	linux-kernel, linux-kernel-mentees, linux-next

[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

Hi Greg,

On Mon, 12 Oct 2020 09:14:28 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam <anant.thazhemadam@gmail.com> wrote:
> >
> > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > that was read must be copied over. Otherwise, a random ethernet address
> > must be assigned.
> > 
> > get_registers() returns 0 if successful, and negative error number
> > otherwise. However, in set_ethernet_addr(), this return value is
> > incorrectly checked.
> > 
> > Since this return value will never be equal to sizeof(node_id), a
> > random MAC address will always be generated and assigned to the
> > device; even in cases when get_registers() is successful.
> > 
> > Correctly modifying the condition that checks if get_registers() was
> > successful or not fixes this problem, and copies the ethernet address
> > appropriately.
> > 
> > Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
> > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> > ---
> > Changes in v2:
> >         * Fixed the format of the Fixes tag
> >         * Modified the commit message to better describe the issue being 
> >           fixed
> > 
> > +CCing Stephen and linux-next, since the commit fixed isn't in the networking
> > tree, but is present in linux-next.
> > 
> >  drivers/net/usb/rtl8150.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index f020401adf04..bf8a60533f3e 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
> >  
> >  	ret = get_registers(dev, IDR, sizeof(node_id), node_id);
> >  
> > -	if (ret == sizeof(node_id)) {
> > +	if (!ret) {
> >  		ether_addr_copy(dev->netdev->dev_addr, node_id);
> >  	} else {
> >  		eth_hw_addr_random(dev->netdev);
> > -- 
> > 2.25.1
> >   
> 
> I will apply the above patch to the merge of the usb tree today to fix
> up a semantic conflict between the usb tree and Linus' tree.

It looks like you forgot to mention this one to Linus :-(

It should probably say:

Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-15 21:59     ` Stephen Rothwell
@ 2020-10-15 22:24       ` Jakub Kicinski
  2020-10-15 22:37         ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2020-10-15 22:24 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Greg KH, Anant Thazhemadam, petkan, davem, linux-usb, netdev,
	linux-kernel, linux-kernel-mentees, linux-next

On Fri, 16 Oct 2020 08:59:22 +1100 Stephen Rothwell wrote:
> > I will apply the above patch to the merge of the usb tree today to fix
> > up a semantic conflict between the usb tree and Linus' tree.  
> 
> It looks like you forgot to mention this one to Linus :-(
> 
> It should probably say:
> 
> Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")

Umpf. I think Greg sent his changes first, so this is on me.

The networking PR is still outstanding, can we reply to the PR with
the merge guidance. Or is it too late?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-15 22:24       ` Jakub Kicinski
@ 2020-10-15 22:37         ` Jakub Kicinski
  2020-10-18 19:54           ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2020-10-15 22:37 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Greg KH, Anant Thazhemadam, petkan, davem, linux-usb, netdev,
	linux-kernel, linux-kernel-mentees, linux-next

On Thu, 15 Oct 2020 15:24:51 -0700 Jakub Kicinski wrote:
> On Fri, 16 Oct 2020 08:59:22 +1100 Stephen Rothwell wrote:
> > > I will apply the above patch to the merge of the usb tree today to fix
> > > up a semantic conflict between the usb tree and Linus' tree.    
> > 
> > It looks like you forgot to mention this one to Linus :-(
> > 
> > It should probably say:
> > 
> > Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")  
> 
> Umpf. I think Greg sent his changes first, so this is on me.
> 
> The networking PR is still outstanding, can we reply to the PR with
> the merge guidance. Or is it too late?

I take that back, this came through net, not net-next so our current
PR is irrelevant :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses
  2020-10-15 22:37         ` Jakub Kicinski
@ 2020-10-18 19:54           ` Jakub Kicinski
  0 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2020-10-18 19:54 UTC (permalink / raw)
  To: Greg KH
  Cc: Stephen Rothwell, Anant Thazhemadam, petkan, davem, linux-usb,
	netdev, linux-kernel, linux-kernel-mentees, linux-next

On Thu, 15 Oct 2020 15:37:00 -0700 Jakub Kicinski wrote:
> On Thu, 15 Oct 2020 15:24:51 -0700 Jakub Kicinski wrote:
> > On Fri, 16 Oct 2020 08:59:22 +1100 Stephen Rothwell wrote:  
> > > > I will apply the above patch to the merge of the usb tree today to fix
> > > > up a semantic conflict between the usb tree and Linus' tree.      
> > > 
> > > It looks like you forgot to mention this one to Linus :-(
> > > 
> > > It should probably say:
> > > 
> > > Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")    
> > 
> > Umpf. I think Greg sent his changes first, so this is on me.
> > 
> > The networking PR is still outstanding, can we reply to the PR with
> > the merge guidance. Or is it too late?  
> 
> I take that back, this came through net, not net-next so our current
> PR is irrelevant :)

Looks like the best thing to do right now is to put this fix in net,
so let me do just that (with the Fixes tag from Stephen).

Thanks Anant!

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-10-18 19:54 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10  6:44 [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses Anant Thazhemadam
2020-10-10 16:59 ` Jakub Kicinski
2020-10-10 18:04   ` Anant Thazhemadam
2020-10-10 18:16     ` Jakub Kicinski
2020-10-10 18:44       ` Anant Thazhemadam
2020-10-10 19:20         ` Jakub Kicinski
2020-10-11 17:30 ` [PATCH v2] " Anant Thazhemadam
2020-10-11 17:59   ` Jakub Kicinski
2020-10-11 18:33     ` Joe Perches
2020-10-11 19:31       ` Petko Manolov
2020-10-11 20:14         ` Joe Perches
2020-10-11 22:14   ` Stephen Rothwell
2020-10-15 21:59     ` Stephen Rothwell
2020-10-15 22:24       ` Jakub Kicinski
2020-10-15 22:37         ` Jakub Kicinski
2020-10-18 19:54           ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).