[patch] fix BUG: in fw_realloc_buffer

[patch] fix BUG: in fw_realloc_buffer

[Jeff Moyer]

Hi,

The fw_realloc_buffer routine does not handle an increase in buffer size of
more than 4k.  It's not clear to me why it expects that it will only get an
extra 4k of data.  The attached patch modifies fw_realloc_buffer to vmalloc
as much memory as is requested, instead of what we previously had + 4k.

I've tested this on my laptop, which would crash occaisionally on boot
without the patch.  With the patch, it hasn't crashed, but I can't be
certain that this code path is exercised.

Comments are very welcome.

Thanks,

Jeff

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

--- vanilla/drivers/base/firmware_class.c.orig	2006-02-13 15:46:15.000000000 -0500
+++ vanilla/drivers/base/firmware_class.c	2006-02-13 15:46:04.000000000 -0500
@@ -211,18 +211,22 @@ static int
 fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 {
 	u8 *new_data;
+	int new_size = fw_priv->alloc_size;
 
 	if (min_size <= fw_priv->alloc_size)
 		return 0;
 
-	new_data = vmalloc(fw_priv->alloc_size + PAGE_SIZE);
+	while (new_size < min_size)
+		new_size += PAGE_SIZE;
+
+	new_data = vmalloc(new_size);
 	if (!new_data) {
 		printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
 		/* Make sure that we don't keep incomplete data */
 		fw_load_abort(fw_priv);
 		return -ENOMEM;
 	}
-	fw_priv->alloc_size += PAGE_SIZE;
+	fw_priv->alloc_size = new_size;
 	if (fw_priv->fw->data) {
 		memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
 		vfree(fw_priv->fw->data);

Re: [patch] fix BUG: in fw_realloc_buffer

[Andrew Morton]

Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Hi,
> 
> The fw_realloc_buffer routine does not handle an increase in buffer size of
> more than 4k.  It's not clear to me why it expects that it will only get an
> extra 4k of data.  The attached patch modifies fw_realloc_buffer to vmalloc
> as much memory as is requested, instead of what we previously had + 4k.
> 
> I've tested this on my laptop, which would crash occaisionally on boot
> without the patch.  With the patch, it hasn't crashed, but I can't be
> certain that this code path is exercised.
> 
> Comments are very welcome.
> 
> Thanks,
> 
> Jeff
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> 
> --- vanilla/drivers/base/firmware_class.c.orig	2006-02-13 15:46:15.000000000 -0500
> +++ vanilla/drivers/base/firmware_class.c	2006-02-13 15:46:04.000000000 -0500
> @@ -211,18 +211,22 @@ static int
>  fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
>  {
>  	u8 *new_data;
> +	int new_size = fw_priv->alloc_size;
>  
>  	if (min_size <= fw_priv->alloc_size)
>  		return 0;
>  
> -	new_data = vmalloc(fw_priv->alloc_size + PAGE_SIZE);
> +	while (new_size < min_size)
> +		new_size += PAGE_SIZE;
> +
> +	new_data = vmalloc(new_size);
>  	if (!new_data) {
>  		printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
>  		/* Make sure that we don't keep incomplete data */
>  		fw_load_abort(fw_priv);
>  		return -ENOMEM;
>  	}
> -	fw_priv->alloc_size += PAGE_SIZE;
> +	fw_priv->alloc_size = new_size;
>  	if (fw_priv->fw->data) {
>  		memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
>  		vfree(fw_priv->fw->data);

A little bit neater this way, I think?

--- devel/drivers/base/firmware_class.c~firmware-fix-bug-in-fw_realloc_buffer	2006-02-13 14:45:52.000000000 -0800
+++ devel-akpm/drivers/base/firmware_class.c	2006-02-13 14:52:05.000000000 -0800
@@ -211,18 +211,20 @@ static int
 fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 {
 	u8 *new_data;
+	int new_size = fw_priv->alloc_size;
 
 	if (min_size <= fw_priv->alloc_size)
 		return 0;
 
-	new_data = vmalloc(fw_priv->alloc_size + PAGE_SIZE);
+	new_size = ALIGN(min_size, PAGE_SIZE);
+	new_data = vmalloc(new_size);
 	if (!new_data) {
 		printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
 		/* Make sure that we don't keep incomplete data */
 		fw_load_abort(fw_priv);
 		return -ENOMEM;
 	}
-	fw_priv->alloc_size += PAGE_SIZE;
+	fw_priv->alloc_size = new_size;
 	if (fw_priv->fw->data) {
 		memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
 		vfree(fw_priv->fw->data);
_

Re: [patch] fix BUG: in fw_realloc_buffer

[Jeff Moyer]

==> Regarding Re: [patch] fix BUG: in fw_realloc_buffer; Andrew Morton <akpm@osdl.org> adds:

akpm> Jeff Moyer <jmoyer@redhat.com> wrote:
>> 
>> Hi,
>> 
>> The fw_realloc_buffer routine does not handle an increase in buffer size of
>> more than 4k.  It's not clear to me why it expects that it will only get an
>> extra 4k of data.  The attached patch modifies fw_realloc_buffer to vmalloc
>> as much memory as is requested, instead of what we previously had + 4k.
>> 
>> I've tested this on my laptop, which would crash occaisionally on boot
>> without the patch.  With the patch, it hasn't crashed, but I can't be
>> certain that this code path is exercised.
>> 
>> Comments are very welcome.
>> 
[snip]

akpm> A little bit neater this way, I think?

> --- devel/drivers/base/firmware_class.c~firmware-fix-bug-in-fw_realloc_buffer	2006-02-13 14:45:52.000000000 -0800
> +++ devel-akpm/drivers/base/firmware_class.c	2006-02-13 14:52:05.000000000 -0800
> @@ -211,18 +211,20 @@ static int
>  fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
>  {
>  	u8 *new_data;
> +	int new_size = fw_priv->alloc_size;

>  	if (min_size <= fw_priv->alloc_size)
>  		return 0;

> -	new_data = vmalloc(fw_priv->alloc_size + PAGE_SIZE);
> +	new_size = ALIGN(min_size, PAGE_SIZE);
> +	new_data = vmalloc(new_size);
>  	if (!new_data) {
>  		printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
>  		/* Make sure that we don't keep incomplete data */
>  		fw_load_abort(fw_priv);
>  		return -ENOMEM;
>  	}
> -	fw_priv->alloc_size += PAGE_SIZE;
> +	fw_priv->alloc_size = new_size;
>  	if (fw_priv->fw->data) {
>  		memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
>  		vfree(fw_priv->fw->data);
> _

Well, I wasn't sure that you would only need to increase by a PAGE.  If you
only need to account for page_size + alignment, then yes, this is better.
It simply wasn't clear to me that this is how we are called.  If I'm not
mistaken, this is the write routine for a file in sysfs.  If that is the
case, why should we assume that writes are broken up into PAGE_SIZE chunks?

Thanks,

Jeff

Re: [patch] fix BUG: in fw_realloc_buffer

[Jeff Moyer]

==> Regarding Re: [patch] fix BUG: in fw_realloc_buffer; Jeff Moyer <jmoyer@redhat.com> adds:

==> Regarding Re: [patch] fix BUG: in fw_realloc_buffer; Andrew Morton <akpm@osdl.org> adds:
akpm> Jeff Moyer <jmoyer@redhat.com> wrote:
>>> 
>>> Hi,
>>> 
>>> The fw_realloc_buffer routine does not handle an increase in buffer size of
>>> more than 4k.  It's not clear to me why it expects that it will only get an
>>> extra 4k of data.  The attached patch modifies fw_realloc_buffer to vmalloc
>>> as much memory as is requested, instead of what we previously had + 4k.
>>> 
>>> I've tested this on my laptop, which would crash occaisionally on boot
>>> without the patch.  With the patch, it hasn't crashed, but I can't be
>>> certain that this code path is exercised.
>>> 
>>> Comments are very welcome.
>>> 
jmoyer> [snip]

akpm> A little bit neater this way, I think?

>> --- devel/drivers/base/firmware_class.c~firmware-fix-bug-in-fw_realloc_buffer	2006-02-13 14:45:52.000000000 -0800
>> +++ devel-akpm/drivers/base/firmware_class.c	2006-02-13 14:52:05.000000000 -0800
>> @@ -211,18 +211,20 @@ static int
>> fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
>> {
>> u8 *new_data;
>> +	int new_size = fw_priv->alloc_size;

>> if (min_size <= fw_priv->alloc_size)
>> return 0;

>> -	new_data = vmalloc(fw_priv->alloc_size + PAGE_SIZE);
>> +	new_size = ALIGN(min_size, PAGE_SIZE);
>> +	new_data = vmalloc(new_size);
>> if (!new_data) {
>> printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
>> /* Make sure that we don't keep incomplete data */
>> fw_load_abort(fw_priv);
>> return -ENOMEM;
>> }
>> -	fw_priv->alloc_size += PAGE_SIZE;
>> +	fw_priv->alloc_size = new_size;
>> if (fw_priv->fw->data) {
>> memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
>> vfree(fw_priv->fw->data);
>> _

jmoyer> Well, I wasn't sure that you would only need to increase by a PAGE.  If you
jmoyer> only need to account for page_size + alignment, then yes, this is better.
jmoyer> It simply wasn't clear to me that this is how we are called.  If I'm not
jmoyer> mistaken, this is the write routine for a file in sysfs.  If that is the
jmoyer> case, why should we assume that writes are broken up into PAGE_SIZE chunks?

Doh, I wasn't looking close enough at this.  Yes, your fix is what was
intended.  Thanks.

-Jeff

Re: [patch] fix BUG: in fw_realloc_buffer

[Andrew Morton]

Jeff Moyer <jmoyer@redhat.com> wrote:
>
> akpm> A little bit neater this way, I think?
> 
> > --- devel/drivers/base/firmware_class.c~firmware-fix-bug-in-fw_realloc_buffer	2006-02-13 14:45:52.000000000 -0800
> > +++ devel-akpm/drivers/base/firmware_class.c	2006-02-13 14:52:05.000000000 -0800
> > @@ -211,18 +211,20 @@ static int
> >  fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
> >  {
> >  	u8 *new_data;
> > +	int new_size = fw_priv->alloc_size;
> 
> >  	if (min_size <= fw_priv->alloc_size)
> >  		return 0;
> 
> > -	new_data = vmalloc(fw_priv->alloc_size + PAGE_SIZE);
> > +	new_size = ALIGN(min_size, PAGE_SIZE);
> > +	new_data = vmalloc(new_size);
> >  	if (!new_data) {
> >  		printk(KERN_ERR "%s: unable to alloc buffer\n", __FUNCTION__);
> >  		/* Make sure that we don't keep incomplete data */
> >  		fw_load_abort(fw_priv);
> >  		return -ENOMEM;
> >  	}
> > -	fw_priv->alloc_size += PAGE_SIZE;
> > +	fw_priv->alloc_size = new_size;
> >  	if (fw_priv->fw->data) {
> >  		memcpy(new_data, fw_priv->fw->data, fw_priv->fw->size);
> >  		vfree(fw_priv->fw->data);
> > _
> 
> Well, I wasn't sure that you would only need to increase by a PAGE.  If you
> only need to account for page_size + alignment, then yes, this is better.
> It simply wasn't clear to me that this is how we are called.  If I'm not
> mistaken, this is the write routine for a file in sysfs.  If that is the
> case, why should we assume that writes are broken up into PAGE_SIZE chunks?
> 

hm?  The code's equivanent, I think.  ->alloc_size is always a multiple of
PAGE_SIZE and the ALIGN makes new_size the next multiple of PAGE_SIZE which
is >= min_size.