From: Pekka Enberg <penberg@cs.helsinki.fi>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Christoph Lameter <cl@linux.com>,
casteyde.christian@free.fr,
Andrew Morton <akpm@linux-foundation.org>,
netdev@vger.kernel.org, bugzilla-daemon@bugzilla.kernel.org,
bugme-daemon@bugzilla.kernel.org,
Vegard Nossum <vegardno@ifi.uio.no>
Subject: Re: [Bugme-new] [Bug 33502] New: Caught 64-bit read from uninitialized memory in __alloc_skb
Date: Thu, 05 May 2011 09:22:49 +0300 [thread overview]
Message-ID: <4DC24239.7050806@cs.helsinki.fi> (raw)
In-Reply-To: <1304576296.32152.713.camel@edumazet-laptop>
On 5/5/11 9:18 AM, Eric Dumazet wrote:
> Le mercredi 20 avril 2011 à 17:01 +0200, Eric Dumazet a écrit :
>> Le mercredi 20 avril 2011 à 09:42 -0500, Christoph Lameter a écrit :
>>
>>> Avoiding the irq handling yields the savings that improve the fastpath. if
>>> you do both then there is only a regression left. So lets go with
>>> disabling the CMPXCHG_LOCAL.
>>
>> OK, let's do that then.
>>
>> Thanks
>>
>> [PATCH v4] slub: dont use cmpxchg_double if KMEMCHECK or DEBUG_PAGEALLOC
>>
>> Christian Casteyde reported a KMEMCHECK splat in slub code.
>>
>> Problem is now we are lockless and allow IRQ in slab_alloc(), the object
>> we manipulate from freelist can be allocated and freed right before we
>> try to read object->next.
>>
>> Same problem can happen with DEBUG_PAGEALLOC
>>
>> Just dont use cmpxchg_double() if either CONFIG_KMEMCHECK or
>> CONFIG_DEBUG_PAGEALLOC is defined.
>>
>> Reported-by: Christian Casteyde<casteyde.christian@free.fr>
>> Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com>
>> Cc: Andrew Morton<akpm@linux-foundation.org>
>> Cc: Pekka Enberg<penberg@cs.helsinki.fi>
>> Cc: Vegard Nossum<vegardno@ifi.uio.no>
>> Cc: Christoph Lameter<cl@linux.com>
>> ---
>> mm/slub.c | 45 +++++++++++++++++++++++++--------------------
>> 1 files changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 94d2a33..f31ab2c 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1540,7 +1540,12 @@ static void unfreeze_slab(struct kmem_cache *s, struct page *page, int tail)
>> }
>> }
>>
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#if defined(CONFIG_CMPXCHG_LOCAL)&& !defined(CONFIG_KMEMCHECK)&& \
>> + !defined(CONFIG_DEBUG_PAGEALLOC)
>> +#define SLUB_USE_CMPXCHG_DOUBLE
>> +#endif
>> +
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> #ifdef CONFIG_PREEMPT
>> /*
>> * Calculate the next globally unique transaction for disambiguiation
>> @@ -1604,7 +1609,7 @@ static inline void note_cmpxchg_failure(const char *n,
>>
>> void init_kmem_cache_cpus(struct kmem_cache *s)
>> {
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> int cpu;
>>
>> for_each_possible_cpu(cpu)
>> @@ -1643,7 +1648,7 @@ static void deactivate_slab(struct kmem_cache *s, struct kmem_cache_cpu *c)
>> page->inuse--;
>> }
>> c->page = NULL;
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> c->tid = next_tid(c->tid);
>> #endif
>> unfreeze_slab(s, page, tail);
>> @@ -1780,7 +1785,7 @@ static void *__slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>> {
>> void **object;
>> struct page *new;
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> unsigned long flags;
>>
>> local_irq_save(flags);
>> @@ -1819,7 +1824,7 @@ load_freelist:
>> c->node = page_to_nid(c->page);
>> unlock_out:
>> slab_unlock(c->page);
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> c->tid = next_tid(c->tid);
>> local_irq_restore(flags);
>> #endif
>> @@ -1858,7 +1863,7 @@ new_slab:
>> }
>> if (!(gfpflags& __GFP_NOWARN)&& printk_ratelimit())
>> slab_out_of_memory(s, gfpflags, node);
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> local_irq_restore(flags);
>> #endif
>> return NULL;
>> @@ -1887,7 +1892,7 @@ static __always_inline void *slab_alloc(struct kmem_cache *s,
>> {
>> void **object;
>> struct kmem_cache_cpu *c;
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> unsigned long tid;
>> #else
>> unsigned long flags;
>> @@ -1896,7 +1901,7 @@ static __always_inline void *slab_alloc(struct kmem_cache *s,
>> if (slab_pre_alloc_hook(s, gfpflags))
>> return NULL;
>>
>> -#ifndef CONFIG_CMPXCHG_LOCAL
>> +#ifndef SLUB_USE_CMPXCHG_DOUBLE
>> local_irq_save(flags);
>> #else
>> redo:
>> @@ -1910,7 +1915,7 @@ redo:
>> */
>> c = __this_cpu_ptr(s->cpu_slab);
>>
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> /*
>> * The transaction ids are globally unique per cpu and per operation on
>> * a per cpu queue. Thus they can be guarantee that the cmpxchg_double
>> @@ -1927,7 +1932,7 @@ redo:
>> object = __slab_alloc(s, gfpflags, node, addr, c);
>>
>> else {
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> /*
>> * The cmpxchg will only match if there was no additional
>> * operation and if we are on the right processor.
>> @@ -1954,7 +1959,7 @@ redo:
>> stat(s, ALLOC_FASTPATH);
>> }
>>
>> -#ifndef CONFIG_CMPXCHG_LOCAL
>> +#ifndef SLUB_USE_CMPXCHG_DOUBLE
>> local_irq_restore(flags);
>> #endif
>>
>> @@ -2034,7 +2039,7 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
>> {
>> void *prior;
>> void **object = (void *)x;
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> unsigned long flags;
>>
>> local_irq_save(flags);
>> @@ -2070,7 +2075,7 @@ checks_ok:
>>
>> out_unlock:
>> slab_unlock(page);
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> local_irq_restore(flags);
>> #endif
>> return;
>> @@ -2084,7 +2089,7 @@ slab_empty:
>> stat(s, FREE_REMOVE_PARTIAL);
>> }
>> slab_unlock(page);
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> local_irq_restore(flags);
>> #endif
>> stat(s, FREE_SLAB);
>> @@ -2113,7 +2118,7 @@ static __always_inline void slab_free(struct kmem_cache *s,
>> {
>> void **object = (void *)x;
>> struct kmem_cache_cpu *c;
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> unsigned long tid;
>> #else
>> unsigned long flags;
>> @@ -2121,7 +2126,7 @@ static __always_inline void slab_free(struct kmem_cache *s,
>>
>> slab_free_hook(s, x);
>>
>> -#ifndef CONFIG_CMPXCHG_LOCAL
>> +#ifndef SLUB_USE_CMPXCHG_DOUBLE
>> local_irq_save(flags);
>>
>> #else
>> @@ -2136,7 +2141,7 @@ redo:
>> */
>> c = __this_cpu_ptr(s->cpu_slab);
>>
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> tid = c->tid;
>> barrier();
>> #endif
>> @@ -2144,7 +2149,7 @@ redo:
>> if (likely(page == c->page&& c->node != NUMA_NO_NODE)) {
>> set_freepointer(s, object, c->freelist);
>>
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> if (unlikely(!this_cpu_cmpxchg_double(
>> s->cpu_slab->freelist, s->cpu_slab->tid,
>> c->freelist, tid,
>> @@ -2160,7 +2165,7 @@ redo:
>> } else
>> __slab_free(s, page, x, addr);
>>
>> -#ifndef CONFIG_CMPXCHG_LOCAL
>> +#ifndef SLUB_USE_CMPXCHG_DOUBLE
>> local_irq_restore(flags);
>> #endif
>> }
>> @@ -2354,7 +2359,7 @@ static inline int alloc_kmem_cache_cpus(struct kmem_cache *s)
>> BUILD_BUG_ON(PERCPU_DYNAMIC_EARLY_SIZE<
>> SLUB_PAGE_SHIFT * sizeof(struct kmem_cache_cpu));
>>
>> -#ifdef CONFIG_CMPXCHG_LOCAL
>> +#ifdef SLUB_USE_CMPXCHG_DOUBLE
>> /*
>> * Must align to double word boundary for the double cmpxchg instructions
>> * to work.
>>
>
>
> Is anybody taking the patch and sending it upstream ?
Oh, sorry, it got lost in my inbox. I can take care of it.
Pekka
next prev parent reply other threads:[~2011-05-05 6:22 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <bug-33502-10286@https.bugzilla.kernel.org/>
2011-04-18 22:38 ` [Bugme-new] [Bug 33502] New: Caught 64-bit read from uninitialized memory in __alloc_skb Andrew Morton
2011-04-19 2:51 ` Eric Dumazet
2011-04-19 3:09 ` Eric Dumazet
2011-04-19 3:20 ` Eric Dumazet
2011-04-19 17:10 ` Christoph Lameter
2011-04-19 20:17 ` Eric Dumazet
2011-04-19 21:18 ` Christoph Lameter
2011-04-20 5:04 ` Eric Dumazet
2011-04-20 14:04 ` Christoph Lameter
2011-04-20 5:56 ` Pekka Enberg
2011-04-20 6:04 ` Eric Dumazet
2011-04-20 7:45 ` casteyde.christian
2011-04-20 7:49 ` Pekka Enberg
2011-04-20 8:09 ` Eric Dumazet
2011-04-20 8:21 ` Pekka Enberg
2011-04-20 9:07 ` Eric Dumazet
2011-04-20 10:02 ` Eric Dumazet
2011-04-20 14:05 ` Christoph Lameter
2011-04-20 14:26 ` Eric Dumazet
2011-04-20 14:42 ` Christoph Lameter
2011-04-20 15:01 ` Eric Dumazet
2011-04-20 15:15 ` Vegard Nossum
2011-04-20 15:34 ` Eric Dumazet
2011-04-20 15:17 ` Christoph Lameter
2011-04-20 15:30 ` Eric Dumazet
2011-04-20 19:36 ` Christian Casteyde
2011-04-20 19:55 ` Eric Dumazet
2011-04-20 20:32 ` Eric Dumazet
2011-05-05 6:18 ` Eric Dumazet
2011-05-05 6:22 ` Pekka Enberg [this message]
2011-05-05 6:50 ` Eric Dumazet
2011-05-05 18:40 ` Christoph Lameter
2011-05-05 18:48 ` Eric Dumazet
2011-05-05 19:05 ` Christoph Lameter
2011-05-09 19:44 ` Pekka Enberg
2011-05-09 20:04 ` Christoph Lameter
2011-05-09 20:06 ` Pekka Enberg
2011-05-10 8:43 ` Eric Dumazet
2011-05-10 9:47 ` Pekka Enberg
2011-05-10 10:03 ` Eric Dumazet
2011-05-10 10:10 ` Pekka Enberg
2011-05-10 10:03 ` Pekka Enberg
2011-05-10 10:17 ` Eric Dumazet
2011-05-10 10:19 ` Pekka Enberg
2011-05-10 11:52 ` Eric Dumazet
2011-05-10 12:24 ` Vegard Nossum
2011-05-10 16:39 ` Christoph Lameter
2011-05-10 17:14 ` Eric Dumazet
2011-05-10 17:30 ` Christoph Lameter
2011-05-10 17:43 ` Christoph Lameter
2011-05-10 18:05 ` Eric Dumazet
2011-05-10 18:28 ` Christoph Lameter
2011-05-10 19:05 ` Christoph Lameter
2011-05-10 19:32 ` Eric Dumazet
2011-05-10 19:38 ` Christoph Lameter
2011-05-10 20:06 ` Eric Dumazet
2011-05-10 20:33 ` Christoph Lameter
2011-05-10 20:45 ` Eric Dumazet
2011-05-10 21:22 ` Christoph Lameter
2011-05-11 3:12 ` Eric Dumazet
2011-05-12 14:36 ` Christoph Lameter
2011-05-13 21:15 ` [PATCH] slub: Make CONFIG_PAGE_ALLOC work with new fastpath Christoph Lameter
2011-05-13 21:26 ` Eric Dumazet
2011-05-10 18:07 ` [Bugme-new] [Bug 33502] New: Caught 64-bit read from uninitialized memory in __alloc_skb Christoph Lameter
2011-05-10 16:33 ` Christoph Lameter
2011-04-19 17:09 ` Christoph Lameter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DC24239.7050806@cs.helsinki.fi \
--to=penberg@cs.helsinki.fi \
--cc=akpm@linux-foundation.org \
--cc=bugme-daemon@bugzilla.kernel.org \
--cc=bugzilla-daemon@bugzilla.kernel.org \
--cc=casteyde.christian@free.fr \
--cc=cl@linux.com \
--cc=eric.dumazet@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=vegardno@ifi.uio.no \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.