Re: [patch] fix BUG: in fw_realloc_buffer

[Jeff Moyer]

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

==> Regarding Re: [patch] fix BUG: in fw_realloc_buffer; Andrew Morton <akpm@xxxxxxxx> adds:
akpm> Jeff Moyer <jmoyer@xxxxxxxxxx> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/