Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Anant Thazhemadam <anant.thazhemadam@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: linux-kernel-mentees@lists.linuxfoundation.org,
	Petko Manolov <petkan@nucleusys.com>,
	"David S. Miller" <davem@davemloft.net>,
	linux-usb@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses
Date: Sun, 11 Oct 2020 00:14:05 +0530
Message-ID: <e772b9f0-f5cd-c50b-86a7-fde22b6e13e3@gmail.com> (raw)
In-Reply-To: <20201010111645.334647af@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>


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



  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-10  6:44 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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e772b9f0-f5cd-c50b-86a7-fde22b6e13e3@gmail.com \
    --to=anant.thazhemadam@gmail.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=petkan@nucleusys.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git