linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
@ 2020-11-02 17:39 Anant Thazhemadam
  2020-11-05  0:24 ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Anant Thazhemadam @ 2020-11-02 17:39 UTC (permalink / raw)
  To: Oliver Neukum, David S . Miller, Jakub Kicinski
  Cc: netdev, Anant Thazhemadam, linux-usb, linux-kernel-mentees, linux-kernel

Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
However, this could lead to potential partial reads/writes being
considered valid, and since most of the callers of
usbnet_{read|write}_cmd() don't take partial reads/writes into account
(only checking for negative error number is done), and this can lead to
issues.

However, the new usb_control_msg_{send|recv}() APIs don't allow partial
reads and writes.
Using the new APIs also relaxes the return value checking that must
be done after usbnet_{read|write}_cmd() is called.

Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
Changes in v3:
	* Aligned continuation lines after the opening brackets
Changes in v2:
	* Fix build error


 drivers/net/usb/usbnet.c | 52 ++++++++--------------------------------
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index bf6c58240bd4..b2df3417a41c 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
 static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 			     u16 value, u16 index, void *data, u16 size)
 {
-	void *buf = NULL;
-	int err = -ENOMEM;
 
 	netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
 		   " value=0x%04x index=0x%04x size=%d\n",
 		   cmd, reqtype, value, index, size);
 
-	if (size) {
-		buf = kmalloc(size, GFP_KERNEL);
-		if (!buf)
-			goto out;
-	}
-
-	err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
-			      cmd, reqtype, value, index, buf, size,
-			      USB_CTRL_GET_TIMEOUT);
-	if (err > 0 && err <= size) {
-        if (data)
-            memcpy(data, buf, err);
-        else
-            netdev_dbg(dev->net,
-                "Huh? Data requested but thrown away.\n");
-    }
-	kfree(buf);
-out:
-	return err;
+	return usb_control_msg_recv(dev->udev, 0,
+				    cmd, reqtype, value, index, data, size,
+				    USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 }
 
 static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 			      u16 value, u16 index, const void *data,
 			      u16 size)
 {
-	void *buf = NULL;
-	int err = -ENOMEM;
-
 	netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
 		   " value=0x%04x index=0x%04x size=%d\n",
 		   cmd, reqtype, value, index, size);
 
-	if (data) {
-		buf = kmemdup(data, size, GFP_KERNEL);
-		if (!buf)
-			goto out;
-	} else {
-        if (size) {
-            WARN_ON_ONCE(1);
-            err = -EINVAL;
-            goto out;
-        }
-    }
-
-	err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-			      cmd, reqtype, value, index, buf, size,
-			      USB_CTRL_SET_TIMEOUT);
-	kfree(buf);
+	if (size && !data) {
+		WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
 
-out:
-	return err;
+	return usb_control_msg_send(dev->udev, 0,
+				    cmd, reqtype, value, index, data, size,
+				    USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
 }
 
 /*
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-11-02 17:39 [Linux-kernel-mentees] [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API Anant Thazhemadam
@ 2020-11-05  0:24 ` Jakub Kicinski
  2020-11-05  2:26   ` Anant Thazhemadam
  0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2020-11-05  0:24 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: linux-usb, netdev, Oliver Neukum, linux-kernel,
	linux-kernel-mentees, David S . Miller

On Mon,  2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote:
> Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
> However, this could lead to potential partial reads/writes being
> considered valid, and since most of the callers of
> usbnet_{read|write}_cmd() don't take partial reads/writes into account
> (only checking for negative error number is done), and this can lead to
> issues.
> 
> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
> reads and writes.
> Using the new APIs also relaxes the return value checking that must
> be done after usbnet_{read|write}_cmd() is called.
> 
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>

So you're changing the semantics without updating the callers?

I'm confused. 

Is this supposed to be applied to some tree which already has the
callers fixed?

At a quick scan at least drivers/net/usb/plusb.c* would get confused 
as it compares the return value to zero and 0 used to mean "nothing
transferred", now it means "all good", no? 

* I haven't looked at all the other callers
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-11-05  0:24 ` Jakub Kicinski
@ 2020-11-05  2:26   ` Anant Thazhemadam
  2020-11-05 16:14     ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Anant Thazhemadam @ 2020-11-05  2:26 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-usb, netdev, Oliver Neukum, linux-kernel,
	linux-kernel-mentees, David S . Miller


On 05/11/20 5:54 am, Jakub Kicinski wrote:
> On Mon,  2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote:
>> Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
>> However, this could lead to potential partial reads/writes being
>> considered valid, and since most of the callers of
>> usbnet_{read|write}_cmd() don't take partial reads/writes into account
>> (only checking for negative error number is done), and this can lead to
>> issues.
>>
>> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
>> reads and writes.
>> Using the new APIs also relaxes the return value checking that must
>> be done after usbnet_{read|write}_cmd() is called.
>>
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> So you're changing the semantics without updating the callers?
>
> I'm confused. 
>
> Is this supposed to be applied to some tree which already has the
> callers fixed?
>
> At a quick scan at least drivers/net/usb/plusb.c* would get confused 
> as it compares the return value to zero and 0 used to mean "nothing
> transferred", now it means "all good", no? 
>
> * I haven't looked at all the other callers

I see. I checked most of the callers that directly called the functions,
but it seems to have slipped my mind that these callers were also
wrappers, and to check the callers for these wrapper.
I apologize for the oversight.
I'll perform a more in-depth analysis of all the callers, fix this mistake,
and send in a patch series instead, that update all the callers too.
Would that be alright?

Thank you for your time.

Thanks,
Anant
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

* Re: [Linux-kernel-mentees] [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-11-05  2:26   ` Anant Thazhemadam
@ 2020-11-05 16:14     ` Jakub Kicinski
  0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2020-11-05 16:14 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: linux-usb, netdev, Oliver Neukum, linux-kernel,
	linux-kernel-mentees, David S . Miller

On Thu, 5 Nov 2020 07:56:08 +0530 Anant Thazhemadam wrote:
> On 05/11/20 5:54 am, Jakub Kicinski wrote:
> > On Mon,  2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote:  
> >> Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
> >> However, this could lead to potential partial reads/writes being
> >> considered valid, and since most of the callers of
> >> usbnet_{read|write}_cmd() don't take partial reads/writes into account
> >> (only checking for negative error number is done), and this can lead to
> >> issues.
> >>
> >> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
> >> reads and writes.
> >> Using the new APIs also relaxes the return value checking that must
> >> be done after usbnet_{read|write}_cmd() is called.
> >>
> >> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>  
> > So you're changing the semantics without updating the callers?
> >
> > I'm confused. 
> >
> > Is this supposed to be applied to some tree which already has the
> > callers fixed?
> >
> > At a quick scan at least drivers/net/usb/plusb.c* would get confused 
> > as it compares the return value to zero and 0 used to mean "nothing
> > transferred", now it means "all good", no? 
> >
> > * I haven't looked at all the other callers  
> 
> I see. I checked most of the callers that directly called the functions,
> but it seems to have slipped my mind that these callers were also
> wrappers, and to check the callers for these wrapper.
> I apologize for the oversight.
> I'll perform a more in-depth analysis of all the callers, fix this mistake,
> and send in a patch series instead, that update all the callers too.
> Would that be alright?

Yes. Probably best if you rename the existing function as first patch,
then add a new one with the old name using usb_control_msg_{send|recv}()
then switch the callers one by one, and finally remove the old renamed
function.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

end of thread, other threads:[~2020-11-05 16:14 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 17:39 [Linux-kernel-mentees] [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API Anant Thazhemadam
2020-11-05  0:24 ` Jakub Kicinski
2020-11-05  2:26   ` Anant Thazhemadam
2020-11-05 16:14     ` 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).