From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pekka Enberg Subject: Re: [PATCH] slub: fix slab_pad_check() and SLAB_DESTROY_BY_RCU Date: Thu, 3 Sep 2009 09:31:01 +0300 Message-ID: <84144f020909022331x2b275aa5n428f88670e0ae8bc@mail.gmail.com> References: <4A87CE60.4020506@gmail.com> <4A896324.3040104@trash.net> <4A9EEF07.5070800@gmail.com> <4A9F1620.2080105@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Zdenek Kabelac , Patrick McHardy , Christoph Lameter , Robin Holt , Linux Kernel Mailing List , Jesper Dangaard Brouer , Linux Netdev List , Netfilter Developers , paulmck@linux.vnet.ibm.com To: Eric Dumazet Return-path: Received: from mail-fx0-f217.google.com ([209.85.220.217]:61945 "EHLO mail-fx0-f217.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754513AbZICGbB convert rfc822-to-8bit (ORCPT ); Thu, 3 Sep 2009 02:31:01 -0400 In-Reply-To: <4A9F1620.2080105@gmail.com> Sender: netfilter-devel-owner@vger.kernel.org List-ID: On Thu, Sep 3, 2009 at 4:04 AM, Eric Dumazet wr= ote: > Zdenek Kabelac a =E9crit : >> >> Well I'm not noticing any ill behavior - also note - rcu_barrier() i= s >> there before the cache is destroyed. >> But as I said - it's just my shot into the dark - which seems to wor= k for me... >> > > Reading again your traces, I do believe there are two bugs in slub > > Maybe not explaining your problem, but worth to fix ! > > Thank you > > [PATCH] slub: fix slab_pad_check() and SLAB_DESTROY_BY_RCU > > When SLAB_POISON is used and slab_pad_check() finds an overwrite of t= he > slab padding, we call restore_bytes() on the whole slab, not only > on the padding. > > kmem_cache_destroy() should call rcu_barrier() *after* kmem_cache_clo= se() > and *before* sysfs_slab_remove() or risk rcu_free_slab() > being called after kmem_cache is deleted (kfreed). > > rmmod nf_conntrack can crash the machine because it has to > kmem_cache_destroy() a SLAB_DESTROY_BY_RCU enabled cache. > > Reported-by: Zdenek Kabelac > Signed-off-by: Eric Dumazet > --- > diff --git a/mm/slub.c b/mm/slub.c > index b9f1491..0ac839f 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -646,7 +646,7 @@ static int slab_pad_check(struct kmem_cache *s, s= truct page *page) > =A0 =A0 =A0 =A0slab_err(s, page, "Padding overwritten. 0x%p-0x%p", fa= ult, end - 1); > =A0 =A0 =A0 =A0print_section("Padding", end - remainder, remainder); > > - =A0 =A0 =A0 restore_bytes(s, "slab padding", POISON_INUSE, start, e= nd); > + =A0 =A0 =A0 restore_bytes(s, "slab padding", POISON_INUSE, end - re= mainder, end); OK, makes sense. > =A0 =A0 =A0 =A0return 0; > =A0} > > @@ -2594,8 +2594,6 @@ static inline int kmem_cache_close(struct kmem_= cache *s) > =A0*/ > =A0void kmem_cache_destroy(struct kmem_cache *s) > =A0{ > - =A0 =A0 =A0 if (s->flags & SLAB_DESTROY_BY_RCU) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 rcu_barrier(); > =A0 =A0 =A0 =A0down_write(&slub_lock); > =A0 =A0 =A0 =A0s->refcount--; > =A0 =A0 =A0 =A0if (!s->refcount) { > @@ -2606,6 +2604,8 @@ void kmem_cache_destroy(struct kmem_cache *s) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"still= has objects.\n", s->name, __func__); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dump_stack(); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (s->flags & SLAB_DESTROY_BY_RCU) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rcu_barrier(); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sysfs_slab_remove(s); > =A0 =A0 =A0 =A0} else > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0up_write(&slub_lock); The rcu_barrier() call was added by this commit: http://git.kernel.org/?p=3Dlinux/kernel/git/torvalds/linux-2.6.git;a=3D= commitdiff;h=3D7ed9f7e5db58c6e8c2b4b738a75d5dcd8e17aad5 I guess we should CC Paul as well. -- To unsubscribe from this list: send the line "unsubscribe netfilter-dev= el" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html