LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered
       [not found]     ` <4D393A99.9060104@kernel.org>
@ 2011-02-14 17:35       ` Eric Dumazet
  2011-02-14 19:43         ` David Miller
  2011-02-15  5:48         ` Pekka Enberg
  0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2011-02-14 17:35 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Andrew Morton, netdev, bugzilla-daemon, bugme-daemon,
	casteyde.christian, Changli Gao, Vegard Nossum, David Miller,
	linux-kernel

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.

[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@free.fr>
Suggested-by: Pekka Enberg <penberg@kernel.org>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: David Miller <davem@davemloft.net>
CC: Changli Gao <xiaosuo@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
---
 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);
 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered
  2011-02-14 17:35       ` [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered Eric Dumazet
@ 2011-02-14 19:43         ` David Miller
  2011-02-15  5:48         ` Pekka Enberg
  1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2011-02-14 19:43 UTC (permalink / raw)
  To: eric.dumazet
  Cc: penberg, akpm, netdev, bugzilla-daemon, bugme-daemon,
	casteyde.christian, xiaosuo, vegardno, linux-kernel

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 14 Feb 2011 18:35:22 +0100

> 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.
> 
> [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@free.fr>
> Suggested-by: Pekka Enberg <penberg@kernel.org>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered
  2011-02-14 17:35       ` [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered Eric Dumazet
  2011-02-14 19:43         ` David Miller
@ 2011-02-15  5:48         ` Pekka Enberg
  2011-02-15 16:40           ` Christoph Lameter
  2011-02-15 22:21           ` David Rientjes
  1 sibling, 2 replies; 5+ messages in thread
From: Pekka Enberg @ 2011-02-15  5:48 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Andrew Morton, netdev, bugzilla-daemon, bugme-daemon,
	casteyde.christian, Changli Gao, Vegard Nossum, David Miller,
	linux-kernel, Christoph Lameter, David Rientjes

On Mon, Feb 14, 2011 at 7:35 PM, Eric Dumazet <eric.dumazet@gmail.com> 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@free.fr>
> Suggested-by: Pekka Enberg <penberg@kernel.org>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: David Miller <davem@davemloft.net>
> CC: Changli Gao <xiaosuo@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> ---
>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered
  2011-02-15  5:48         ` Pekka Enberg
@ 2011-02-15 16:40           ` Christoph Lameter
  2011-02-15 22:21           ` David Rientjes
  1 sibling, 0 replies; 5+ messages in thread
From: Christoph Lameter @ 2011-02-15 16:40 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Eric Dumazet, Andrew Morton, netdev, bugzilla-daemon,
	bugme-daemon, casteyde.christian, Changli Gao, Vegard Nossum,
	David Miller, linux-kernel, David Rientjes

On Tue, 15 Feb 2011, Pekka Enberg wrote:

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

My eyes hurt. Is there some way you could use tabs or spaces instead of
these weird symbols?

If the kmemcheck people are fine with checking data beyond the last byte
of the object then its fine with me.

Acked-by: Christoph Lameter <cl@linux.com>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered
  2011-02-15  5:48         ` Pekka Enberg
  2011-02-15 16:40           ` Christoph Lameter
@ 2011-02-15 22:21           ` David Rientjes
  1 sibling, 0 replies; 5+ messages in thread
From: David Rientjes @ 2011-02-15 22:21 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Eric Dumazet, Andrew Morton, netdev, bugzilla-daemon,
	bugme-daemon, casteyde.christian, Changli Gao, Vegard Nossum,
	David Miller, linux-kernel, Christoph Lameter

On Tue, 15 Feb 2011, Pekka Enberg wrote:

> > [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@free.fr>
> > Suggested-by: Pekka Enberg <penberg@kernel.org>
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > CC: David Miller <davem@davemloft.net>
> > CC: Changli Gao <xiaosuo@gmail.com>
> > CC: Andrew Morton <akpm@linux-foundation.org>

Acked-by: David Rientjes <rientjes@google.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-02-15 22:21 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <bug-27212-10286@https.bugzilla.kernel.org/>
     [not found] ` <20110120122549.85863a84.akpm@linux-foundation.org>
     [not found]   ` <1295556085.2613.22.camel@edumazet-laptop>
     [not found]     ` <4D393A99.9060104@kernel.org>
2011-02-14 17:35       ` [Bugme-new] [Bug 27212] New: Warning kmemcheck: Caught 64-bit read from uninitialized memory in netlink_broadcast_filtered Eric Dumazet
2011-02-14 19:43         ` David Miller
2011-02-15  5:48         ` Pekka Enberg
2011-02-15 16:40           ` Christoph Lameter
2011-02-15 22:21           ` David Rientjes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).