Re: [PATCH v2] scatterlist: prevent invalid free when alloc fails

[Tejun Heo <tj@kernel.org>]

Hello,

On 08/30/2010 05:01 PM, Jeffrey Carlyle wrote:
> When alloc fails, free_table is being called. Depending on the number of
> bytes requested, we determine if we are going to call _get_free_page()
> or kmalloc(). When alloc fails, our math is wrong (due to sg_size - 1),
> and the last buffer is wrongfully assumed to have been allocated by
> kmalloc. Hence, kfree gets called and a panic occurs.
> 
> Signed-off-by: Jeffrey Carlyle <jeff.carlyle@motorola.com>
> Signed-off-by: Olusanya Soyannwo <c23746@motorola.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <jaxboe@fusionio.com>

Supposing it's verified to fix the same issue,

  Acked-by: Tejun Heo <tj@kernel.org>

trivial suggestions below,

> ---
>  lib/scatterlist.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index a5ec428..9bc637f 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -248,8 +248,14 @@ int __sg_alloc_table(struct sg_table *table, unsigned int nents,
>  		left -= sg_size;
>  
>  		sg = alloc_fn(alloc_size, gfp_mask);
> -		if (unlikely(!sg))
> +		if (unlikely(!sg)) {
> +			/*
> +			 * Adjust entry count so that proper free function is
> +			 * used in sg_kfree.
> +			 */

I think it would be better why the adjustment is necessary.  IOW,
something like "Adjust entry count to reflect that the last entry of
the previous table won't be used for linkage.  Without this,
sg_kfree() may get confused."

Thanks.

-- 
tejun