Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: usb: usbnet: update  __usbnet_{read|write}_cmd() to use new API
@ 2020-10-10  6:56 Anant Thazhemadam
  2020-10-10 17:03 ` Jakub Kicinski
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Anant Thazhemadam @ 2020-10-10  6:56 UTC (permalink / raw)
  Cc: linux-kernel-mentees, Anant Thazhemadam, Oliver Neukum,
	David S. Miller, Jakub Kicinski, netdev, linux-usb, 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>
---
Since not all callers of usbnet_{read|write}_cmd() check if a complete 
read/write happened, partial reads can go unnoticed.

This issue was briefly mentioned here.
	https://lore.kernel.org/linux-usb/1565777764.25764.4.camel@suse.com/

Using the new API in place of the old one doesn't break anything.
This is mainly because usb_control_msg_{send|recv}() returns 0 on success
and a negative error number on failure (which includes partial reads/writes).

Thus, the error checking condition provided by the present callers of 
usbnet_{read|write}_cmd() for failure (return value < 0 is considered as an 
error) will hold. 
And similarly, the condition checked by some callers for 'success' 
(return value >= 0 && return value < length/size) will also hold.

However, if I have missed out on any caller that this might cause problems with,
please let me know, and I will fix that up as well.

 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..dd9fe530a374 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, GPF_KERNEL);
 }
 
 /*
-- 
2.25.1


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

* Re: [PATCH] net: usb: usbnet: update  __usbnet_{read|write}_cmd() to use new API
  2020-10-10  6:56 [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API Anant Thazhemadam
@ 2020-10-10 17:03 ` Jakub Kicinski
  2020-10-10 17:48   ` Anant Thazhemadam
  2020-10-13  0:14 ` kernel test robot
  2020-10-29 13:22 ` [PATCH v2] " Anant Thazhemadam
  2 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2020-10-10 17:03 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: linux-kernel-mentees, Oliver Neukum, David S. Miller, netdev,
	linux-usb, linux-kernel

On Sat, 10 Oct 2020 12:26:23 +0530 Anant Thazhemadam wrote:
> GPF_KERNEL

You haven't even built this, let alone tested :/

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

* Re: [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-10 17:03 ` Jakub Kicinski
@ 2020-10-10 17:48   ` Anant Thazhemadam
  0 siblings, 0 replies; 12+ messages in thread
From: Anant Thazhemadam @ 2020-10-10 17:48 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-kernel-mentees, Oliver Neukum, David S. Miller, netdev,
	linux-usb, linux-kernel


On 10/10/20 10:33 pm, Jakub Kicinski wrote:
> On Sat, 10 Oct 2020 12:26:23 +0530 Anant Thazhemadam wrote:
>> GPF_KERNEL
> You haven't even built this, let alone tested :/

I'm really sorry about this.
Turns out, my .config wasn't set generated by make allyesconfig, and thus
this regression went undetected.

I can submit a v2 that doesn't break the build, and which is build tested
properly.
However, I'm not clear on how else I could locally run unit tests pertaining to this
patch. I understand the critical requirement for testing changes and would really
appreciate it if someone could direct me towards some resource or another that
could help me figure that out too.

Once again, I'm really sorry for this oversight.
Thank you for your time.

Thanks,
Anant

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

* Re: [PATCH] net: usb: usbnet: update  __usbnet_{read|write}_cmd() to use new API
  2020-10-10  6:56 [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API Anant Thazhemadam
  2020-10-10 17:03 ` Jakub Kicinski
@ 2020-10-13  0:14 ` kernel test robot
  2020-10-29 13:22 ` [PATCH v2] " Anant Thazhemadam
  2 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2020-10-13  0:14 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: kbuild-all, clang-built-linux, linux-kernel-mentees,
	Anant Thazhemadam, Oliver Neukum, Jakub Kicinski, netdev,
	linux-usb, linux-kernel


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

Hi Anant,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.9 next-20201012]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anant-Thazhemadam/net-usb-usbnet-update-__usbnet_-read-write-_cmd-to-use-new-API/20201010-145950
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 6f2f486d57c4d562cdf4932320b66fbb878ab1c4
config: x86_64-randconfig-a005-20201012 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e72d3eaf38f217698f72cb8fdc969a6e72dad3a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/77da62a1a1fcc73c3d8309c273dfd4d497918c6e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anant-Thazhemadam/net-usb-usbnet-update-__usbnet_-read-write-_cmd-to-use-new-API/20201010-145950
        git checkout 77da62a1a1fcc73c3d8309c273dfd4d497918c6e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/usb/usbnet.c:1990:9: error: implicit declaration of function 'usb_control_msg_recv' [-Werror,-Wimplicit-function-declaration]
           return usb_control_msg_recv(dev->udev, 0,
                  ^
   drivers/net/usb/usbnet.c:1990:9: note: did you mean 'usb_control_msg'?
   include/linux/usb.h:1794:12: note: 'usb_control_msg' declared here
   extern int usb_control_msg(struct usb_device *dev, unsigned int pipe,
              ^
   drivers/net/usb/usbnet.c:2008:9: error: implicit declaration of function 'usb_control_msg_send' [-Werror,-Wimplicit-function-declaration]
           return usb_control_msg_send(dev->udev, 0,
                  ^
   drivers/net/usb/usbnet.c:2008:9: note: did you mean 'usb_control_msg'?
   include/linux/usb.h:1794:12: note: 'usb_control_msg' declared here
   extern int usb_control_msg(struct usb_device *dev, unsigned int pipe,
              ^
>> drivers/net/usb/usbnet.c:2010:26: error: use of undeclared identifier 'GPF_KERNEL'
                           USB_CTRL_SET_TIMEOUT, GPF_KERNEL);
                                                 ^
   3 errors generated.

vim +/GPF_KERNEL +2010 drivers/net/usb/usbnet.c

  1994	
  1995	static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
  1996				      u16 value, u16 index, const void *data,
  1997				      u16 size)
  1998	{
  1999		netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
  2000			   " value=0x%04x index=0x%04x size=%d\n",
  2001			   cmd, reqtype, value, index, size);
  2002	
  2003		if (size && !data) {
  2004			WARN_ON_ONCE(1);
  2005			return -EINVAL;
  2006		}
  2007	
  2008		return usb_control_msg_send(dev->udev, 0,
  2009				cmd, reqtype, value, index, data, size,
> 2010				USB_CTRL_SET_TIMEOUT, GPF_KERNEL);
  2011	}
  2012	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37241 bytes --]

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

* [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-10  6:56 [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API Anant Thazhemadam
  2020-10-10 17:03 ` Jakub Kicinski
  2020-10-13  0:14 ` kernel test robot
@ 2020-10-29 13:22 ` Anant Thazhemadam
  2020-10-29 15:16   ` Anant Thazhemadam
                     ` (2 more replies)
  2 siblings, 3 replies; 12+ messages in thread
From: Anant Thazhemadam @ 2020-10-29 13:22 UTC (permalink / raw)
  To: Oliver Neukum, David S . Miller, Jakub Kicinski
  Cc: netdev, linux-usb, linux-kernel, linux-kernel-mentees, Anant Thazhemadam

Currently, __usbnet_{read|write}_cmd() use usb_control_msg(),
and thus consider potential partial reads/writes being done to 
be perfectly valid.
Quite a few callers of usbnet_{read|write}_cmd() don't enforce
checking for partial reads/writes into account either, automatically
assuming that a complete read/write occurs.

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 v2:
	* Fix build error

This patch has been compile and build tested with a .config file that
was generated using make allyesconfig, and the build error has been 
fixed.
Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing,
and would appreciate it if someone could do that.

 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..2f7c7b7f4047 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


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

* Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-29 13:22 ` [PATCH v2] " Anant Thazhemadam
@ 2020-10-29 15:16   ` Anant Thazhemadam
  2020-10-29 16:30     ` Alan Stern
  2020-10-31 21:11   ` Jakub Kicinski
  2020-10-31 21:35   ` [PATCH v3] " Anant Thazhemadam
  2 siblings, 1 reply; 12+ messages in thread
From: Anant Thazhemadam @ 2020-10-29 15:16 UTC (permalink / raw)
  To: Oliver Neukum, David S . Miller, Jakub Kicinski
  Cc: netdev, linux-usb, LKML, linux-kernel-mentees


On 29/10/20 6:52 pm, Anant Thazhemadam wrote:
> Currently, __usbnet_{read|write}_cmd() use usb_control_msg(),
> and thus consider potential partial reads/writes being done to 
> be perfectly valid.
> Quite a few callers of usbnet_{read|write}_cmd() don't enforce
> checking for partial reads/writes into account either, automatically
> assuming that a complete read/write occurs.
>
> 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> <mailto:anant.thazhemadam@gmail.com>
> ---
> Changes in v2:
> 	* Fix build error
>
> This patch has been compile and build tested with a .config file that
> was generated using make allyesconfig, and the build error has been 
> fixed.
> Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing,
> and would appreciate it if someone could do that.
>
>  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..2f7c7b7f4047 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);
>  }
>  
>  /*

I had a v2 prepared and ready but was told to wait for a week before sending it in,
since usb_control_msg_{send|recv}() that were being used were not present in the
networking tree at the time, and all the trees would be converged by then.
So, just to be on the safer side, I waited for two weeks.
I checked the net tree, and found the APIs there too (defined in usb.h).

However the build seems to fail here,
    https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/O2BERGN7SYYC6LNOOKNUGPS2IJLDWYT7/

I'm not entirely sure at this point why this is happening, and would appreciate it if
someone could take the time to tell me if and how this might be an issue with my
patch.

Thanks,
Anant


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

* Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-29 15:16   ` Anant Thazhemadam
@ 2020-10-29 16:30     ` Alan Stern
  0 siblings, 0 replies; 12+ messages in thread
From: Alan Stern @ 2020-10-29 16:30 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: Oliver Neukum, David S . Miller, Jakub Kicinski, netdev,
	linux-usb, LKML, linux-kernel-mentees

On Thu, Oct 29, 2020 at 08:46:59PM +0530, Anant Thazhemadam wrote:
> I had a v2 prepared and ready but was told to wait for a week before sending it in,
> since usb_control_msg_{send|recv}() that were being used were not present in the
> networking tree at the time, and all the trees would be converged by then.
> So, just to be on the safer side, I waited for two weeks.
> I checked the net tree, and found the APIs there too (defined in usb.h).
> 
> However the build seems to fail here,
>     https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/O2BERGN7SYYC6LNOOKNUGPS2IJLDWYT7/
> 
> I'm not entirely sure at this point why this is happening, and would appreciate it if
> someone could take the time to tell me if and how this might be an issue with my
> patch.

It's happening because the tree you used for building did not include 
the declarations of usb_control_msg_{send|recv}().

It's hard to tell exactly what that tree does contain, because the 
github.com links embedded in the web page you mention above don't work.

Alan Stern

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

* Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-29 13:22 ` [PATCH v2] " Anant Thazhemadam
  2020-10-29 15:16   ` Anant Thazhemadam
@ 2020-10-31 21:11   ` Jakub Kicinski
  2020-10-31 21:31     ` Anant Thazhemadam
  2020-10-31 21:35   ` [PATCH v3] " Anant Thazhemadam
  2 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2020-10-31 21:11 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: Oliver Neukum, David S . Miller, netdev, linux-usb, linux-kernel,
	linux-kernel-mentees

On Thu, 29 Oct 2020 18:52:56 +0530 Anant Thazhemadam wrote:
> +	return usb_control_msg_recv(dev->udev, 0,
> +			      cmd, reqtype, value, index, data, size,
> +			      USB_CTRL_GET_TIMEOUT, GFP_KERNEL);

Please align continuation lines after the opening bracket.

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

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


On 01/11/20 2:41 am, Jakub Kicinski wrote:
> On Thu, 29 Oct 2020 18:52:56 +0530 Anant Thazhemadam wrote:
>> +	return usb_control_msg_recv(dev->udev, 0,
>> +			      cmd, reqtype, value, index, data, size,
>> +			      USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
> Please align continuation lines after the opening bracket.

I will do that, and send in a v3 right away.

Thanks,
Anant


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

* [PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-29 13:22 ` [PATCH v2] " Anant Thazhemadam
  2020-10-29 15:16   ` Anant Thazhemadam
  2020-10-31 21:11   ` Jakub Kicinski
@ 2020-10-31 21:35   ` Anant Thazhemadam
  2020-11-02  9:40     ` Oliver Neukum
  2 siblings, 1 reply; 12+ messages in thread
From: Anant Thazhemadam @ 2020-10-31 21:35 UTC (permalink / raw)
  To: Oliver Neukum, David S . Miller, Jakub Kicinski
  Cc: netdev, linux-usb, linux-kernel, linux-kernel-mentees, Anant Thazhemadam

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


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

* Re: [PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-31 21:35   ` [PATCH v3] " Anant Thazhemadam
@ 2020-11-02  9:40     ` Oliver Neukum
  2020-11-02 16:40       ` Anant Thazhemadam
  0 siblings, 1 reply; 12+ messages in thread
From: Oliver Neukum @ 2020-11-02  9:40 UTC (permalink / raw)
  To: Anant Thazhemadam, David S . Miller, Jakub Kicinski
  Cc: netdev, linux-usb, linux-kernel, linux-kernel-mentees

Am Sonntag, den 01.11.2020, 03:05 +0530 schrieb Anant Thazhemadam:
> 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.
> 

Hi,

plesae send this as a post of its own. We cannot take a new set
as a reply to an older set.

	Regards
		Oliver



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

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


On 02/11/20 3:10 pm, Oliver Neukum wrote:
> Am Sonntag, den 01.11.2020, 03:05 +0530 schrieb Anant Thazhemadam:
>> 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.
>>
> Hi,
>
> plesae send this as a post of its own. We cannot take a new set
> as a reply to an older set.
>
> 	Regards
> 		Oliver
>

Got it. I will do that.

Thanks,
Anant

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

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10  6:56 [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API Anant Thazhemadam
2020-10-10 17:03 ` Jakub Kicinski
2020-10-10 17:48   ` Anant Thazhemadam
2020-10-13  0:14 ` kernel test robot
2020-10-29 13:22 ` [PATCH v2] " Anant Thazhemadam
2020-10-29 15:16   ` Anant Thazhemadam
2020-10-29 16:30     ` Alan Stern
2020-10-31 21:11   ` Jakub Kicinski
2020-10-31 21:31     ` Anant Thazhemadam
2020-10-31 21:35   ` [PATCH v3] " Anant Thazhemadam
2020-11-02  9:40     ` Oliver Neukum
2020-11-02 16:40       ` Anant Thazhemadam

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