Linux Kernel Mentees Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [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
  2020-10-13  0:14 ` kernel test robot
  0 siblings, 2 replies; 4+ messages in thread
From: Anant Thazhemadam @ 2020-10-10  6:56 UTC (permalink / raw)
  Cc: Anant Thazhemadam, linux-usb, netdev, Oliver Neukum,
	linux-kernel, Jakub Kicinski, linux-kernel-mentees,
	David S. Miller

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

_______________________________________________
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] [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-10  6:56 [Linux-kernel-mentees] [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
  1 sibling, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2020-10-10 17:03 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: linux-usb, netdev, Oliver Neukum, linux-kernel,
	linux-kernel-mentees, David S. Miller

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

You haven't even built this, let alone tested :/
_______________________________________________
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] [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; 4+ messages in thread
From: Anant Thazhemadam @ 2020-10-10 17:48 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: linux-usb, netdev, Oliver Neukum, linux-kernel,
	linux-kernel-mentees, David S. Miller


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
_______________________________________________
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] [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API
  2020-10-10  6:56 [Linux-kernel-mentees] [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
  1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2020-10-13  0:14 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: Anant Thazhemadam, linux-usb, kbuild-all, netdev, Oliver Neukum,
	linux-kernel, clang-built-linux, Jakub Kicinski,
	linux-kernel-mentees


[-- 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 --]

[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
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, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10  6:56 [Linux-kernel-mentees] [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

Linux Kernel Mentees Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kernel-mentees/0 linux-kernel-mentees/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-kernel-mentees linux-kernel-mentees/ https://lore.kernel.org/linux-kernel-mentees \
		linux-kernel-mentees@lists.linuxfoundation.org linux-kernel-mentees@lists.linux-foundation.org
	public-inbox-index linux-kernel-mentees

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxfoundation.lists.linux-kernel-mentees


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