[PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[Ezequiel Garcia]

Currently stk1160_read_reg() uses a stack-allocated char to get the
read control value. This is wrong because usb_control_msg() requires
a kmalloc-ed buffer.

This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
the read value.

While here, let's remove the urb_buf array which was meant for a similar
purpose, but never really used.

Cc: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++-
 drivers/media/usb/stk1160/stk1160.h      |  1 -
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
index 34a26e0..03504dc 100644
--- a/drivers/media/usb/stk1160/stk1160-core.c
+++ b/drivers/media/usb/stk1160/stk1160-core.c
@@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value)
 {
 	int ret;
 	int pipe = usb_rcvctrlpipe(dev->udev, 0);
+	u8 *buf;
 
 	*value = 0;
+
+	buf = kmalloc(sizeof(u8), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 	ret = usb_control_msg(dev->udev, pipe, 0x00,
 			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-			0x00, reg, value, sizeof(u8), HZ);
+			0x00, reg, buf, sizeof(u8), HZ);
 	if (ret < 0) {
 		stk1160_err("read failed on reg 0x%x (%d)\n",
 			reg, ret);
+		kfree(buf);
 		return ret;
 	}
 
+	*value = *buf;
+	kfree(buf);
 	return 0;
 }
 
diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
index 05b05b1..abdea48 100644
--- a/drivers/media/usb/stk1160/stk1160.h
+++ b/drivers/media/usb/stk1160/stk1160.h
@@ -143,7 +143,6 @@ struct stk1160 {
 	int num_alt;
 
 	struct stk1160_isoc_ctl isoc_ctl;
-	char urb_buf[255];	 /* urb control msg buffer */
 
 	/* frame properties */
 	int width;		  /* current frame width */
-- 
1.9.1

Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[Ezequiel Garcia]

On Apr 17, Ezequiel Garcia wrote:
> Currently stk1160_read_reg() uses a stack-allocated char to get the
> read control value. This is wrong because usb_control_msg() requires
> a kmalloc-ed buffer.
> 
> This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
> the read value.
> 
> While here, let's remove the urb_buf array which was meant for a similar
> purpose, but never really used.
> 
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>

Ouch, I forgot to Cc Sander!

Sander, Here's the patch:

https://patchwork.linuxtv.org/patch/23660/

Maybe you can give it a ride and confirm it fixes the warning over there?

Also, have you observed any serious issues caused by this or just the
DMA API debug warning?

> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++-
>  drivers/media/usb/stk1160/stk1160.h      |  1 -
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
> index 34a26e0..03504dc 100644
> --- a/drivers/media/usb/stk1160/stk1160-core.c
> +++ b/drivers/media/usb/stk1160/stk1160-core.c
> @@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value)
>  {
>  	int ret;
>  	int pipe = usb_rcvctrlpipe(dev->udev, 0);
> +	u8 *buf;
>  
>  	*value = 0;
> +
> +	buf = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
>  	ret = usb_control_msg(dev->udev, pipe, 0x00,
>  			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> -			0x00, reg, value, sizeof(u8), HZ);
> +			0x00, reg, buf, sizeof(u8), HZ);
>  	if (ret < 0) {
>  		stk1160_err("read failed on reg 0x%x (%d)\n",
>  			reg, ret);
> +		kfree(buf);
>  		return ret;
>  	}
>  
> +	*value = *buf;
> +	kfree(buf);
>  	return 0;
>  }
>  
> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
> index 05b05b1..abdea48 100644
> --- a/drivers/media/usb/stk1160/stk1160.h
> +++ b/drivers/media/usb/stk1160/stk1160.h
> @@ -143,7 +143,6 @@ struct stk1160 {
>  	int num_alt;
>  
>  	struct stk1160_isoc_ctl isoc_ctl;
> -	char urb_buf[255];	 /* urb control msg buffer */
>  
>  	/* frame properties */
>  	int width;		  /* current frame width */
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[Sander Eikelenboom]

Friday, April 25, 2014, 11:51:53 PM, you wrote:

> On Apr 17, Ezequiel Garcia wrote:
>> Currently stk1160_read_reg() uses a stack-allocated char to get the
>> read control value. This is wrong because usb_control_msg() requires
>> a kmalloc-ed buffer.
>> 
>> This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
>> the read value.
>> 
>> While here, let's remove the urb_buf array which was meant for a similar
>> purpose, but never really used.
>> 
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>

> Ouch, I forgot to Cc Sander!

> Sander, Here's the patch:

> https://patchwork.linuxtv.org/patch/23660/

> Maybe you can give it a ride and confirm it fixes the warning over there?

> Also, have you observed any serious issues caused by this or just the
> DMA API debug warning?

Hi Ezequiel,

Just tested  with your v2 patch for a while and haven't seen the warning again 
:-)

I don't remember for certain if it gave any serious issues .. since i have been 
running with you v1 patch now for a while and it's on a test machine i use 
infrequently.

--
Sander

>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++-
>>  drivers/media/usb/stk1160/stk1160.h      |  1 -
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
>> index 34a26e0..03504dc 100644
>> --- a/drivers/media/usb/stk1160/stk1160-core.c
>> +++ b/drivers/media/usb/stk1160/stk1160-core.c
>> @@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value)
>>  {
>>       int ret;
>>       int pipe = usb_rcvctrlpipe(dev->udev, 0);
>> +     u8 *buf;
>>  
>>       *value = 0;
>> +
>> +     buf = kmalloc(sizeof(u8), GFP_KERNEL);
>> +     if (!buf)
>> +             return -ENOMEM;
>>       ret = usb_control_msg(dev->udev, pipe, 0x00,
>>                       USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>> -                     0x00, reg, value, sizeof(u8), HZ);
>> +                     0x00, reg, buf, sizeof(u8), HZ);
>>       if (ret < 0) {
>>               stk1160_err("read failed on reg 0x%x (%d)\n",
>>                       reg, ret);
>> +             kfree(buf);
>>               return ret;
>>       }
>>  
>> +     *value = *buf;
>> +     kfree(buf);
>>       return 0;
>>  }
>>  
>> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
>> index 05b05b1..abdea48 100644
>> --- a/drivers/media/usb/stk1160/stk1160.h
>> +++ b/drivers/media/usb/stk1160/stk1160.h
>> @@ -143,7 +143,6 @@ struct stk1160 {
>>       int num_alt;
>>  
>>       struct stk1160_isoc_ctl isoc_ctl;
>> -     char urb_buf[255];       /* urb control msg buffer */
>>  
>>       /* frame properties */
>>       int width;                /* current frame width */
>> -- 
>> 1.9.1
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[Hans Verkuil]

Hi Ezequiel,

On 04/17/2014 02:28 PM, Ezequiel Garcia wrote:
> Currently stk1160_read_reg() uses a stack-allocated char to get the
> read control value. This is wrong because usb_control_msg() requires
> a kmalloc-ed buffer.
> 
> This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
> the read value.
> 
> While here, let's remove the urb_buf array which was meant for a similar
> purpose, but never really used.

Rather than allocating and freeing a buffer for every read_reg I would allocate
this buffer in the probe function.

That way this allocation is done only once.

Regards,

	Hans

> 
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++-
>  drivers/media/usb/stk1160/stk1160.h      |  1 -
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
> index 34a26e0..03504dc 100644
> --- a/drivers/media/usb/stk1160/stk1160-core.c
> +++ b/drivers/media/usb/stk1160/stk1160-core.c
> @@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value)
>  {
>  	int ret;
>  	int pipe = usb_rcvctrlpipe(dev->udev, 0);
> +	u8 *buf;
>  
>  	*value = 0;
> +
> +	buf = kmalloc(sizeof(u8), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
>  	ret = usb_control_msg(dev->udev, pipe, 0x00,
>  			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> -			0x00, reg, value, sizeof(u8), HZ);
> +			0x00, reg, buf, sizeof(u8), HZ);
>  	if (ret < 0) {
>  		stk1160_err("read failed on reg 0x%x (%d)\n",
>  			reg, ret);
> +		kfree(buf);
>  		return ret;
>  	}
>  
> +	*value = *buf;
> +	kfree(buf);
>  	return 0;
>  }
>  
> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
> index 05b05b1..abdea48 100644
> --- a/drivers/media/usb/stk1160/stk1160.h
> +++ b/drivers/media/usb/stk1160/stk1160.h
> @@ -143,7 +143,6 @@ struct stk1160 {
>  	int num_alt;
>  
>  	struct stk1160_isoc_ctl isoc_ctl;
> -	char urb_buf[255];	 /* urb control msg buffer */
>  
>  	/* frame properties */
>  	int width;		  /* current frame width */
> 

Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[Ezequiel Garcia]

On 09 May 12:34 PM, Hans Verkuil wrote:
> Hi Ezequiel,
> 
> On 04/17/2014 02:28 PM, Ezequiel Garcia wrote:
> > Currently stk1160_read_reg() uses a stack-allocated char to get the
> > read control value. This is wrong because usb_control_msg() requires
> > a kmalloc-ed buffer.
> > 
> > This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
> > the read value.
> > 
> > While here, let's remove the urb_buf array which was meant for a similar
> > purpose, but never really used.
> 
> Rather than allocating and freeing a buffer for every read_reg I would allocate
> this buffer in the probe function.
> 
> That way this allocation is done only once.
> 

I get your point. I just thought that since the control URBs are only used for
changing the configuration parameters, and this path is scarcely taken, it wasn't
a big deal to allocate it each time.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[Ezequiel Garcia]

Hi Hans,

On 09 May 12:34 PM, Hans Verkuil wrote:
> On 04/17/2014 02:28 PM, Ezequiel Garcia wrote:
> > Currently stk1160_read_reg() uses a stack-allocated char to get the
> > read control value. This is wrong because usb_control_msg() requires
> > a kmalloc-ed buffer.
> > 
> > This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
> > the read value.
> > 
> > While here, let's remove the urb_buf array which was meant for a similar
> > purpose, but never really used.
> 
> Rather than allocating and freeing a buffer for every read_reg I would allocate
> this buffer in the probe function.
> 
> That way this allocation is done only once.
> 

Hm... sorry for being so stubborn, but I've just noticed that having a
shared buffer would require adding a spinlock to protect it, where the current
proposal doesn't need it.

Do you still think that's the right thing to do?

Thanks!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs

[Hans Verkuil]

On 05/17/2014 02:21 PM, Ezequiel Garcia wrote:
> Hi Hans,
> 
> On 09 May 12:34 PM, Hans Verkuil wrote:
>> On 04/17/2014 02:28 PM, Ezequiel Garcia wrote:
>>> Currently stk1160_read_reg() uses a stack-allocated char to get the
>>> read control value. This is wrong because usb_control_msg() requires
>>> a kmalloc-ed buffer.
>>>
>>> This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
>>> the read value.
>>>
>>> While here, let's remove the urb_buf array which was meant for a similar
>>> purpose, but never really used.
>>
>> Rather than allocating and freeing a buffer for every read_reg I would allocate
>> this buffer in the probe function.
>>
>> That way this allocation is done only once.
>>
> 
> Hm... sorry for being so stubborn, but I've just noticed that having a
> shared buffer would require adding a spinlock to protect it, where the current
> proposal doesn't need it.
> 
> Do you still think that's the right thing to do?

I'm still not entirely happy, but I've decided to accept it. It's a bug and it
needs to be fixed. Adding a mutex to protect the datastructure adds only more
complexity and it not really worth the effort.

Regards,

	Hans