From: Vladimir Davydov <vdavydov@parallels.com> To: Christoph Lameter <cl@gentwo.org> Cc: <akpm@linux-foundation.org>, <iamjoonsoo.kim@lge.com>, <rientjes@google.com>, <penberg@kernel.org>, <hannes@cmpxchg.org>, <mhocko@suse.cz>, <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org> Subject: Re: [PATCH -mm v2 5/8] slub: make slab_free non-preemptable Date: Mon, 9 Jun 2014 16:52:13 +0400 [thread overview] Message-ID: <20140609125211.GA32192@esperanza> (raw) In-Reply-To: <alpine.DEB.2.10.1406060942160.32229@gentwo.org> On Fri, Jun 06, 2014 at 09:46:57AM -0500, Christoph Lameter wrote: > On Fri, 6 Jun 2014, Vladimir Davydov wrote: > > > This patch makes SLUB's implementation of kmem_cache_free > > non-preemptable. As a result, synchronize_sched() will work as a barrier > > against kmem_cache_free's in flight, so that issuing it before cache > > destruction will protect us against the use-after-free. > > > Subject: slub: reenable preemption before the freeing of slabs from slab_free > > I would prefer to call the page allocator with preemption enabled if possible. > > Signed-off-by: Christoph Lameter <cl@linux.com> > > Index: linux/mm/slub.c > =================================================================== > --- linux.orig/mm/slub.c 2014-05-29 11:45:32.065859887 -0500 > +++ linux/mm/slub.c 2014-06-06 09:45:12.822480834 -0500 > @@ -1998,6 +1998,7 @@ > if (n) > spin_unlock(&n->list_lock); > > + preempt_enable(); The whole function (unfreeze_partials) is currently called with irqs off, so this is effectively a no-op. I guess we can restore irqs here though. > while (discard_page) { > page = discard_page; > discard_page = discard_page->next; > @@ -2006,6 +2007,7 @@ > discard_slab(s, page); If we just freed the last slab of the cache and then get preempted (suppose we restored irqs above), nothing will prevent the cache from destruction, which may result in use-after-free below. We need to be more cautious if we want to call for page allocator with preemption and irqs on. However, I still don't understand what's the point in it. We *already* call discard_slab with irqs disabled, which is harder, and it haven't caused any problems AFAIK. Moreover, even if we enabled preemption/irqs, it wouldn't guarantee that discard_slab would always be called with preemption/irqs on, because the whole function - I mean kmem_cache_free - can be called with preemption/irqs disabled. So my point it would only complicate the code. Thanks. > stat(s, FREE_SLAB); > } > + preempt_disable(); > #endif > } >
WARNING: multiple messages have this Message-ID (diff)
From: Vladimir Davydov <vdavydov@parallels.com> To: Christoph Lameter <cl@gentwo.org> Cc: akpm@linux-foundation.org, iamjoonsoo.kim@lge.com, rientjes@google.com, penberg@kernel.org, hannes@cmpxchg.org, mhocko@suse.cz, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH -mm v2 5/8] slub: make slab_free non-preemptable Date: Mon, 9 Jun 2014 16:52:13 +0400 [thread overview] Message-ID: <20140609125211.GA32192@esperanza> (raw) In-Reply-To: <alpine.DEB.2.10.1406060942160.32229@gentwo.org> On Fri, Jun 06, 2014 at 09:46:57AM -0500, Christoph Lameter wrote: > On Fri, 6 Jun 2014, Vladimir Davydov wrote: > > > This patch makes SLUB's implementation of kmem_cache_free > > non-preemptable. As a result, synchronize_sched() will work as a barrier > > against kmem_cache_free's in flight, so that issuing it before cache > > destruction will protect us against the use-after-free. > > > Subject: slub: reenable preemption before the freeing of slabs from slab_free > > I would prefer to call the page allocator with preemption enabled if possible. > > Signed-off-by: Christoph Lameter <cl@linux.com> > > Index: linux/mm/slub.c > =================================================================== > --- linux.orig/mm/slub.c 2014-05-29 11:45:32.065859887 -0500 > +++ linux/mm/slub.c 2014-06-06 09:45:12.822480834 -0500 > @@ -1998,6 +1998,7 @@ > if (n) > spin_unlock(&n->list_lock); > > + preempt_enable(); The whole function (unfreeze_partials) is currently called with irqs off, so this is effectively a no-op. I guess we can restore irqs here though. > while (discard_page) { > page = discard_page; > discard_page = discard_page->next; > @@ -2006,6 +2007,7 @@ > discard_slab(s, page); If we just freed the last slab of the cache and then get preempted (suppose we restored irqs above), nothing will prevent the cache from destruction, which may result in use-after-free below. We need to be more cautious if we want to call for page allocator with preemption and irqs on. However, I still don't understand what's the point in it. We *already* call discard_slab with irqs disabled, which is harder, and it haven't caused any problems AFAIK. Moreover, even if we enabled preemption/irqs, it wouldn't guarantee that discard_slab would always be called with preemption/irqs on, because the whole function - I mean kmem_cache_free - can be called with preemption/irqs disabled. So my point it would only complicate the code. Thanks. > stat(s, FREE_SLAB); > } > + preempt_disable(); > #endif > } > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2014-06-09 12:52 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-06-06 13:22 [PATCH -mm v2 0/8] memcg/slab: reintroduce dead cache self-destruction Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 1/8] memcg: cleanup memcg_cache_params refcnt usage Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 2/8] memcg: destroy kmem caches when last slab is freed Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 3/8] memcg: mark caches that belong to offline memcgs as dead Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-10 7:48 ` Joonsoo Kim 2014-06-10 7:48 ` Joonsoo Kim 2014-06-10 10:06 ` Vladimir Davydov 2014-06-10 10:06 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 4/8] slub: don't fail kmem_cache_shrink if slab placement optimization fails Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 5/8] slub: make slab_free non-preemptable Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 14:46 ` Christoph Lameter 2014-06-06 14:46 ` Christoph Lameter 2014-06-09 12:52 ` Vladimir Davydov [this message] 2014-06-09 12:52 ` Vladimir Davydov 2014-06-09 13:52 ` Christoph Lameter 2014-06-09 13:52 ` Christoph Lameter 2014-06-12 6:58 ` Joonsoo Kim 2014-06-12 6:58 ` Joonsoo Kim 2014-06-12 10:03 ` Vladimir Davydov 2014-06-12 10:03 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 6/8] memcg: wait for kfree's to finish before destroying cache Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 7/8] slub: make dead memcg caches discard free slabs immediately Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 14:48 ` Christoph Lameter 2014-06-06 14:48 ` Christoph Lameter 2014-06-10 8:09 ` Joonsoo Kim 2014-06-10 8:09 ` Joonsoo Kim 2014-06-10 10:09 ` Vladimir Davydov 2014-06-10 10:09 ` Vladimir Davydov 2014-06-06 13:22 ` [PATCH -mm v2 8/8] slab: " Vladimir Davydov 2014-06-06 13:22 ` Vladimir Davydov 2014-06-06 14:52 ` Christoph Lameter 2014-06-06 14:52 ` Christoph Lameter 2014-06-09 13:04 ` Vladimir Davydov 2014-06-09 13:04 ` Vladimir Davydov 2014-06-10 7:43 ` Joonsoo Kim 2014-06-10 7:43 ` Joonsoo Kim 2014-06-10 10:03 ` Vladimir Davydov 2014-06-10 10:03 ` Vladimir Davydov 2014-06-10 14:26 ` Christoph Lameter 2014-06-10 14:26 ` Christoph Lameter 2014-06-10 15:18 ` Vladimir Davydov 2014-06-10 15:18 ` Vladimir Davydov 2014-06-11 8:11 ` Joonsoo Kim 2014-06-11 8:11 ` Joonsoo Kim 2014-06-11 21:24 ` Vladimir Davydov 2014-06-11 21:24 ` Vladimir Davydov 2014-06-12 6:53 ` Joonsoo Kim 2014-06-12 6:53 ` Joonsoo Kim 2014-06-12 10:02 ` Vladimir Davydov 2014-06-12 10:02 ` Vladimir Davydov 2014-06-13 16:34 ` Christoph Lameter 2014-06-13 16:34 ` 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=20140609125211.GA32192@esperanza \ --to=vdavydov@parallels.com \ --cc=akpm@linux-foundation.org \ --cc=cl@gentwo.org \ --cc=hannes@cmpxchg.org \ --cc=iamjoonsoo.kim@lge.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.cz \ --cc=penberg@kernel.org \ --cc=rientjes@google.com \ /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: linkBe 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.