[Linux-kernel-mentees] [PATCH 0/4] net: usb: avoid using usb_control_msg() directly

[Linux-kernel-mentees] [PATCH 0/4] net: usb: avoid using usb_control_msg() directly

[Himadri Pandya]

A recent bug-fix shaded light on possible incorrect use of
usb_control_msg() without proper error check. This resulted in
introducing new usb core api wrapper functions usb_control_msg_send()
and usb_control_msg_recv() by Greg KH. This patch series continue the
clean-up using the new functions.

Himadri Pandya (4):
  net: usbnet: use usb_control_msg_recv() and usb_control_msg_send()
  net: sierra_net: use usb_control_msg_recv()
  net: usb: rtl8150: use usb_control_msg_recv() and
    usb_control_msg_send()
  net: rndis_host: use usb_control_msg_recv() and usb_control_msg_send()

 drivers/net/usb/rndis_host.c | 44 +++++++++++++---------------------
 drivers/net/usb/rtl8150.c    | 32 +++++--------------------
 drivers/net/usb/sierra_net.c | 17 ++++++-------
 drivers/net/usb/usbnet.c     | 46 ++++--------------------------------
 4 files changed, 34 insertions(+), 105 deletions(-)

-- 
2.17.1

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

[Linux-kernel-mentees] [PATCH 1/4] net: usbnet: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

Potential incorrect use of usb_control_msg() has resulted in new wrapper
functions to enforce its correct usage with proper error check. Hence
use these new wrapper functions instead of calling usb_control_msg()
directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/net/usb/usbnet.c | 46 ++++------------------------------------
 1 file changed, 4 insertions(+), 42 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 2b2a841cd938..a38a85bef46a 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1982,64 +1982,26 @@ 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,
+	return usb_control_msg_recv(dev->udev, 0,
+			      cmd, reqtype, value, index, data, 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;
 }
 
 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,
+	return usb_control_msg_send(dev->udev, 0,
+			      cmd, reqtype, value, index, data, size,
 			      USB_CTRL_SET_TIMEOUT);
-	kfree(buf);
-
-out:
-	return err;
 }
 
 /*
-- 
2.17.1

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

[Linux-kernel-mentees] [PATCH 2/4] net: sierra_net: use usb_control_msg_recv()

[Himadri Pandya]

The new usb api function usb_control_msg_recv() nicely wrapps
usb_control_msg() with proper error check. Hence use it instead of
directly calling usb_control_msg().

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/net/usb/sierra_net.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 0abd257b634c..f3a5f83cb256 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -478,16 +478,13 @@ static void sierra_net_kevent(struct work_struct *work)
 			return;
 
 		ifnum = priv->ifnum;
-		len = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
-				USB_CDC_GET_ENCAPSULATED_RESPONSE,
-				USB_DIR_IN|USB_TYPE_CLASS|USB_RECIP_INTERFACE,
-				0, ifnum, buf, SIERRA_NET_USBCTL_BUF_LEN,
-				USB_CTRL_SET_TIMEOUT);
-
-		if (len < 0) {
-			netdev_err(dev->net,
-				"usb_control_msg failed, status %d\n", len);
-		} else {
+		len = usb_control_msg_recv(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
+					   USB_CDC_GET_ENCAPSULATED_RESPONSE,
+					   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+					   0, ifnum, buf, SIERRA_NET_USBCTL_BUF_LEN,
+					   USB_CTRL_SET_TIMEOUT);
+
+		if (len) {
 			struct hip_hdr	hh;
 
 			dev_dbg(&dev->udev->dev, "%s: Received status message,"
-- 
2.17.1

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

[Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

Many usage of usb_control_msg() do not have proper error check on return
value leaving scope for bugs on short reads. New usb_control_msg_recv()
and usb_control_msg_send() nicely wraps usb_control_msg() with proper
error check. Hence use the wrappers instead of calling usb_control_msg()
directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/net/usb/rtl8150.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index 733f120c852b..e3002b675921 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150";
 */
 static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
 {
-	void *buf;
-	int ret;
-
-	buf = kmalloc(size, GFP_NOIO);
-	if (!buf)
-		return -ENOMEM;
-
-	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
-			      RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
-			      indx, 0, buf, size, 500);
-	if (ret > 0 && ret <= size)
-		memcpy(data, buf, ret);
-	kfree(buf);
-	return ret;
+	return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS,
+				    RTL8150_REQT_READ, indx, 0, data,
+				    size, 500);
 }
 
 static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
 {
-	void *buf;
-	int ret;
-
-	buf = kmemdup(data, size, GFP_NOIO);
-	if (!buf)
-		return -ENOMEM;
-
-	ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-			      RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE,
-			      indx, 0, buf, size, 500);
-	kfree(buf);
-	return ret;
+	return usb_control_msg_send(dev->udev, 0, RTL8150_REQ_SET_REGS,
+				    RTL8150_REQT_WRITE, indx, 0, data,
+				    size, 500);
 }
 
 static void async_set_reg_cb(struct urb *urb)
-- 
2.17.1

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

[Linux-kernel-mentees] [PATCH 4/4] net: rndis_host: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/net/usb/rndis_host.c | 44 ++++++++++++++----------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
index 6fa7a009a24a..30fc4a7183d3 100644
--- a/drivers/net/usb/rndis_host.c
+++ b/drivers/net/usb/rndis_host.c
@@ -113,14 +113,13 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
 		buf->request_id = (__force __le32) xid;
 	}
 	master_ifnum = info->control->cur_altsetting->desc.bInterfaceNumber;
-	retval = usb_control_msg(dev->udev,
-		usb_sndctrlpipe(dev->udev, 0),
-		USB_CDC_SEND_ENCAPSULATED_COMMAND,
-		USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-		0, master_ifnum,
-		buf, le32_to_cpu(buf->msg_len),
-		RNDIS_CONTROL_TIMEOUT_MS);
-	if (unlikely(retval < 0 || xid == 0))
+	retval = usb_control_msg_send(dev->udev, 0,
+				      USB_CDC_SEND_ENCAPSULATED_COMMAND,
+				      USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+				      0, master_ifnum, buf,
+				      le32_to_cpu(buf->msg_len),
+				      RNDIS_CONTROL_TIMEOUT_MS);
+	if (unlikely(xid == 0))
 		return retval;
 
 	/* Some devices don't respond on the control channel until
@@ -139,14 +138,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
 	/* Poll the control channel; the request probably completed immediately */
 	rsp = le32_to_cpu(buf->msg_type) | RNDIS_MSG_COMPLETION;
 	for (count = 0; count < 10; count++) {
-		memset(buf, 0, CONTROL_BUFFER_SIZE);
-		retval = usb_control_msg(dev->udev,
-			usb_rcvctrlpipe(dev->udev, 0),
-			USB_CDC_GET_ENCAPSULATED_RESPONSE,
-			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-			0, master_ifnum,
-			buf, buflen,
-			RNDIS_CONTROL_TIMEOUT_MS);
+		retval = usb_control_msg_recv(dev->udev, 0,
+					      USB_CDC_GET_ENCAPSULATED_RESPONSE,
+					      USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+					      0, master_ifnum, buf, buflen,
+					      RNDIS_CONTROL_TIMEOUT_MS);
 		if (likely(retval >= 8)) {
 			msg_type = le32_to_cpu(buf->msg_type);
 			msg_len = le32_to_cpu(buf->msg_len);
@@ -178,17 +174,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
 				msg->msg_type = cpu_to_le32(RNDIS_MSG_KEEPALIVE_C);
 				msg->msg_len = cpu_to_le32(sizeof *msg);
 				msg->status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
-				retval = usb_control_msg(dev->udev,
-					usb_sndctrlpipe(dev->udev, 0),
-					USB_CDC_SEND_ENCAPSULATED_COMMAND,
-					USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-					0, master_ifnum,
-					msg, sizeof *msg,
-					RNDIS_CONTROL_TIMEOUT_MS);
-				if (unlikely(retval < 0))
-					dev_dbg(&info->control->dev,
-						"rndis keepalive err %d\n",
-						retval);
+				retval = usb_control_msg_send(dev->udev, 0,
+							      USB_CDC_SEND_ENCAPSULATED_COMMAND,
+							      USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+							      0, master_ifnum, msg, sizeof(*msg),
+							      RNDIS_CONTROL_TIMEOUT_MS);
 				}
 				break;
 			default:
-- 
2.17.1

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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Oliver Neukum]

Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya:

Hi,

> Many usage of usb_control_msg() do not have proper error check on return
> value leaving scope for bugs on short reads. New usb_control_msg_recv()
> and usb_control_msg_send() nicely wraps usb_control_msg() with proper
> error check. Hence use the wrappers instead of calling usb_control_msg()
> directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
Nacked-by: Oliver Neukum <oneukum@suse.com>

> ---
>  drivers/net/usb/rtl8150.c | 32 ++++++--------------------------
>  1 file changed, 6 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 733f120c852b..e3002b675921 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150";
>  */
>  static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
>  {
> -	void *buf;
> -	int ret;
> -
> -	buf = kmalloc(size, GFP_NOIO);

GFP_NOIO is used here for a reason. You need to use this helper
while in contexts of error recovery and runtime PM.

> -	if (!buf)
> -		return -ENOMEM;
> -
> -	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> -			      RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> -			      indx, 0, buf, size, 500);
> -	if (ret > 0 && ret <= size)
> -		memcpy(data, buf, ret);
> -	kfree(buf);
> -	return ret;
> +	return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS,
> +				    RTL8150_REQT_READ, indx, 0, data,
> +				    size, 500);

This internally uses kmemdup() with GFP_KERNEL.
You cannot make this change. The API does not support it.
I am afraid we will have to change the API first, before more
such changes are done.

I would suggest dropping the whole series for now.

	Regards
		Oliver

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

Re: [Linux-kernel-mentees] [PATCH 4/4] net: rndis_host: use usb_control_msg_recv() and usb_control_msg_send()

[Greg KH]

On Wed, Sep 23, 2020 at 02:35:19PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/net/usb/rndis_host.c | 44 ++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> index 6fa7a009a24a..30fc4a7183d3 100644
> --- a/drivers/net/usb/rndis_host.c
> +++ b/drivers/net/usb/rndis_host.c
> @@ -113,14 +113,13 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
>  		buf->request_id = (__force __le32) xid;
>  	}
>  	master_ifnum = info->control->cur_altsetting->desc.bInterfaceNumber;
> -	retval = usb_control_msg(dev->udev,
> -		usb_sndctrlpipe(dev->udev, 0),
> -		USB_CDC_SEND_ENCAPSULATED_COMMAND,
> -		USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -		0, master_ifnum,
> -		buf, le32_to_cpu(buf->msg_len),
> -		RNDIS_CONTROL_TIMEOUT_MS);
> -	if (unlikely(retval < 0 || xid == 0))
> +	retval = usb_control_msg_send(dev->udev, 0,
> +				      USB_CDC_SEND_ENCAPSULATED_COMMAND,
> +				      USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +				      0, master_ifnum, buf,
> +				      le32_to_cpu(buf->msg_len),
> +				      RNDIS_CONTROL_TIMEOUT_MS);
> +	if (unlikely(xid == 0))
>  		return retval;
>  
>  	/* Some devices don't respond on the control channel until
> @@ -139,14 +138,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
>  	/* Poll the control channel; the request probably completed immediately */
>  	rsp = le32_to_cpu(buf->msg_type) | RNDIS_MSG_COMPLETION;
>  	for (count = 0; count < 10; count++) {
> -		memset(buf, 0, CONTROL_BUFFER_SIZE);
> -		retval = usb_control_msg(dev->udev,
> -			usb_rcvctrlpipe(dev->udev, 0),
> -			USB_CDC_GET_ENCAPSULATED_RESPONSE,
> -			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -			0, master_ifnum,
> -			buf, buflen,
> -			RNDIS_CONTROL_TIMEOUT_MS);
> +		retval = usb_control_msg_recv(dev->udev, 0,
> +					      USB_CDC_GET_ENCAPSULATED_RESPONSE,
> +					      USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +					      0, master_ifnum, buf, buflen,
> +					      RNDIS_CONTROL_TIMEOUT_MS);
>  		if (likely(retval >= 8)) {

retval here is never going to be positive, right?  So I don't think this
patch is correct :(

>  			msg_type = le32_to_cpu(buf->msg_type);
>  			msg_len = le32_to_cpu(buf->msg_len);
> @@ -178,17 +174,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
>  				msg->msg_type = cpu_to_le32(RNDIS_MSG_KEEPALIVE_C);
>  				msg->msg_len = cpu_to_le32(sizeof *msg);
>  				msg->status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
> -				retval = usb_control_msg(dev->udev,
> -					usb_sndctrlpipe(dev->udev, 0),
> -					USB_CDC_SEND_ENCAPSULATED_COMMAND,
> -					USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -					0, master_ifnum,
> -					msg, sizeof *msg,
> -					RNDIS_CONTROL_TIMEOUT_MS);
> -				if (unlikely(retval < 0))
> -					dev_dbg(&info->control->dev,
> -						"rndis keepalive err %d\n",
> -						retval);
> +				retval = usb_control_msg_send(dev->udev, 0,
> +							      USB_CDC_SEND_ENCAPSULATED_COMMAND,
> +							      USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +							      0, master_ifnum, msg, sizeof(*msg),
> +							      RNDIS_CONTROL_TIMEOUT_MS);

You lost the error message that the previous call had if something went
wrong.  Don't know if it's really needed, but there's no reason to
remove it here.

thanks,

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

Re: [Linux-kernel-mentees] [PATCH 0/4] net: usb: avoid using usb_control_msg() directly

[Greg KH]

On Wed, Sep 23, 2020 at 02:35:15PM +0530, Himadri Pandya wrote:
> A recent bug-fix shaded light on possible incorrect use of
> usb_control_msg() without proper error check. This resulted in
> introducing new usb core api wrapper functions usb_control_msg_send()
> and usb_control_msg_recv() by Greg KH. This patch series continue the
> clean-up using the new functions.
> 
> Himadri Pandya (4):
>   net: usbnet: use usb_control_msg_recv() and usb_control_msg_send()
>   net: sierra_net: use usb_control_msg_recv()
>   net: usb: rtl8150: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   net: rndis_host: use usb_control_msg_recv() and usb_control_msg_send()
> 
>  drivers/net/usb/rndis_host.c | 44 +++++++++++++---------------------
>  drivers/net/usb/rtl8150.c    | 32 +++++--------------------
>  drivers/net/usb/sierra_net.c | 17 ++++++-------
>  drivers/net/usb/usbnet.c     | 46 ++++--------------------------------
>  4 files changed, 34 insertions(+), 105 deletions(-)
> 

Note, all of these depend on a set of patches in my usb tree, so they
should probably wait to be sent to the networking developers until after
5.10-rc1 is out.

thanks,

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

Re: [Linux-kernel-mentees] [PATCH 1/4] net: usbnet: use usb_control_msg_recv() and usb_control_msg_send()

[Greg KH]

On Wed, Sep 23, 2020 at 02:35:16PM +0530, Himadri Pandya wrote:
> Potential incorrect use of usb_control_msg() has resulted in new wrapper
> functions to enforce its correct usage with proper error check. Hence
> use these new wrapper functions instead of calling usb_control_msg()
> directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> ---
>  drivers/net/usb/usbnet.c | 46 ++++------------------------------------
>  1 file changed, 4 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 2b2a841cd938..a38a85bef46a 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1982,64 +1982,26 @@ 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,
> +	return usb_control_msg_recv(dev->udev, 0,
> +			      cmd, reqtype, value, index, data, 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;
>  }

Now there is no real need for these wrapper functions at all, except for
the debugging which I doubt anyone needs anymore.

So how about just deleting these and calling the real function instead?

thanks,

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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya:
>
> Hi,
>
> > Many usage of usb_control_msg() do not have proper error check on return
> > value leaving scope for bugs on short reads. New usb_control_msg_recv()
> > and usb_control_msg_send() nicely wraps usb_control_msg() with proper
> > error check. Hence use the wrappers instead of calling usb_control_msg()
> > directly.
> >
> > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> Nacked-by: Oliver Neukum <oneukum@suse.com>
>
> > ---
> >  drivers/net/usb/rtl8150.c | 32 ++++++--------------------------
> >  1 file changed, 6 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index 733f120c852b..e3002b675921 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150";
> >  */
> >  static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> >  {
> > -     void *buf;
> > -     int ret;
> > -
> > -     buf = kmalloc(size, GFP_NOIO);
>
> GFP_NOIO is used here for a reason. You need to use this helper
> while in contexts of error recovery and runtime PM.
>

Understood. Apologies for proposing such a stupid change.

> > -     if (!buf)
> > -             return -ENOMEM;
> > -
> > -     ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> > -                           RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> > -                           indx, 0, buf, size, 500);
> > -     if (ret > 0 && ret <= size)
> > -             memcpy(data, buf, ret);
> > -     kfree(buf);
> > -     return ret;
> > +     return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS,
> > +                                 RTL8150_REQT_READ, indx, 0, data,
> > +                                 size, 500);
>
> This internally uses kmemdup() with GFP_KERNEL.
> You cannot make this change. The API does not support it.
> I am afraid we will have to change the API first, before more
> such changes are done.
>
> I would suggest dropping the whole series for now.

Okay. Thanks for the review.

Regards,
Himadri

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

Re: [Linux-kernel-mentees] [PATCH 1/4] net: usbnet: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

On Wed, Sep 23, 2020 at 3:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 23, 2020 at 02:35:16PM +0530, Himadri Pandya wrote:
> > Potential incorrect use of usb_control_msg() has resulted in new wrapper
> > functions to enforce its correct usage with proper error check. Hence
> > use these new wrapper functions instead of calling usb_control_msg()
> > directly.
> >
> > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> > ---
> >  drivers/net/usb/usbnet.c | 46 ++++------------------------------------
> >  1 file changed, 4 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> > index 2b2a841cd938..a38a85bef46a 100644
> > --- a/drivers/net/usb/usbnet.c
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -1982,64 +1982,26 @@ 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,
> > +     return usb_control_msg_recv(dev->udev, 0,
> > +                           cmd, reqtype, value, index, data, 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;
> >  }
>
> Now there is no real need for these wrapper functions at all, except for
> the debugging which I doubt anyone needs anymore.
>
> So how about just deleting these and calling the real function instead?
>

Yes, that would be a better thing to do.

Thanks,
Himadri

> thanks,
>
> greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 4/4] net: rndis_host: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

On Wed, Sep 23, 2020 at 3:52 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 23, 2020 at 02:35:19PM +0530, Himadri Pandya wrote:
> > The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> > usb_control_msg() with proper error check. Hence use the wrappers
> > instead of calling usb_control_msg() directly.
> >
> > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> > ---
> >  drivers/net/usb/rndis_host.c | 44 ++++++++++++++----------------------
> >  1 file changed, 17 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> > index 6fa7a009a24a..30fc4a7183d3 100644
> > --- a/drivers/net/usb/rndis_host.c
> > +++ b/drivers/net/usb/rndis_host.c
> > @@ -113,14 +113,13 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
> >               buf->request_id = (__force __le32) xid;
> >       }
> >       master_ifnum = info->control->cur_altsetting->desc.bInterfaceNumber;
> > -     retval = usb_control_msg(dev->udev,
> > -             usb_sndctrlpipe(dev->udev, 0),
> > -             USB_CDC_SEND_ENCAPSULATED_COMMAND,
> > -             USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> > -             0, master_ifnum,
> > -             buf, le32_to_cpu(buf->msg_len),
> > -             RNDIS_CONTROL_TIMEOUT_MS);
> > -     if (unlikely(retval < 0 || xid == 0))
> > +     retval = usb_control_msg_send(dev->udev, 0,
> > +                                   USB_CDC_SEND_ENCAPSULATED_COMMAND,
> > +                                   USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> > +                                   0, master_ifnum, buf,
> > +                                   le32_to_cpu(buf->msg_len),
> > +                                   RNDIS_CONTROL_TIMEOUT_MS);
> > +     if (unlikely(xid == 0))
> >               return retval;
> >
> >       /* Some devices don't respond on the control channel until
> > @@ -139,14 +138,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
> >       /* Poll the control channel; the request probably completed immediately */
> >       rsp = le32_to_cpu(buf->msg_type) | RNDIS_MSG_COMPLETION;
> >       for (count = 0; count < 10; count++) {
> > -             memset(buf, 0, CONTROL_BUFFER_SIZE);
> > -             retval = usb_control_msg(dev->udev,
> > -                     usb_rcvctrlpipe(dev->udev, 0),
> > -                     USB_CDC_GET_ENCAPSULATED_RESPONSE,
> > -                     USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> > -                     0, master_ifnum,
> > -                     buf, buflen,
> > -                     RNDIS_CONTROL_TIMEOUT_MS);
> > +             retval = usb_control_msg_recv(dev->udev, 0,
> > +                                           USB_CDC_GET_ENCAPSULATED_RESPONSE,
> > +                                           USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> > +                                           0, master_ifnum, buf, buflen,
> > +                                           RNDIS_CONTROL_TIMEOUT_MS);
> >               if (likely(retval >= 8)) {
>
> retval here is never going to be positive, right?  So I don't think this
> patch is correct :(
>

Yes :(.

> >                       msg_type = le32_to_cpu(buf->msg_type);
> >                       msg_len = le32_to_cpu(buf->msg_len);
> > @@ -178,17 +174,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen)
> >                               msg->msg_type = cpu_to_le32(RNDIS_MSG_KEEPALIVE_C);
> >                               msg->msg_len = cpu_to_le32(sizeof *msg);
> >                               msg->status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
> > -                             retval = usb_control_msg(dev->udev,
> > -                                     usb_sndctrlpipe(dev->udev, 0),
> > -                                     USB_CDC_SEND_ENCAPSULATED_COMMAND,
> > -                                     USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> > -                                     0, master_ifnum,
> > -                                     msg, sizeof *msg,
> > -                                     RNDIS_CONTROL_TIMEOUT_MS);
> > -                             if (unlikely(retval < 0))
> > -                                     dev_dbg(&info->control->dev,
> > -                                             "rndis keepalive err %d\n",
> > -                                             retval);
> > +                             retval = usb_control_msg_send(dev->udev, 0,
> > +                                                           USB_CDC_SEND_ENCAPSULATED_COMMAND,
> > +                                                           USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> > +                                                           0, master_ifnum, msg, sizeof(*msg),
> > +                                                           RNDIS_CONTROL_TIMEOUT_MS);
>
> You lost the error message that the previous call had if something went
> wrong.  Don't know if it's really needed, but there's no reason to
> remove it here.
>

The wrapper returns the error so thought that might work instead. But
yes, the old msg is better.

Anyways, this series is dropped :).

Thanks for the review,
Himadri

> thanks,
>
> greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Oliver Neukum]

Am Mittwoch, den 23.09.2020, 19:36 +0530 schrieb Himadri Pandya:
> On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote:
> > 
> > Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya:

> > GFP_NOIO is used here for a reason. You need to use this helper
> > while in contexts of error recovery and runtime PM.
> > 
> 
> Understood. Apologies for proposing such a stupid change.

Hi,

sorry if you concluded that the patch was stupid. That was not my
intent. It was the best the API allowed for. If an API makes it
easy to make a mistake, the problem is with the API, not the developer.

	Regards
		Oliver

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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

On Wed, Sep 23, 2020 at 7:51 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Mittwoch, den 23.09.2020, 19:36 +0530 schrieb Himadri Pandya:
> > On Wed, Sep 23, 2020 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote:
> > >
> > > Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya:
>
> > > GFP_NOIO is used here for a reason. You need to use this helper
> > > while in contexts of error recovery and runtime PM.
> > >
> >
> > Understood. Apologies for proposing such a stupid change.
>
> Hi,
>
> sorry if you concluded that the patch was stupid. That was not my
> intent. It was the best the API allowed for. If an API makes it
> easy to make a mistake, the problem is with the API, not the developer.
>
>         Regards
>                 Oliver
>

I meant that it was stupid to change it without properly understanding
the significance of GFP_NOIO in this context.

So now, do we re-write the wrapper functions with flag passed as a parameter?

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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Petko Manolov]

On 20-09-23 12:22:37, Oliver Neukum wrote:
> Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya:
> 
> Hi,
> 
> > Many usage of usb_control_msg() do not have proper error check on return
> > value leaving scope for bugs on short reads. New usb_control_msg_recv()
> > and usb_control_msg_send() nicely wraps usb_control_msg() with proper
> > error check. Hence use the wrappers instead of calling usb_control_msg()
> > directly.
> > 
> > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> Nacked-by: Oliver Neukum <oneukum@suse.com>
> 
> > ---
> >  drivers/net/usb/rtl8150.c | 32 ++++++--------------------------
> >  1 file changed, 6 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index 733f120c852b..e3002b675921 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150";
> >  */
> >  static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> >  {
> > -	void *buf;
> > -	int ret;
> > -
> > -	buf = kmalloc(size, GFP_NOIO);
> 
> GFP_NOIO is used here for a reason. You need to use this helper
> while in contexts of error recovery and runtime PM.
> 
> > -	if (!buf)
> > -		return -ENOMEM;
> > -
> > -	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> > -			      RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> > -			      indx, 0, buf, size, 500);
> > -	if (ret > 0 && ret <= size)
> > -		memcpy(data, buf, ret);
> > -	kfree(buf);
> > -	return ret;
> > +	return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS,
> > +				    RTL8150_REQT_READ, indx, 0, data,
> > +				    size, 500);
> 
> This internally uses kmemdup() with GFP_KERNEL.
> You cannot make this change. The API does not support it.
> I am afraid we will have to change the API first, before more
> such changes are done.

One possible fix is to add yet another argument to usb_control_msg_recv(), which 
would be the GFP_XYZ flag to pass on to kmemdup().  Up to Greg, of course.


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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Oliver Neukum]

Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov:

> > This internally uses kmemdup() with GFP_KERNEL.
> > You cannot make this change. The API does not support it.
> > I am afraid we will have to change the API first, before more
> > such changes are done.
> 
> One possible fix is to add yet another argument to usb_control_msg_recv(), which 
> would be the GFP_XYZ flag to pass on to kmemdup().  Up to Greg, of course.

Hi,

submitted. The problem is those usages that are very hard to trace.
I'd dislike to just slab GFP_NOIO on them for no obvious reason.

	Regards
		Oliver


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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Oliver Neukum]

Am Mittwoch, den 23.09.2020, 20:02 +0530 schrieb Himadri Pandya:

> I meant that it was stupid to change it without properly understanding
> the significance of GFP_NOIO in this context.
> 
> So now, do we re-write the wrapper functions with flag passed as a parameter?

Hi,

I hope I set you in CC for a patch set doing exactly that.

Do not let me or other maintainers discourage you from writing patches.
Look at it this way. Had you not written this patch, I would not have
looked into the matter. Patches are supposed to be reviewed.
If you want additional information, just ask. We do not want
people discouraged from writing substantial patches.

	Regards
		Oliver


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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Petko Manolov]

On 20-09-24 13:09:05, Oliver Neukum wrote:
> Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov:
> 
> > One possible fix is to add yet another argument to usb_control_msg_recv(), 
> > which would be the GFP_XYZ flag to pass on to kmemdup().  Up to Greg, of 
> > course.
> 
> submitted. The problem is those usages that are very hard to trace. I'd 
> dislike to just slab GFP_NOIO on them for no obvious reason.

Do you mean you submitted a patch for usb_control_msg_recv() (because i don't 
see it on linux-netdev) or i'm reading this all wrong?


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

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Greg KH]

On Thu, Sep 24, 2020 at 06:40:26PM +0300, Petko Manolov wrote:
> On 20-09-24 13:09:05, Oliver Neukum wrote:
> > Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov:
> > 
> > > One possible fix is to add yet another argument to usb_control_msg_recv(), 
> > > which would be the GFP_XYZ flag to pass on to kmemdup().  Up to Greg, of 
> > > course.
> > 
> > submitted. The problem is those usages that are very hard to trace. I'd 
> > dislike to just slab GFP_NOIO on them for no obvious reason.
> 
> Do you mean you submitted a patch for usb_control_msg_recv() (because i don't 
> see it on linux-netdev) or i'm reading this all wrong?

It's on the linux-usb list:
	https://lore.kernel.org/r/20200923134348.23862-1-oneukum@suse.com
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Himadri Pandya]

On Thu, Sep 24, 2020 at 5:06 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Mittwoch, den 23.09.2020, 20:02 +0530 schrieb Himadri Pandya:
>
> > I meant that it was stupid to change it without properly understanding
> > the significance of GFP_NOIO in this context.
> >
> > So now, do we re-write the wrapper functions with flag passed as a parameter?
>
> Hi,
>
> I hope I set you in CC for a patch set doing exactly that.
>

Yes.

> Do not let me or other maintainers discourage you from writing patches.
> Look at it this way. Had you not written this patch, I would not have
> looked into the matter. Patches are supposed to be reviewed.
> If you want additional information, just ask. We do not want
> people discouraged from writing substantial patches.
>

Understood :).

I'll send v2 after the update in API is merged.

Thanks,
Himadri

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