| Message ID | 20201010064459.6563-1-anant.thazhemadam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: usb: rtl8150: don't incorrectly assign random MAC addresses | expand |
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.
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
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.
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
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.
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);
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(-)