Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756053AbcKBP7L (ORCPT ); Wed, 2 Nov 2016 11:59:11 -0400 Received: from mail-yw0-f180.google.com ([209.85.161.180]:33788 "EHLO mail-yw0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755930AbcKBP7H (ORCPT ); Wed, 2 Nov 2016 11:59:07 -0400 MIME-Version: 1.0 In-Reply-To: References: <1477939010-111710-1-git-send-email-thgarnie@google.com> From: Thomas Garnier Date: Wed, 2 Nov 2016 08:59:05 -0700 Message-ID: Subject: Re: [PATCH v2] memcg: Prevent memcg caches to be both OFF_SLAB & OBJFREELIST_SLAB To: David Rientjes Cc: Christoph Lameter , Pekka Enberg , Joonsoo Kim , Andrew Morton , Linux-MM , LKML , Greg Thelen , Vladimir Davydov , Michal Hocko Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4034 Lines: 104 On Mon, Oct 31, 2016 at 4:38 PM, David Rientjes wrote: > On Mon, 31 Oct 2016, Thomas Garnier wrote: > >> While testing OBJFREELIST_SLAB integration with pagealloc, we found a >> bug where kmem_cache(sys) would be created with both CFLGS_OFF_SLAB & >> CFLGS_OBJFREELIST_SLAB. >> >> The original kmem_cache is created early making OFF_SLAB not possible. >> When kmem_cache(sys) is created, OFF_SLAB is possible and if pagealloc >> is enabled it will try to enable it first under certain conditions. >> Given kmem_cache(sys) reuses the original flag, you can have both flags >> at the same time resulting in allocation failures and odd behaviors. >> >> This fix discards allocator specific flags from memcg and ensure >> cache_create cannot be called with them. >> >> Fixes: b03a017bebc4 ("mm/slab: introduce new slab management type, OBJFREELIST_SLAB") >> Signed-off-by: Thomas Garnier >> Signed-off-by: Greg Thelen > > Order of the signoffs is strange, should this have a > > From: Greg Thelen > > in the first line or is this your patch? > Yes, thanks for pointing that out. I will put Greg as owner and myself as tester. That make more sense for this patch. >> --- >> Based on next-20161025 >> --- >> mm/slab.h | 3 +++ >> mm/slab_common.c | 10 ++++++++-- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/mm/slab.h b/mm/slab.h >> index 9653f2e..58be647 100644 >> --- a/mm/slab.h >> +++ b/mm/slab.h >> @@ -144,6 +144,9 @@ static inline unsigned long kmem_cache_flags(unsigned long object_size, >> >> #define CACHE_CREATE_MASK (SLAB_CORE_FLAGS | SLAB_DEBUG_FLAGS | SLAB_CACHE_FLAGS) >> >> +/* Common allocator flags allowed for cache_create. */ >> +#define SLAB_FLAGS_PERMITTED (CACHE_CREATE_MASK | SLAB_KASAN) >> + >> int __kmem_cache_shutdown(struct kmem_cache *); >> void __kmem_cache_release(struct kmem_cache *); >> int __kmem_cache_shrink(struct kmem_cache *, bool); >> diff --git a/mm/slab_common.c b/mm/slab_common.c >> index 71f0b28..01d067c 100644 >> --- a/mm/slab_common.c >> +++ b/mm/slab_common.c >> @@ -329,6 +329,12 @@ static struct kmem_cache *create_cache(const char *name, >> struct kmem_cache *s; >> int err; >> >> + /* Do not allow allocator specific flags */ >> + if (flags & ~SLAB_FLAGS_PERMITTED) { >> + err = -EINVAL; >> + goto out; >> + } >> + > > Why not just flags &= SLAB_FLAGS_PERMITTED if we're concerned about this > like kmem_cache_create does &= CACHE_CREATE_MASK? > Christoph on the first version advised removing invalid flags on the caller and checking they are correct in kmem_cache_create. The memcg path putting the wrong flags is through create_cache but I still used this approach. >> err = -ENOMEM; >> s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL); >> if (!s) >> @@ -533,8 +539,8 @@ void memcg_create_kmem_cache(struct mem_cgroup *memcg, >> >> s = create_cache(cache_name, root_cache->object_size, >> root_cache->size, root_cache->align, >> - root_cache->flags, root_cache->ctor, >> - memcg, root_cache); >> + root_cache->flags & SLAB_FLAGS_PERMITTED, >> + root_cache->ctor, memcg, root_cache); >> /* >> * If we could not create a memcg cache, do not complain, because >> * that's not critical at all as we can always proceed with the root > > This introduces an inconsistency that isn't explained: why is SLAB_KASAN, > the only reason why SLAB_FLAGS_PERMITTED needs to be defined, permitted > for memcg_create_kmem_cache() but not kmem_cache_create()? (If we need to > keep SLAB_FLAGS_PERMITTED around, I think it needs a new name since its a > restriction on the cache, not slab.) The idea was that SLAB_FLAGS_PERMITTED would be all the common flags. SLAB_KASAN was the only one not on CACHE_CREATE_MASK. Christoph: Which approach to do you prefer? -- Thomas