Re: [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bitread from uninitialized memory in netlink_broadcast_filtered

[Pekka Enberg]

On Mon, Feb 14, 2011 at 7:35 PM, Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
> Le vendredi 21 janvier 2011 à 09:49 +0200, Pekka Enberg a écrit :
>
>> It actually looks like a bug in SLUB+kmemcheck. The
>> kmemcheck_slab_alloc() call in slab_post_alloc_hook() should use ksize()
>> instead of s->objsize. SLAB seems to do the right thing already. Anyone
>> care to send a patch my way?
>>
>
> Hmm, what do you think of following patch ?
>
> Thanks, and sorry for the delay.

Looks good to me. Christoph, David, any objections to the patch?

> [PATCH] slub: fix kmemcheck calls to match ksize() hints
>
> Recent use of ksize() in network stack (commit ca44ac38 : net: don't
> reallocate skb->head unless the current one hasn't the needed extra size
> or is shared) triggers kmemcheck warnings, because ksize() can return
> more space than kmemcheck is aware of.
>
> Pekka Enberg noticed SLAB+kmemcheck is doing the right thing, while SLUB
> +kmemcheck doesnt.
>
> Bugzilla reference #27212
>
> Reported-by: Christian Casteyde <casteyde.christian@xxxxxxx>
> Suggested-by: Pekka Enberg <penberg@xxxxxxxxxx>
> Signed-off-by: Eric Dumazet <eric.dumazet@xxxxxxxxx>
> CC: David Miller <davem@xxxxxxxxxxxxx>
> CC: Changli Gao <xiaosuo@xxxxxxxxx>
> CC: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>  mm/slub.c |   49 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index e15aa7f..ee0aeb8 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -797,10 +797,34 @@ static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
>        return should_failslab(s->objsize, flags, s->flags);
>  }
>
> +static inline size_t slab_ksize(const struct kmem_cache *s)
> +{
> +#ifdef CONFIG_SLUB_DEBUG
> +       /*
> +        * Debugging requires use of the padding between object
> +        * and whatever may come after it.
> +        */
> +       if (s->flags & (SLAB_RED_ZONE | SLAB_POISON))
> +               return s->objsize;
> +
> +#endif
> +       /*
> +        * If we have the need to store the freelist pointer
> +        * back there or track user information then we can
> +        * only use the space before that information.
> +        */
> +       if (s->flags & (SLAB_DESTROY_BY_RCU | SLAB_STORE_USER))
> +               return s->inuse;
> +       /*
> +        * Else we can use all the padding etc for the allocation
> +        */
> +       return s->size;
> +}
> +
>  static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags, void *object)
>  {
>        flags &= gfp_allowed_mask;
> -       kmemcheck_slab_alloc(s, flags, object, s->objsize);
> +       kmemcheck_slab_alloc(s, flags, object, slab_ksize(s));
>        kmemleak_alloc_recursive(object, s->objsize, 1, s->flags, flags);
>  }
>
> @@ -2696,7 +2720,6 @@ EXPORT_SYMBOL(__kmalloc_node);
>  size_t ksize(const void *object)
>  {
>        struct page *page;
> -       struct kmem_cache *s;
>
>        if (unlikely(object == ZERO_SIZE_PTR))
>                return 0;
> @@ -2707,28 +2730,8 @@ size_t ksize(const void *object)
>                WARN_ON(!PageCompound(page));
>                return PAGE_SIZE << compound_order(page);
>        }
> -       s = page->slab;
>
> -#ifdef CONFIG_SLUB_DEBUG
> -       /*
> -        * Debugging requires use of the padding between object
> -        * and whatever may come after it.
> -        */
> -       if (s->flags & (SLAB_RED_ZONE | SLAB_POISON))
> -               return s->objsize;
> -
> -#endif
> -       /*
> -        * If we have the need to store the freelist pointer
> -        * back there or track user information then we can
> -        * only use the space before that information.
> -        */
> -       if (s->flags & (SLAB_DESTROY_BY_RCU | SLAB_STORE_USER))
> -               return s->inuse;
> -       /*
> -        * Else we can use all the padding etc for the allocation
> -        */
> -       return s->size;
> +       return slab_ksize(page->slab);
>  }
>  EXPORT_SYMBOL(ksize);
>
>
>
> --
> 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/
>
--
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/