[PATCH net 0/4] Fix on-stack USB buffers

[PATCH net 0/4] Fix on-stack USB buffers

[Ben Hutchings]

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

Allocating USB buffers on the stack is not portable, and no longer
works on x86_64 (with VMAP_STACK enabled as per default).  This
series fixes all the instances I could find where USB networking
drivers do that.

Ben.

Ben Hutchings (4):
  pegasus: Use heap buffers for all register access
  rtl8150: Use heap buffers for all register access
  catc: Combine failure cleanup code in catc_probe()
  catc: Use heap buffer for memory size test

 drivers/net/usb/catc.c    | 56 ++++++++++++++++++++++++++++-------------------
 drivers/net/usb/pegasus.c | 29 ++++++++++++++++++++----
 drivers/net/usb/rtl8150.c | 34 ++++++++++++++++++++++------
 3 files changed, 86 insertions(+), 33 deletions(-)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH net 1/4] pegasus: Use heap buffers for all register access

[Ben Hutchings]

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

Allocating USB buffers on the stack is not portable, and no longer
works on x86_64 (with VMAP_STACK enabled as per default).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
References: https://bugs.debian.org/852556
Reported-by: Lisandro Damián Nicanor Pérez Meyer <lisandro@debian.org>
Tested-by: Lisandro Damián Nicanor Pérez Meyer <lisandro@debian.org>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/net/usb/pegasus.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 24e803fe9a53..36674484c6fb 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -126,40 +126,61 @@ static void async_ctrl_callback(struct urb *urb)
 
 static int get_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void *data)
 {
+	u8 *buf;
 	int ret;
 
+	buf = kmalloc(size, GFP_NOIO);
+	if (!buf)
+		return -ENOMEM;
+
 	ret = usb_control_msg(pegasus->usb, usb_rcvctrlpipe(pegasus->usb, 0),
 			      PEGASUS_REQ_GET_REGS, PEGASUS_REQT_READ, 0,
-			      indx, data, size, 1000);
+			      indx, buf, size, 1000);
 	if (ret < 0)
 		netif_dbg(pegasus, drv, pegasus->net,
 			  "%s returned %d\n", __func__, ret);
+	else if (ret <= size)
+		memcpy(data, buf, ret);
+	kfree(buf);
 	return ret;
 }
 
-static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void *data)
+static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
+			 const void *data)
 {
+	u8 *buf;
 	int ret;
 
+	buf = kmemdup(data, size, GFP_NOIO);
+	if (!buf)
+		return -ENOMEM;
+
 	ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
 			      PEGASUS_REQ_SET_REGS, PEGASUS_REQT_WRITE, 0,
-			      indx, data, size, 100);
+			      indx, buf, size, 100);
 	if (ret < 0)
 		netif_dbg(pegasus, drv, pegasus->net,
 			  "%s returned %d\n", __func__, ret);
+	kfree(buf);
 	return ret;
 }
 
 static int set_register(pegasus_t *pegasus, __u16 indx, __u8 data)
 {
+	u8 *buf;
 	int ret;
 
+	buf = kmemdup(&data, 1, GFP_NOIO);
+	if (!buf)
+		return -ENOMEM;
+
 	ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
 			      PEGASUS_REQ_SET_REG, PEGASUS_REQT_WRITE, data,
-			      indx, &data, 1, 1000);
+			      indx, buf, 1, 1000);
 	if (ret < 0)
 		netif_dbg(pegasus, drv, pegasus->net,
 			  "%s returned %d\n", __func__, ret);
+	kfree(buf);
 	return ret;
 }
 


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Ben Hutchings]

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

Allocating USB buffers on the stack is not portable, and no longer
works on x86_64 (with VMAP_STACK enabled as per default).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/net/usb/rtl8150.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index 95b7bd0d7abc..c81c79110cef 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -155,16 +155,36 @@ static const char driver_name [] = "rtl8150";
 */
 static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
 {
-	return usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
-			       RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
-			       indx, 0, data, size, 500);
+	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;
 }
 
-static int set_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
+static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
 {
-	return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
-			       RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE,
-			       indx, 0, data, size, 500);
+	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;
 }
 
 static void async_set_reg_cb(struct urb *urb)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH net 3/4] catc: Combine failure cleanup code in catc_probe()

[Ben Hutchings]

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

Signed-off-by: Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>
---
 drivers/net/usb/catc.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/net/usb/catc.c b/drivers/net/usb/catc.c
index 3daa41bdd4ea..985909eab72c 100644
--- a/drivers/net/usb/catc.c
+++ b/drivers/net/usb/catc.c
@@ -776,7 +776,7 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id
 	struct net_device *netdev;
 	struct catc *catc;
 	u8 broadcast[ETH_ALEN];
-	int i, pktsz;
+	int i, pktsz, ret;
 
 	if (usb_set_interface(usbdev,
 			intf->altsetting->desc.bInterfaceNumber, 1)) {
@@ -811,12 +811,8 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id
 	if ((!catc->ctrl_urb) || (!catc->tx_urb) || 
 	    (!catc->rx_urb) || (!catc->irq_urb)) {
 		dev_err(&intf->dev, "No free urbs available.\n");
-		usb_free_urb(catc->ctrl_urb);
-		usb_free_urb(catc->tx_urb);
-		usb_free_urb(catc->rx_urb);
-		usb_free_urb(catc->irq_urb);
-		free_netdev(netdev);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto fail_free;
 	}
 
 	/* The F5U011 has the same vendor/product as the netmate but a device version of 0x130 */
@@ -913,16 +909,21 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id
 	usb_set_intfdata(intf, catc);
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
-	if (register_netdev(netdev) != 0) {
-		usb_set_intfdata(intf, NULL);
-		usb_free_urb(catc->ctrl_urb);
-		usb_free_urb(catc->tx_urb);
-		usb_free_urb(catc->rx_urb);
-		usb_free_urb(catc->irq_urb);
-		free_netdev(netdev);
-		return -EIO;
-	}
+	ret = register_netdev(netdev);
+	if (ret)
+		goto fail_clear_intfdata;
+
 	return 0;
+
+fail_clear_intfdata:
+	usb_set_intfdata(intf, NULL);
+fail_free:
+	usb_free_urb(catc->ctrl_urb);
+	usb_free_urb(catc->tx_urb);
+	usb_free_urb(catc->rx_urb);
+	usb_free_urb(catc->irq_urb);
+	free_netdev(netdev);
+	return ret;
 }
 
 static void catc_disconnect(struct usb_interface *intf)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH net 4/4] catc: Use heap buffer for memory size test

[Ben Hutchings]

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

Allocating USB buffers on the stack is not portable, and no longer
works on x86_64 (with VMAP_STACK enabled as per default).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/net/usb/catc.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/catc.c b/drivers/net/usb/catc.c
index 985909eab72c..0acc9b640419 100644
--- a/drivers/net/usb/catc.c
+++ b/drivers/net/usb/catc.c
@@ -776,7 +776,7 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id
 	struct net_device *netdev;
 	struct catc *catc;
 	u8 broadcast[ETH_ALEN];
-	int i, pktsz, ret;
+	int pktsz, ret;
 
 	if (usb_set_interface(usbdev,
 			intf->altsetting->desc.bInterfaceNumber, 1)) {
@@ -840,15 +840,24 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id
                 catc->irq_buf, 2, catc_irq_done, catc, 1);
 
 	if (!catc->is_f5u011) {
+		u32 *buf;
+		int i;
+
 		dev_dbg(dev, "Checking memory size\n");
 
-		i = 0x12345678;
-		catc_write_mem(catc, 0x7a80, &i, 4);
-		i = 0x87654321;	
-		catc_write_mem(catc, 0xfa80, &i, 4);
-		catc_read_mem(catc, 0x7a80, &i, 4);
+		buf = kmalloc(4, GFP_KERNEL);
+		if (!buf) {
+			ret = -ENOMEM;
+			goto fail_free;
+		}
+
+		*buf = 0x12345678;
+		catc_write_mem(catc, 0x7a80, buf, 4);
+		*buf = 0x87654321;
+		catc_write_mem(catc, 0xfa80, buf, 4);
+		catc_read_mem(catc, 0x7a80, buf, 4);
 	  
-		switch (i) {
+		switch (*buf) {
 		case 0x12345678:
 			catc_set_reg(catc, TxBufCount, 8);
 			catc_set_reg(catc, RxBufCount, 32);
@@ -863,6 +872,8 @@ static int catc_probe(struct usb_interface *intf, const struct usb_device_id *id
 			dev_dbg(dev, "32k Memory\n");
 			break;
 		}
+
+		kfree(buf);
 	  
 		dev_dbg(dev, "Getting MAC from SEEROM.\n");
 	  

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Greg KH]

On Sat, Feb 04, 2017 at 04:56:03PM +0000, Ben Hutchings wrote:
> Allocating USB buffers on the stack is not portable, and no longer
> works on x86_64 (with VMAP_STACK enabled as per default).

It's never worked on other platforms, so these should go to the stable
releases please.

thanks,

greg k-h

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-04 16:56:32, Ben Hutchings wrote:
> Allocating USB buffers on the stack is not portable, and no longer
> works on x86_64 (with VMAP_STACK enabled as per default).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  drivers/net/usb/rtl8150.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 95b7bd0d7abc..c81c79110cef 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -155,16 +155,36 @@ static const char driver_name [] = "rtl8150";
>  */
>  static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
>  {
> -	return usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> -			       RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> -			       indx, 0, data, size, 500);
> +	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;
>  }
>  
> -static int set_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> +static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
>  {
> -	return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
> -			       RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE,
> -			       indx, 0, data, size, 500);
> +	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;
>  }
>  
>  static void async_set_reg_cb(struct urb *urb)
> 


ACK.


cheers,
Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-05 01:30:39, Greg KH wrote:
> On Sat, Feb 04, 2017 at 04:56:03PM +0000, Ben Hutchings wrote:
> > Allocating USB buffers on the stack is not portable, and no longer
> > works on x86_64 (with VMAP_STACK enabled as per default).
> 
> It's never worked on other platforms, so these should go to the stable 
> releases please.

As far as i know both drivers works fine on other platforms, though I only 
tested it on arm and mipsel. ;)

Random thought: isn't it better to add the alloc/free code in usb_control_msg() 
and avoid code duplication all over the driver space?


cheers,
Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Greg KH]

On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> On 17-02-05 01:30:39, Greg KH wrote:
> > On Sat, Feb 04, 2017 at 04:56:03PM +0000, Ben Hutchings wrote:
> > > Allocating USB buffers on the stack is not portable, and no longer
> > > works on x86_64 (with VMAP_STACK enabled as per default).
> > 
> > It's never worked on other platforms, so these should go to the stable 
> > releases please.
> 
> As far as i know both drivers works fine on other platforms, though I only 
> tested it on arm and mipsel. ;)

It all depends on the arm and mips platforms, the ones that can not DMA
from stack memory are the ones that would always fail here (since the
2.2 kernel days).

> Random thought: isn't it better to add the alloc/free code in usb_control_msg() 
> and avoid code duplication all over the driver space?

A very long time ago we considered it, but realized that the majority of
drivers already had the memory dynamically allocated, so we just went
with this.  Perhaps we could revisit that if it turns out we were wrong,
and would simplify things.

thanks,

greg k-h

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-06 09:28:22, Greg KH wrote:
> On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > On 17-02-05 01:30:39, Greg KH wrote:
> > > On Sat, Feb 04, 2017 at 04:56:03PM +0000, Ben Hutchings wrote:
> > > > Allocating USB buffers on the stack is not portable, and no longer works 
> > > > on x86_64 (with VMAP_STACK enabled as per default).
> > > 
> > > It's never worked on other platforms, so these should go to the stable 
> > > releases please.
> > 
> > As far as i know both drivers works fine on other platforms, though I only 
> > tested it on arm and mipsel. ;)
> 
> It all depends on the arm and mips platforms, the ones that can not DMA from 
> stack memory are the ones that would always fail here (since the 2.2 kernel 
> days).

Seems like most modern SOCs have decent DMA controllers.

> > Random thought: isn't it better to add the alloc/free code in 
> > usb_control_msg() and avoid code duplication all over the driver space?
> 
> A very long time ago we considered it, but realized that the majority of 
> drivers already had the memory dynamically allocated, so we just went with 
> this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> simplify things.

A quick glance at usb_control_msg() users (looked only at .../net/usb and 
.../usb/serial) shows that most of them have the buffer allocated in stack.  
These transfers are relatively infrequent and quite small the performance hit 
caused by memcpy() should also be negligible.

I suspect getting the buffer allocation in usb_control_msg() will help with two 
things in the same time:

 - allocate in DMA-able memory;
 - code reduction;

On the bad side i've no idea who'll volunteer to go through all 
usb_control_msg() sites and perform the fix.  Seems like the right job for an 
intern to me. :D


cheers,
Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Johan Hovold]

On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> On 17-02-06 09:28:22, Greg KH wrote:
> > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:

> > > Random thought: isn't it better to add the alloc/free code in 
> > > usb_control_msg() and avoid code duplication all over the driver space?
> > 
> > A very long time ago we considered it, but realized that the majority of 
> > drivers already had the memory dynamically allocated, so we just went with 
> > this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> > simplify things.
> 
> A quick glance at usb_control_msg() users (looked only at .../net/usb and 
> .../usb/serial) shows that most of them have the buffer allocated in stack.  

That's simply not true at all for usb-serial. If you find an instance
were a stack allocated buffer is used for DMA that hasn't yet been
fixed in USB serial, then please point it out so we can fix it up (I
doubt you'll find one, though).

> These transfers are relatively infrequent and quite small the performance hit 
> caused by memcpy() should also be negligible.

Note that some drivers allocate a single buffer which is reused for
several (or all) control transfers.

Johan

RE: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[David Laight]

From: Petko Manolov
> Sent: 06 February 2017 12:51
...
> I suspect getting the buffer allocation in usb_control_msg() will help with two
> things in the same time:
> 
>  - allocate in DMA-able memory;
>  - code reduction;

Is there code around there to do 'bounce buffer' allocation
for systems where not all dma memory is actually accessible
by all devices??

In which case it could (probably) be used for non-dma memory?

	David

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Johan Hovold]

On Mon, Feb 06, 2017 at 02:21:24PM +0100, Johan Hovold wrote:
> On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> > On 17-02-06 09:28:22, Greg KH wrote:
> > > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> 
> > > > Random thought: isn't it better to add the alloc/free code in 
> > > > usb_control_msg() and avoid code duplication all over the driver space?
> > > 
> > > A very long time ago we considered it, but realized that the majority of 
> > > drivers already had the memory dynamically allocated, so we just went with 
> > > this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> > > simplify things.
> > 
> > A quick glance at usb_control_msg() users (looked only at .../net/usb and 
> > .../usb/serial) shows that most of them have the buffer allocated in stack.  
> 
> That's simply not true at all for usb-serial. If you find an instance
> were a stack allocated buffer is used for DMA that hasn't yet been
> fixed in USB serial, then please point it out so we can fix it up (I
> doubt you'll find one, though).

Heh, I just found one myself (in ark3116) that I'll fix up. Please let
me know if you find any more.

Johan

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Johan Hovold]

On Mon, Feb 06, 2017 at 02:32:23PM +0100, Johan Hovold wrote:
> On Mon, Feb 06, 2017 at 02:21:24PM +0100, Johan Hovold wrote:
> > On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> > > On 17-02-06 09:28:22, Greg KH wrote:
> > > > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > 
> > > > > Random thought: isn't it better to add the alloc/free code in 
> > > > > usb_control_msg() and avoid code duplication all over the driver space?
> > > > 
> > > > A very long time ago we considered it, but realized that the majority of 
> > > > drivers already had the memory dynamically allocated, so we just went with 
> > > > this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> > > > simplify things.
> > > 
> > > A quick glance at usb_control_msg() users (looked only at .../net/usb and 
> > > .../usb/serial) shows that most of them have the buffer allocated in stack.  
> > 
> > That's simply not true at all for usb-serial. If you find an instance
> > were a stack allocated buffer is used for DMA that hasn't yet been
> > fixed in USB serial, then please point it out so we can fix it up (I
> > doubt you'll find one, though).
> 
> Heh, I just found one myself (in ark3116) that I'll fix up. Please let
> me know if you find any more.

False alarm, the ark3116 driver is fine too.

Johan

RE: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[David Laight]

From: Ben Hutchings
> Sent: 04 February 2017 16:57
> Allocating USB buffers on the stack is not portable, and no longer
> works on x86_64 (with VMAP_STACK enabled as per default).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>
> ---
>  drivers/net/usb/rtl8150.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 95b7bd0d7abc..c81c79110cef 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -155,16 +155,36 @@ static const char driver_name [] = "rtl8150";
>  */
>  static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
>  {
> -	return usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> -			       RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> -			       indx, 0, data, size, 500);
> +	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);

If ret > size something is horridly wrong.
Silently not updating the callers buffer at all cannot be right.

> +	kfree(buf);
> +	return ret;

I can't help feeling that it would be better to add a wrapper to
usb_control_msg() that does the kmalloc() and memcpy()s and
drop that into all the call sites.

	David

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Ben Hutchings]

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

On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> From: Ben Hutchings
[...]
> > +	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);
> 
> If ret > size something is horridly wrong.
> Silently not updating the callers buffer at all cannot be right.

Yes, it seems strange to check this.  I originally wrote this as ret >
0, but then I checked the usbnet core and found __usbnet_read_cmd()
has the second comparison as well.

> > +	kfree(buf);
> > +	return ret;
> 
> I can't help feeling that it would be better to add a wrapper to
> usb_control_msg() that does the kmalloc() and memcpy()s and
> drop that into all the call sites.

It might be.  Right now I'm trying to patch up a bunch of regressions
rather than argue over an API change.

Ben.

-- 
Ben Hutchings
Experience is what causes a person to make new mistakes instead of old ones.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-06 14:46:21, Johan Hovold wrote:
> On Mon, Feb 06, 2017 at 02:32:23PM +0100, Johan Hovold wrote:
> > On Mon, Feb 06, 2017 at 02:21:24PM +0100, Johan Hovold wrote:
> > > On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> > > > On 17-02-06 09:28:22, Greg KH wrote:
> > > > > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > > 
> > > > > > Random thought: isn't it better to add the alloc/free code in 
> > > > > > usb_control_msg() and avoid code duplication all over the driver space?
> > > > > 
> > > > > A very long time ago we considered it, but realized that the majority of 
> > > > > drivers already had the memory dynamically allocated, so we just went with 
> > > > > this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> > > > > simplify things.
> > > > 
> > > > A quick glance at usb_control_msg() users (looked only at .../net/usb and 
> > > > .../usb/serial) shows that most of them have the buffer allocated in stack.  
> > > 
> > > That's simply not true at all for usb-serial. If you find an instance
> > > were a stack allocated buffer is used for DMA that hasn't yet been
> > > fixed in USB serial, then please point it out so we can fix it up (I
> > > doubt you'll find one, though).
> > 
> > Heh, I just found one myself (in ark3116) that I'll fix up. Please let
> > me know if you find any more.
> 
> False alarm, the ark3116 driver is fine too.

You may want to check pl2303_vendor_read() in drivers/usb/serial/pl2303.c
It seems to me that 'buf' is allocated in the stack. ;)


		Petko

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-06 16:25:20, Ben Hutchings wrote:
> On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > From: Ben Hutchings
> [...]
> > > +	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);
> > 
> > If ret > size something is horridly wrong.
> > Silently not updating the callers buffer at all cannot be right.
> 
> Yes, it seems strange to check this.  I originally wrote this as ret >
> 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> has the second comparison as well.
> 
> > > +	kfree(buf);
> > > +	return ret;

Since we return what usb_control_msg() told us to return i assume the error code 
will be available to anybody who cares.

> > I can't help feeling that it would be better to add a wrapper to
> > usb_control_msg() that does the kmalloc() and memcpy()s and
> > drop that into all the call sites.
> 
> It might be.  Right now I'm trying to patch up a bunch of regressions rather 
> than argue over an API change.

Right, first thing first.

I am in favor of changing the API, but this should not happen in the stable 
releases.  I hope Greg will make up his mind and let us know.


cheers,
Petko

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Greg KH]

On Tue, Feb 07, 2017 at 12:24:12PM +0200, Petko Manolov wrote:
> On 17-02-06 14:46:21, Johan Hovold wrote:
> > On Mon, Feb 06, 2017 at 02:32:23PM +0100, Johan Hovold wrote:
> > > On Mon, Feb 06, 2017 at 02:21:24PM +0100, Johan Hovold wrote:
> > > > On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> > > > > On 17-02-06 09:28:22, Greg KH wrote:
> > > > > > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > > > 
> > > > > > > Random thought: isn't it better to add the alloc/free code in 
> > > > > > > usb_control_msg() and avoid code duplication all over the driver space?
> > > > > > 
> > > > > > A very long time ago we considered it, but realized that the majority of 
> > > > > > drivers already had the memory dynamically allocated, so we just went with 
> > > > > > this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> > > > > > simplify things.
> > > > > 
> > > > > A quick glance at usb_control_msg() users (looked only at .../net/usb and 
> > > > > .../usb/serial) shows that most of them have the buffer allocated in stack.  
> > > > 
> > > > That's simply not true at all for usb-serial. If you find an instance
> > > > were a stack allocated buffer is used for DMA that hasn't yet been
> > > > fixed in USB serial, then please point it out so we can fix it up (I
> > > > doubt you'll find one, though).
> > > 
> > > Heh, I just found one myself (in ark3116) that I'll fix up. Please let
> > > me know if you find any more.
> > 
> > False alarm, the ark3116 driver is fine too.
> 
> You may want to check pl2303_vendor_read() in drivers/usb/serial/pl2303.c
> It seems to me that 'buf' is allocated in the stack. ;)

Really?  Doesn't look like it to me, it's passed in from the caller and
it's allocated in that caller.  What am I missing here?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Greg KH]

On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> On 17-02-06 16:25:20, Ben Hutchings wrote:
> > On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > > From: Ben Hutchings
> > [...]
> > > > +	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);
> > > 
> > > If ret > size something is horridly wrong.
> > > Silently not updating the callers buffer at all cannot be right.
> > 
> > Yes, it seems strange to check this.  I originally wrote this as ret >
> > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > has the second comparison as well.
> > 
> > > > +	kfree(buf);
> > > > +	return ret;
> 
> Since we return what usb_control_msg() told us to return i assume the error code 
> will be available to anybody who cares.
> 
> > > I can't help feeling that it would be better to add a wrapper to
> > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > drop that into all the call sites.
> > 
> > It might be.  Right now I'm trying to patch up a bunch of regressions rather 
> > than argue over an API change.
> 
> Right, first thing first.
> 
> I am in favor of changing the API, but this should not happen in the stable 
> releases.  I hope Greg will make up his mind and let us know.

make up my mind about what?  These are bugs, they should be fixed, I'm
not taking a total api change at this point in time, sorry.

greg k-h

RE: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[David Laight]

From: Greg KH
> Sent: 07 February 2017 10:52
> To: Petko Manolov
> Cc: Ben Hutchings; David Laight; netdev@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access
> 
> On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > > > From: Ben Hutchings
> > > [...]
> > > > > +	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);
> > > >
> > > > If ret > size something is horridly wrong.
> > > > Silently not updating the callers buffer at all cannot be right.
> > >
> > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > has the second comparison as well.
> > >
> > > > > +	kfree(buf);
> > > > > +	return ret;
> >
> > Since we return what usb_control_msg() told us to return i assume the error code
> > will be available to anybody who cares.
> >
> > > > I can't help feeling that it would be better to add a wrapper to
> > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > drop that into all the call sites.
> > >
> > > It might be.  Right now I'm trying to patch up a bunch of regressions rather
> > > than argue over an API change.
> >
> > Right, first thing first.
> >
> > I am in favor of changing the API, but this should not happen in the stable
> > releases.  I hope Greg will make up his mind and let us know.
> 
> make up my mind about what?  These are bugs, they should be fixed, I'm
> not taking a total api change at this point in time, sorry.

Adding a usb_control_msg_with_malloc() wrapper is a smaller change than those
proposed. The smaller churn probably makes back porting other changes easier.

Given the nature of this problem, and how common it seems to be,
it is almost worth renaming usb_control_msg() itself so that all the
callers are properly audited.

	David

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

['Greg KH']

On Tue, Feb 07, 2017 at 11:56:51AM +0000, David Laight wrote:
> From: Greg KH
> > Sent: 07 February 2017 10:52
> > To: Petko Manolov
> > Cc: Ben Hutchings; David Laight; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access
> > 
> > On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > > On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > > > > From: Ben Hutchings
> > > > [...]
> > > > > > +	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);
> > > > >
> > > > > If ret > size something is horridly wrong.
> > > > > Silently not updating the callers buffer at all cannot be right.
> > > >
> > > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > > has the second comparison as well.
> > > >
> > > > > > +	kfree(buf);
> > > > > > +	return ret;
> > >
> > > Since we return what usb_control_msg() told us to return i assume the error code
> > > will be available to anybody who cares.
> > >
> > > > > I can't help feeling that it would be better to add a wrapper to
> > > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > > drop that into all the call sites.
> > > >
> > > > It might be.  Right now I'm trying to patch up a bunch of regressions rather
> > > > than argue over an API change.
> > >
> > > Right, first thing first.
> > >
> > > I am in favor of changing the API, but this should not happen in the stable
> > > releases.  I hope Greg will make up his mind and let us know.
> > 
> > make up my mind about what?  These are bugs, they should be fixed, I'm
> > not taking a total api change at this point in time, sorry.
> 
> Adding a usb_control_msg_with_malloc() wrapper is a smaller change than those
> proposed. The smaller churn probably makes back porting other changes easier.
> 
> Given the nature of this problem, and how common it seems to be,
> it is almost worth renaming usb_control_msg() itself so that all the
> callers are properly audited.

As this is something that we have been auditing for a decade now, I
don't think you will find all that many instances :)

But for now, fixes like this are fine, if someone wants to tackle the
larger issue here, with a new api function, that would be great.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 11:45:06, Greg KH wrote:
> On Tue, Feb 07, 2017 at 12:24:12PM +0200, Petko Manolov wrote:
> > On 17-02-06 14:46:21, Johan Hovold wrote:
> > > On Mon, Feb 06, 2017 at 02:32:23PM +0100, Johan Hovold wrote:
> > > > On Mon, Feb 06, 2017 at 02:21:24PM +0100, Johan Hovold wrote:
> > > > > On Mon, Feb 06, 2017 at 02:51:09PM +0200, Petko Manolov wrote:
> > > > > > On 17-02-06 09:28:22, Greg KH wrote:
> > > > > > > On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> > > > > 
> > > > > > > > Random thought: isn't it better to add the alloc/free code in 
> > > > > > > > usb_control_msg() and avoid code duplication all over the driver space?
> > > > > > > 
> > > > > > > A very long time ago we considered it, but realized that the majority of 
> > > > > > > drivers already had the memory dynamically allocated, so we just went with 
> > > > > > > this.  Perhaps we could revisit that if it turns out we were wrong, and would 
> > > > > > > simplify things.
> > > > > > 
> > > > > > A quick glance at usb_control_msg() users (looked only at .../net/usb and 
> > > > > > .../usb/serial) shows that most of them have the buffer allocated in stack.  
> > > > > 
> > > > > That's simply not true at all for usb-serial. If you find an instance
> > > > > were a stack allocated buffer is used for DMA that hasn't yet been
> > > > > fixed in USB serial, then please point it out so we can fix it up (I
> > > > > doubt you'll find one, though).
> > > > 
> > > > Heh, I just found one myself (in ark3116) that I'll fix up. Please let
> > > > me know if you find any more.
> > > 
> > > False alarm, the ark3116 driver is fine too.
> > 
> > You may want to check pl2303_vendor_read() in drivers/usb/serial/pl2303.c
> > It seems to me that 'buf' is allocated in the stack. ;)
> 
> Really?  Doesn't look like it to me, it's passed in from the caller and it's 
> allocated in that caller.  What am I missing here?

Sorry, got confused by the weird pointer definition of the last argument:

static int pl2303_vendor_read(struct usb_serial *serial, u16 value, unsigned char buf[1])


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 11:51:31, Greg KH wrote:
> On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > > > From: Ben Hutchings
> > > [...]
> > > > > +	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);
> > > > 
> > > > If ret > size something is horridly wrong.
> > > > Silently not updating the callers buffer at all cannot be right.
> > > 
> > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > has the second comparison as well.
> > > 
> > > > > +	kfree(buf);
> > > > > +	return ret;
> > 
> > Since we return what usb_control_msg() told us to return i assume the error code 
> > will be available to anybody who cares.
> > 
> > > > I can't help feeling that it would be better to add a wrapper to
> > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > drop that into all the call sites.
> > > 
> > > It might be.  Right now I'm trying to patch up a bunch of regressions rather 
> > > than argue over an API change.
> > 
> > Right, first thing first.
> > 
> > I am in favor of changing the API, but this should not happen in the stable 
> > releases.  I hope Greg will make up his mind and let us know.
> 
> make up my mind about what?  These are bugs, they should be fixed, I'm not 
> taking a total api change at this point in time, sorry.

Exactly what i said above: " ... shoud not happen in the stable releases".  

Would you consider what David proposed (usb_control_msg_with_malloc()) for 4.11, 
for example?  I for one will use something like that in all my drivers.


cheers,
Petko

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Greg KH]

On Tue, Feb 07, 2017 at 02:53:24PM +0200, Petko Manolov wrote:
> On 17-02-07 11:51:31, Greg KH wrote:
> > On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > > On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > > > > From: Ben Hutchings
> > > > [...]
> > > > > > +	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);
> > > > > 
> > > > > If ret > size something is horridly wrong.
> > > > > Silently not updating the callers buffer at all cannot be right.
> > > > 
> > > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > > has the second comparison as well.
> > > > 
> > > > > > +	kfree(buf);
> > > > > > +	return ret;
> > > 
> > > Since we return what usb_control_msg() told us to return i assume the error code 
> > > will be available to anybody who cares.
> > > 
> > > > > I can't help feeling that it would be better to add a wrapper to
> > > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > > drop that into all the call sites.
> > > > 
> > > > It might be.  Right now I'm trying to patch up a bunch of regressions rather 
> > > > than argue over an API change.
> > > 
> > > Right, first thing first.
> > > 
> > > I am in favor of changing the API, but this should not happen in the stable 
> > > releases.  I hope Greg will make up his mind and let us know.
> > 
> > make up my mind about what?  These are bugs, they should be fixed, I'm not 
> > taking a total api change at this point in time, sorry.
> 
> Exactly what i said above: " ... shoud not happen in the stable releases".  
> 
> Would you consider what David proposed (usb_control_msg_with_malloc()) for 4.11, 
> for example?  I for one will use something like that in all my drivers.

Sure, but you might want to make it a bit smaller of a function name :)

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 14:01:02, Greg KH wrote:
> On Tue, Feb 07, 2017 at 02:53:24PM +0200, Petko Manolov wrote:
> > On 17-02-07 11:51:31, Greg KH wrote:
> > > On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > > > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > > > On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > > > > > From: Ben Hutchings
> > > > > [...]
> > > > > > > +	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);
> > > > > > 
> > > > > > If ret > size something is horridly wrong.
> > > > > > Silently not updating the callers buffer at all cannot be right.
> > > > > 
> > > > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > > > has the second comparison as well.
> > > > > 
> > > > > > > +	kfree(buf);
> > > > > > > +	return ret;
> > > > 
> > > > Since we return what usb_control_msg() told us to return i assume the error code 
> > > > will be available to anybody who cares.
> > > > 
> > > > > > I can't help feeling that it would be better to add a wrapper to
> > > > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > > > drop that into all the call sites.
> > > > > 
> > > > > It might be.  Right now I'm trying to patch up a bunch of regressions rather 
> > > > > than argue over an API change.
> > > > 
> > > > Right, first thing first.
> > > > 
> > > > I am in favor of changing the API, but this should not happen in the stable 
> > > > releases.  I hope Greg will make up his mind and let us know.
> > > 
> > > make up my mind about what?  These are bugs, they should be fixed, I'm not 
> > > taking a total api change at this point in time, sorry.
> > 
> > Exactly what i said above: " ... shoud not happen in the stable releases".  
> > 
> > Would you consider what David proposed (usb_control_msg_with_malloc()) for 4.11, 
> > for example?  I for one will use something like that in all my drivers.
> 
> Sure, but you might want to make it a bit smaller of a function name :)

Whatever i say may be taken as volunteering. :D

Second - i've got really bad taste when naming things.  asdfgl() is a short name 
although not very descriptive. ;)


Seriously, if nobody else step up i'll do my best to come up with a patch in the 
next few days.



cheers,
Petko

RE: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[David Laight]

From: Petko Manolov
> Sent: 07 February 2017 13:21
...
> > > Would you consider what David proposed (usb_control_msg_with_malloc()) for 4.11,
> > > for example?  I for one will use something like that in all my drivers.
> >
> > Sure, but you might want to make it a bit smaller of a function name :)
> 
> Whatever i say may be taken as volunteering. :D
> 
> Second - i've got really bad taste when naming things.  asdfgl() is a short name
> although not very descriptive. ;)
...

Actually, for short control transfers a field in the urb itself could be used
(unless another 8 bytes makes the structure larger).

IIRC for xhci, the 8 byte pointer field can be marked as containing the
actual data - saving the hardware from doing another dma as well.

	David

Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 14:14:30, David Laight wrote:
> From: Petko Manolov
> > Sent: 07 February 2017 13:21
> ...
> > > > Would you consider what David proposed (usb_control_msg_with_malloc()) for 4.11,
> > > > for example?  I for one will use something like that in all my drivers.
> > >
> > > Sure, but you might want to make it a bit smaller of a function name :)
> > 
> > Whatever i say may be taken as volunteering. :D
> > 
> > Second - i've got really bad taste when naming things.  asdfgl() is a short name
> > although not very descriptive. ;)
> ...
> 
> Actually, for short control transfers a field in the urb itself could be used 
> (unless another 8 bytes makes the structure larger).
> 
> IIRC for xhci, the 8 byte pointer field can be marked as containing the actual 
> data - saving the hardware from doing another dma as well.

Unless set up for automatic triggering, 8 byte DMA transfer is an overkill (by 
means of having to program a bunch of IO registers), especially on a 64bit 
machine.

However, not knowing the USB host controllers specifics i'll shut up.


cheers,
Petko

Re: [PATCH net 0/4] Fix on-stack USB buffers

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sat, 4 Feb 2017 16:54:51 +0000

> Allocating USB buffers on the stack is not portable, and no longer
> works on x86_64 (with VMAP_STACK enabled as per default).  This
> series fixes all the instances I could find where USB networking
> drivers do that.

Series applied and queued up for -stable, thanks Ben.

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Steve Calfee]

On Mon, Feb 6, 2017 at 4:51 AM, Petko Manolov <petkan-nPnTwAqkgEqakBO8gow8eQ@public.gmane.org> wrote:
> On 17-02-06 09:28:22, Greg KH wrote:
>> On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
>> > On 17-02-05 01:30:39, Greg KH wrote:
>> > > On Sat, Feb 04, 2017 at 04:56:03PM +0000, Ben Hutchings wrote:
>> > > > Allocating USB buffers on the stack is not portable, and no longer works
>> > > > on x86_64 (with VMAP_STACK enabled as per default).
>> > >
>> > > It's never worked on other platforms, so these should go to the stable
>> > > releases please.
>> >
>> > As far as i know both drivers works fine on other platforms, though I only
>> > tested it on arm and mipsel. ;)
>>
>> It all depends on the arm and mips platforms, the ones that can not DMA from
>> stack memory are the ones that would always fail here (since the 2.2 kernel
>> days).
>
> Seems like most modern SOCs have decent DMA controllers.
>

The real problem is not DMA exactly, it is cache coherency.

X86 has a coherent cache and all the cpu cores watch DMA transfers and
keep the cpu caches up to date.

Most ARMs and MIPS processors have incoherent cache, so DMA can change
memory without the CPU cache updates. CPU cache view of what is in
memory can be different from what was DMAed in, this makes failures
very hard to detect, reproduce and racy.

So all DMA buffers should always be separate allocations from the
stack AND not be embedded in structs. Memory allocations are always at
least cache line aligned, so coherency is not a problem.

Regards, Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net 1/4] pegasus: Use heap buffers for all register access

[Petko Manolov]

On 17-02-07 10:32:16, Steve Calfee wrote:
> On Mon, Feb 6, 2017 at 4:51 AM, Petko Manolov <petkan@nucleusys.com> wrote:
> > On 17-02-06 09:28:22, Greg KH wrote:
> >> On Mon, Feb 06, 2017 at 10:14:44AM +0200, Petko Manolov wrote:
> >> > On 17-02-05 01:30:39, Greg KH wrote:
> >> > > On Sat, Feb 04, 2017 at 04:56:03PM +0000, Ben Hutchings wrote:
> >> > > > Allocating USB buffers on the stack is not portable, and no longer works
> >> > > > on x86_64 (with VMAP_STACK enabled as per default).
> >> > >
> >> > > It's never worked on other platforms, so these should go to the stable
> >> > > releases please.
> >> >
> >> > As far as i know both drivers works fine on other platforms, though I only
> >> > tested it on arm and mipsel. ;)
> >>
> >> It all depends on the arm and mips platforms, the ones that can not DMA 
> >> from stack memory are the ones that would always fail here (since the 2.2 
> >> kernel days).
> >
> > Seems like most modern SOCs have decent DMA controllers.
> >
> 
> The real problem is not DMA exactly, it is cache coherency.
> 
> X86 has a coherent cache and all the cpu cores watch DMA transfers and keep 
> the cpu caches up to date.

Yep, these cpus are more user friendly.

> Most ARMs and MIPS processors have incoherent cache, so DMA can change memory 
> without the CPU cache updates. CPU cache view of what is in memory can be 
> different from what was DMAed in, this makes failures very hard to detect, 
> reproduce and racy.

Except for a very few purposes (like framebuffer memory) one should be crazy to 
use anything but kseg1 for accessing the peripherals.  One of the real problems 
here is the 512MB limit of the uncached segment.  While the DMA controller can 
typically access all available RAM you'll run into the issues that you describe 
with more than that amount of memory.

MIPS like doing things small and simple.  Everybody knows that software can 
compensate for hardware omissions. ;)

> So all DMA buffers should always be separate allocations from the stack AND 
> not be embedded in structs. Memory allocations are always at least cache line 
> aligned, so coherency is not a problem.

I do agree.


cheers,
Petko