[PATCH v2 0/2] Fix __GFP_ZERO vs constructor

[PATCH v2 0/2] Fix __GFP_ZERO vs constructor

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

v1->v2:
 - Added review/ack tags (thanks!)
 - Switched the order of the patches
 - Reworded commit message of the patch which actually fixes the bug
 - Moved slab debug patches under CONFIG_DEBUG_VM to check _every_
   allocation and added checks in the slowpath for the allocations which
   end up allocating a page.

Matthew Wilcox (2):
  Fix NULL pointer in page_cache_tree_insert
  slab: __GFP_ZERO is incompatible with a constructor

 mm/filemap.c | 9 ++++-----
 mm/slab.c    | 7 ++++---
 mm/slab.h    | 7 +++++++
 mm/slob.c    | 4 +++-
 mm/slub.c    | 5 +++--
 5 files changed, 21 insertions(+), 11 deletions(-)

-- 
2.16.3

[PATCH v2 1/2] Fix NULL pointer in page_cache_tree_insert

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

f2fs specifies the __GFP_ZERO flag for allocating some of its pages.
Unfortunately, the page cache also uses the mapping's GFP flags for
allocating radix tree nodes.  It always masked off the __GFP_HIGHMEM
flag, and masks off __GFP_ZERO in some paths, but not all.  That causes
radix tree nodes to be allocated with a NULL list_head, which causes
backtraces like:

[<ffffff80086f4de0>] __list_del_entry+0x30/0xd0
[<ffffff8008362018>] list_lru_del+0xac/0x1ac
[<ffffff800830f04c>] page_cache_tree_insert+0xd8/0x110

The __GFP_DMA and __GFP_DMA32 flags would also be able to sneak through
if they are ever used.  Fix them all by using GFP_RECLAIM_MASK at the
innermost location, and remove it from earlier in the callchain.

Fixes: 449dd6984d0e ("mm: keep page cache radix tree nodes in check")
Reported-by: Chris Fries <cfries@google.com>
Debugged-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Michal Hocko <mhocko@suse.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org
---
 mm/filemap.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index c2147682f4c3..1a4bfc5ed3dc 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -785,7 +785,7 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
 	VM_BUG_ON_PAGE(!PageLocked(new), new);
 	VM_BUG_ON_PAGE(new->mapping, new);
 
-	error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
+	error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK);
 	if (!error) {
 		struct address_space *mapping = old->mapping;
 		void (*freepage)(struct page *);
@@ -841,7 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
 			return error;
 	}
 
-	error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
+	error = radix_tree_maybe_preload(gfp_mask & GFP_RECLAIM_MASK);
 	if (error) {
 		if (!huge)
 			mem_cgroup_cancel_charge(page, memcg, false);
@@ -1574,8 +1574,7 @@ struct page *pagecache_get_page(struct address_space *mapping, pgoff_t offset,
 		if (fgp_flags & FGP_ACCESSED)
 			__SetPageReferenced(page);
 
-		err = add_to_page_cache_lru(page, mapping, offset,
-				gfp_mask & GFP_RECLAIM_MASK);
+		err = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
 		if (unlikely(err)) {
 			put_page(page);
 			page = NULL;
@@ -2378,7 +2377,7 @@ static int page_cache_read(struct file *file, pgoff_t offset, gfp_t gfp_mask)
 		if (!page)
 			return -ENOMEM;
 
-		ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask & GFP_KERNEL);
+		ret = add_to_page_cache_lru(page, mapping, offset, gfp_mask);
 		if (ret == 0)
 			ret = mapping->a_ops->readpage(file, page);
 		else if (ret == -EEXIST)
-- 
2.16.3

[PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

__GFP_ZERO requests that the object be initialised to all-zeroes,
while the purpose of a constructor is to initialise an object to a
particular pattern.  We cannot do both.  Add a warning to catch any
users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
a constructor.

Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slab.c | 7 ++++---
 mm/slab.h | 7 +++++++
 mm/slob.c | 4 +++-
 mm/slub.c | 5 +++--
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 58c8cecc26ab..9ad85fd9fca8 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2661,6 +2661,7 @@ static struct page *cache_grow_begin(struct kmem_cache *cachep,
 				invalid_mask, &invalid_mask, flags, &flags);
 		dump_stack();
 	}
+	BUG_ON(cachep->ctor && (flags & __GFP_ZERO));
 	local_flags = flags & (GFP_CONSTRAINT_MASK|GFP_RECLAIM_MASK);
 
 	check_irq_off();
@@ -3325,7 +3326,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	local_irq_restore(save_flags);
 	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
 
-	if (unlikely(flags & __GFP_ZERO) && ptr)
+	if (unlikely(flags & __GFP_ZERO) && ptr && slab_no_ctor(cachep))
 		memset(ptr, 0, cachep->object_size);
 
 	slab_post_alloc_hook(cachep, flags, 1, &ptr);
@@ -3382,7 +3383,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
 	objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
 	prefetchw(objp);
 
-	if (unlikely(flags & __GFP_ZERO) && objp)
+	if (unlikely(flags & __GFP_ZERO) && objp && slab_no_ctor(cachep))
 		memset(objp, 0, cachep->object_size);
 
 	slab_post_alloc_hook(cachep, flags, 1, &objp);
@@ -3589,7 +3590,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
 	cache_alloc_debugcheck_after_bulk(s, flags, size, p, _RET_IP_);
 
 	/* Clear memory outside IRQ disabled section */
-	if (unlikely(flags & __GFP_ZERO))
+	if (unlikely(flags & __GFP_ZERO) && slab_no_ctor(s))
 		for (i = 0; i < size; i++)
 			memset(p[i], 0, s->object_size);
 
diff --git a/mm/slab.h b/mm/slab.h
index 3cd4677953c6..896818c7b30a 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -515,6 +515,13 @@ static inline void dump_unreclaimable_slab(void)
 
 void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr);
 
+static inline bool slab_no_ctor(struct kmem_cache *s)
+{
+	if (IS_ENABLED(CONFIG_DEBUG_VM))
+		return !WARN_ON_ONCE(s->ctor);
+	return true;
+}
+
 #ifdef CONFIG_SLAB_FREELIST_RANDOM
 int cache_random_seq_create(struct kmem_cache *cachep, unsigned int count,
 			gfp_t gfp);
diff --git a/mm/slob.c b/mm/slob.c
index 1a46181b675c..958173fd7c24 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -556,8 +556,10 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
 					    flags, node);
 	}
 
-	if (b && c->ctor)
+	if (b && c->ctor) {
+		WARN_ON_ONCE(flags & __GFP_ZERO);
 		c->ctor(b);
+	}
 
 	kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
 	return b;
diff --git a/mm/slub.c b/mm/slub.c
index a28488643603..9f8f38a552e5 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1576,6 +1576,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 
 	if (gfpflags_allow_blocking(flags))
 		local_irq_enable();
+	BUG_ON(s->ctor && (flags & __GFP_ZERO));
 
 	flags |= s->allocflags;
 
@@ -2725,7 +2726,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
 		stat(s, ALLOC_FASTPATH);
 	}
 
-	if (unlikely(gfpflags & __GFP_ZERO) && object)
+	if (unlikely(gfpflags & __GFP_ZERO) && object && slab_no_ctor(s))
 		memset(object, 0, s->object_size);
 
 	slab_post_alloc_hook(s, gfpflags, 1, &object);
@@ -3149,7 +3150,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
 	local_irq_enable();
 
 	/* Clear memory outside IRQ disabled fastpath loop */
-	if (unlikely(flags & __GFP_ZERO)) {
+	if (unlikely(flags & __GFP_ZERO) && slab_no_ctor(s)) {
 		int j;
 
 		for (j = 0; j < i; j++)
-- 
2.16.3

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Michal Hocko]

On Tue 10-04-18 23:03:20, Matthew Wilcox wrote:
> diff --git a/mm/slab.c b/mm/slab.c
> index 58c8cecc26ab..9ad85fd9fca8 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2661,6 +2661,7 @@ static struct page *cache_grow_begin(struct kmem_cache *cachep,
>  				invalid_mask, &invalid_mask, flags, &flags);
>  		dump_stack();
>  	}
> +	BUG_ON(cachep->ctor && (flags & __GFP_ZERO));

NAK. We really do not want to blow the whole kernel just because
somebody is doing something stupid. Make it WARN_ON_ONCE and fix up the
flag.

> +static inline bool slab_no_ctor(struct kmem_cache *s)
> +{
> +	if (IS_ENABLED(CONFIG_DEBUG_VM))
> +		return !WARN_ON_ONCE(s->ctor);
> +	return true;
> +}

I do realize that you want to keep the hotpath without additional checks
but if for nothing else this is a really bad misnomer.
debug_slab_no_ctor()? I can clearly see how somebody uses this blindly
for a different purpose.
[...]
> diff --git a/mm/slub.c b/mm/slub.c
> index a28488643603..9f8f38a552e5 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1576,6 +1576,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>  
>  	if (gfpflags_allow_blocking(flags))
>  		local_irq_enable();
> +	BUG_ON(s->ctor && (flags & __GFP_ZERO));

No no on this as well.

Othe than that. Once those are fixed, feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Tue, 10 Apr 2018, Matthew Wilcox wrote:

> diff --git a/mm/slab.h b/mm/slab.h
> index 3cd4677953c6..896818c7b30a 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -515,6 +515,13 @@ static inline void dump_unreclaimable_slab(void)
>
>  void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr);
>
> +static inline bool slab_no_ctor(struct kmem_cache *s)
> +{
> +	if (IS_ENABLED(CONFIG_DEBUG_VM))
> +		return !WARN_ON_ONCE(s->ctor);
> +	return true;
> +}
> +
>  #ifdef CONFIG_SLAB_FREELIST_RANDOM
>  int cache_random_seq_create(struct kmem_cache *cachep, unsigned int count,
>  			gfp_t gfp);

Move that to mm/slab.c? Debugging is runtime enabled with SLUB not compile
time as with SLAB.

> +++ b/mm/slub.c
> @@ -2725,7 +2726,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
>  		stat(s, ALLOC_FASTPATH);
>  	}
>
> -	if (unlikely(gfpflags & __GFP_ZERO) && object)
> +	if (unlikely(gfpflags & __GFP_ZERO) && object && slab_no_ctor(s))
>  		memset(object, 0, s->object_size);
>
>  	slab_post_alloc_hook(s, gfpflags, 1, &object);

Please put this in a code path that is enabled by specifying

slub_debug

on the kernel command line.

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Wed, Apr 11, 2018 at 08:44:23AM -0500, Christopher Lameter wrote:
> > +++ b/mm/slub.c
> > @@ -2725,7 +2726,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> >  		stat(s, ALLOC_FASTPATH);
> >  	}
> >
> > -	if (unlikely(gfpflags & __GFP_ZERO) && object)
> > +	if (unlikely(gfpflags & __GFP_ZERO) && object && slab_no_ctor(s))
> >  		memset(object, 0, s->object_size);
> >
> >  	slab_post_alloc_hook(s, gfpflags, 1, &object);
> 
> Please put this in a code path that is enabled by specifying
> 
> slub_debug
> 
> on the kernel command line.

I don't understand.  First, I had:

	if (unlikely(gfpflags & __GFP_ZERO) && object && !WARN_ON_ONCE(s->ctor))

and you didn't like that because it was putting checking into a (semi)fast
path.  Now you want me to add a check for slub_debug somewhere?  I dont
see an existing one I can leverage that will hit on every allocation.
Perhaps I'm missing something.

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Wed, 11 Apr 2018, Matthew Wilcox wrote:

> > >  	slab_post_alloc_hook(s, gfpflags, 1, &object);
> >
> > Please put this in a code path that is enabled by specifying
> >
> > slub_debug
> >
> > on the kernel command line.
>
> I don't understand.  First, I had:
>
> 	if (unlikely(gfpflags & __GFP_ZERO) && object && !WARN_ON_ONCE(s->ctor))
>
> and you didn't like that because it was putting checking into a (semi)fast
> path.  Now you want me to add a check for slub_debug somewhere?  I dont
> see an existing one I can leverage that will hit on every allocation.
> Perhaps I'm missing something.

The WARN_ON is only enabled when you configure and build the kernel with
debugging enabled (CONFIG_VM_DEBUG). That is a compile time debugging
feature like supported by SLAB.

SLUB debugging is different because we had problems isolating memory
corruption bugs in the production kernels for years. The debug code is
always included in the build but kept out of the hotpaths.


The debug can be enabled when needed to find memory corruption errors
without the need to rebuild a kernel for a prod environment (which may
change race conditions etc) because we only then need to add a kernel
parameter.

"slub_debug" enables kmem_cache->flags & SLAB_DEBUG and that forces all
fastpath processing to be disabled. Thus you can check reliably in the
slow path only for the GFP_ZERO problem.

Add the check to the other debug stuff already there. F.e. in
alloc_debug_processing() or after

if (kmem_cache_debug(s) ...

in ____slab_alloc()

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Wed, Apr 11, 2018 at 04:11:17PM -0500, Christopher Lameter wrote:
> On Wed, 11 Apr 2018, Matthew Wilcox wrote:
> > > Please put this in a code path that is enabled by specifying
> > >
> > > slub_debug
> > >
> > > on the kernel command line.
> >
> > I don't understand.  First, I had:
> >
> > 	if (unlikely(gfpflags & __GFP_ZERO) && object && !WARN_ON_ONCE(s->ctor))
> >
> > and you didn't like that because it was putting checking into a (semi)fast
> > path.  Now you want me to add a check for slub_debug somewhere?  I dont
> > see an existing one I can leverage that will hit on every allocation.
> > Perhaps I'm missing something.
> 
> The WARN_ON is only enabled when you configure and build the kernel with
> debugging enabled (CONFIG_VM_DEBUG). That is a compile time debugging
> feature like supported by SLAB.

Yes.  I want to have an option to check *every single* allocation.

> "slub_debug" enables kmem_cache->flags & SLAB_DEBUG and that forces all
> fastpath processing to be disabled. Thus you can check reliably in the
> slow path only for the GFP_ZERO problem.
> 
> Add the check to the other debug stuff already there. F.e. in
> alloc_debug_processing() or after
> 
> if (kmem_cache_debug(s) ...
> 
> in ____slab_alloc()

I don't see how that works ... can you explain a little more?

I see ___slab_alloc() is called from __slab_alloc().  And I see
slab_alloc_node does this:

        object = c->freelist;
        page = c->page;
        if (unlikely(!object || !node_match(page, node))) {
                object = __slab_alloc(s, gfpflags, node, addr, c);
                stat(s, ALLOC_SLOWPATH);

But I don't see how slub_debug leads to c->freelist always being NULL.
It looks like it gets repopulated from page->freelist in ___slab_alloc()
at the load_freelist label.

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Wed, 11 Apr 2018, Matthew Wilcox wrote:

>
> I don't see how that works ... can you explain a little more?
>
> I see ___slab_alloc() is called from __slab_alloc().  And I see
> slab_alloc_node does this:
>
>         object = c->freelist;
>         page = c->page;
>         if (unlikely(!object || !node_match(page, node))) {
>                 object = __slab_alloc(s, gfpflags, node, addr, c);
>                 stat(s, ALLOC_SLOWPATH);
>
> But I don't see how slub_debug leads to c->freelist always being NULL.
> It looks like it gets repopulated from page->freelist in ___slab_alloc()
> at the load_freelist label.

c->freelist is NULL and thus ___slab_alloc (slowpath) is called.
___slab_alloc populates c->freelist and gets the new object pointer.

if debugging is on then c->freelist is set to NULL at the end of
___slab_alloc because deactivate_slab() is called.

Thus the next invocation of the fastpath will find that c->freelist is
NULL and go to the slowpath. ...

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Thu, Apr 12, 2018 at 09:10:23AM -0500, Christopher Lameter wrote:
> On Wed, 11 Apr 2018, Matthew Wilcox wrote:
> > I don't see how that works ... can you explain a little more?
>
> c->freelist is NULL and thus ___slab_alloc (slowpath) is called.
> ___slab_alloc populates c->freelist and gets the new object pointer.
> 
> if debugging is on then c->freelist is set to NULL at the end of
> ___slab_alloc because deactivate_slab() is called.
> 
> Thus the next invocation of the fastpath will find that c->freelist is
> NULL and go to the slowpath. ...

_ah_.  I hadn't figured out that c->page was always NULL in the debugging
case too, so ___slab_alloc() always hits the 'new_slab' case.  Thanks!

Re: [PATCH v2 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Thu, 12 Apr 2018, Matthew Wilcox wrote:

> > Thus the next invocation of the fastpath will find that c->freelist is
> > NULL and go to the slowpath. ...
>
> _ah_.  I hadn't figured out that c->page was always NULL in the debugging
> case too, so ___slab_alloc() always hits the 'new_slab' case.  Thanks!

Also note that you can have SLUB also do the build with all debugging on
without having to use a command like parameter (like SLAB). That requires
CONFIG_SLUB_DEBUG_ON to be set. CONFIG_SLUB_DEBUG is set by default for
all distro builds. It only includes the debug code but does not activate
them by default. Kernel command line parameters allow you to selectively
switch on debugging features for specific slab caches so that you can
limit the latency variations introduced by debugging into the production
kernel. Thus subtle races may be found.

[PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

__GFP_ZERO requests that the object be initialised to all-zeroes,
while the purpose of a constructor is to initialise an object to a
particular pattern.  We cannot do both.  Add a warning to catch any
users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
a constructor.

Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 mm/slab.c | 2 ++
 mm/slob.c | 4 +++-
 mm/slub.c | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 58c8cecc26ab..aca63d49b270 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2661,6 +2661,7 @@ static struct page *cache_grow_begin(struct kmem_cache *cachep,
 				invalid_mask, &invalid_mask, flags, &flags);
 		dump_stack();
 	}
+	WARN_ON_ONCE(cachep->ctor && (flags & __GFP_ZERO));
 	local_flags = flags & (GFP_CONSTRAINT_MASK|GFP_RECLAIM_MASK);
 
 	check_irq_off();
@@ -3067,6 +3068,7 @@ static inline void cache_alloc_debugcheck_before(struct kmem_cache *cachep,
 static void *cache_alloc_debugcheck_after(struct kmem_cache *cachep,
 				gfp_t flags, void *objp, unsigned long caller)
 {
+	WARN_ON_ONCE(cachep->ctor && (flags & __GFP_ZERO));
 	if (!objp)
 		return objp;
 	if (cachep->flags & SLAB_POISON) {
diff --git a/mm/slob.c b/mm/slob.c
index 1a46181b675c..958173fd7c24 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -556,8 +556,10 @@ static void *slob_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
 					    flags, node);
 	}
 
-	if (b && c->ctor)
+	if (b && c->ctor) {
+		WARN_ON_ONCE(flags & __GFP_ZERO);
 		c->ctor(b);
+	}
 
 	kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
 	return b;
diff --git a/mm/slub.c b/mm/slub.c
index a28488643603..0487d316a665 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2434,6 +2434,8 @@ static inline void *new_slab_objects(struct kmem_cache *s, gfp_t flags,
 	struct kmem_cache_cpu *c = *pc;
 	struct page *page;
 
+	WARN_ON_ONCE(s->ctor && (flags & __GFP_ZERO));
+
 	freelist = get_partial(s, flags, node, c);
 
 	if (freelist)
-- 
2.16.3

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Christopher Lameter]

On Thu, 12 Apr 2018, Matthew Wilcox wrote:

> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.

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

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Guenter Roeck]

Hi,

On Thu, Apr 12, 2018 at 12:13:22PM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> __GFP_ZERO requests that the object be initialised to all-zeroes,
> while the purpose of a constructor is to initialise an object to a
> particular pattern.  We cannot do both.  Add a warning to catch any
> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> a constructor.
> 
> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: Michal Hocko <mhocko@suse.com>

Seen with v4.18-rc7-139-gef46808 and v4.18-rc7-178-g0b5b1f9a78b5 when
booting sh4 images in qemu:

ata1: PATA max PIO0 mmio cmd 0xb4001000 ctl 0xb400080c irq 107
physmap platform flash device: 02000001 at 00000000
WARNING: CPU: 0 PID: 926 at mm/slab.c:2666 cache_alloc_refill+0x8a/0x594
Modules linked in:

CPU: 0 PID: 926 Comm: kworker/u2:2 Not tainted 4.18.0-rc7-00139-gef46808 #1
PC is at cache_alloc_refill+0x8a/0x594
PR is at kmem_cache_alloc+0x72/0xac
PC  : 8c0b1e06 SP  : 8fad5ed8 SR  : 400080f0 
TEA : c0087fe0
R0  : 00008000 R1  : 006080c0 R2  : 006080c0 R3  : 8c01e110
R4  : 8f801a00 R5  : 00000000 R6  : 00000000 R7  : 00000000
R8  : 0000000c R9  : 006080c0 R10 : 8c48302c R11 : 8fae8e60
R12 : 8f802540 R13 : 8f801a00 R14 : 8c4f22e8
MACH: 00000085 MACL: 00000e00 GBR : 00000000 PR  : 8c0b244e

Call trace:
 [<(ptrval)>] arch_local_irq_restore+0x0/0x24
 [<(ptrval)>] arch_local_save_flags+0x0/0x8
 [<(ptrval)>] kmem_cache_alloc+0x72/0xac
 [<(ptrval)>] arch_local_irq_restore+0x0/0x24
 [<(ptrval)>] mm_init.isra.7+0xb4/0x104
 [<(ptrval)>] __do_execve_file+0x1f8/0x5b4
 [<(ptrval)>] do_execve+0x16/0x24
 [<(ptrval)>] arch_local_irq_restore+0x0/0x24
 [<(ptrval)>] call_usermodehelper_exec_async+0xe0/0x124
 [<(ptrval)>] ret_from_kernel_thread+0xc/0x14
 [<(ptrval)>] schedule_tail+0x0/0x54
 [<(ptrval)>] call_usermodehelper_exec_async+0x0/0x124

---[ end trace 10ff75dd606d4222 ]---

This is not a a new trace; I had missed it because the "cut here" line
is missing from the log.

qemu command line:

qemu-system-sh4 -M r2d -kernel ./arch/sh/boot/zImage \
	-initrd rootfs.cpio \
	-append 'rdinit=/sbin/init console=ttySC1,115200 noiotrap' \
	-serial null -serial stdio -net nic,model=rtl8139 \
	-net user -nographic -monitor null

Guenter

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Fri, Aug 03, 2018 at 02:22:57PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Apr 12, 2018 at 12:13:22PM -0700, Matthew Wilcox wrote:
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
> > 
> > Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Seen with v4.18-rc7-139-gef46808 and v4.18-rc7-178-g0b5b1f9a78b5 when
> booting sh4 images in qemu:

Thanks!  It's under discussion here:

https://marc.info/?t\x153301426900002&r=1&w=2

also reported here with a bogus backtrace:

https://marc.info/?l=linux-sh&m\x153305755505935&w=2

Short version: It's a bug that's been present since 2009 and nobody
noticed until now.  And nobody's quite sure what the effect of this
bug is.

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Matthew Wilcox]

On Fri, Aug 03, 2018 at 02:22:57PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Apr 12, 2018 at 12:13:22PM -0700, Matthew Wilcox wrote:
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > while the purpose of a constructor is to initialise an object to a
> > particular pattern.  We cannot do both.  Add a warning to catch any
> > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > a constructor.
> > 
> > Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Seen with v4.18-rc7-139-gef46808 and v4.18-rc7-178-g0b5b1f9a78b5 when
> booting sh4 images in qemu:

Thanks!  It's under discussion here:

https://marc.info/?t=153301426900002&r=1&w=2

also reported here with a bogus backtrace:

https://marc.info/?l=linux-sh&m=153305755505935&w=2

Short version: It's a bug that's been present since 2009 and nobody
noticed until now.  And nobody's quite sure what the effect of this
bug is.

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Geert Uytterhoeven]

On Sat, Aug 4, 2018 at 12:34 AM Matthew Wilcox <willy@infradead.org> wrote:
> On Fri, Aug 03, 2018 at 02:22:57PM -0700, Guenter Roeck wrote:
> > On Thu, Apr 12, 2018 at 12:13:22PM -0700, Matthew Wilcox wrote:
> > > From: Matthew Wilcox <mawilcox@microsoft.com>
> > > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > > while the purpose of a constructor is to initialise an object to a
> > > particular pattern.  We cannot do both.  Add a warning to catch any
> > > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > > a constructor.
> > >
> > > Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > > Acked-by: Michal Hocko <mhocko@suse.com>
> >
> > Seen with v4.18-rc7-139-gef46808 and v4.18-rc7-178-g0b5b1f9a78b5 when
> > booting sh4 images in qemu:
>
> Thanks!  It's under discussion here:
>
> https://marc.info/?t\x153301426900002&r=1&w=2

and https://www.spinics.net/lists/linux-sh/msg53298.html

> also reported here with a bogus backtrace:
>
> https://marc.info/?l=linux-sh&m\x153305755505935&w=2
>
> Short version: It's a bug that's been present since 2009 and nobody
> noticed until now.  And nobody's quite sure what the effect of this
> bug is.
Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Geert Uytterhoeven]

On Sat, Aug 4, 2018 at 12:34 AM Matthew Wilcox <willy@infradead.org> wrote:
> On Fri, Aug 03, 2018 at 02:22:57PM -0700, Guenter Roeck wrote:
> > On Thu, Apr 12, 2018 at 12:13:22PM -0700, Matthew Wilcox wrote:
> > > From: Matthew Wilcox <mawilcox@microsoft.com>
> > > __GFP_ZERO requests that the object be initialised to all-zeroes,
> > > while the purpose of a constructor is to initialise an object to a
> > > particular pattern.  We cannot do both.  Add a warning to catch any
> > > users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
> > > a constructor.
> > >
> > > Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
> > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > > Acked-by: Michal Hocko <mhocko@suse.com>
> >
> > Seen with v4.18-rc7-139-gef46808 and v4.18-rc7-178-g0b5b1f9a78b5 when
> > booting sh4 images in qemu:
>
> Thanks!  It's under discussion here:
>
> https://marc.info/?t=153301426900002&r=1&w=2

and https://www.spinics.net/lists/linux-sh/msg53298.html

> also reported here with a bogus backtrace:
>
> https://marc.info/?l=linux-sh&m=153305755505935&w=2
>
> Short version: It's a bug that's been present since 2009 and nobody
> noticed until now.  And nobody's quite sure what the effect of this
> bug is.
Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Guenter Roeck]

On 08/04/2018 02:28 AM, Geert Uytterhoeven wrote:
> On Sat, Aug 4, 2018 at 12:34 AM Matthew Wilcox <willy@infradead.org> wrote:
>> On Fri, Aug 03, 2018 at 02:22:57PM -0700, Guenter Roeck wrote:
>>> On Thu, Apr 12, 2018 at 12:13:22PM -0700, Matthew Wilcox wrote:
>>>> From: Matthew Wilcox <mawilcox@microsoft.com>
>>>> __GFP_ZERO requests that the object be initialised to all-zeroes,
>>>> while the purpose of a constructor is to initialise an object to a
>>>> particular pattern.  We cannot do both.  Add a warning to catch any
>>>> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
>>>> a constructor.
>>>>
>>>> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
>>>> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
>>>> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>>>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>>>> Acked-by: Michal Hocko <mhocko@suse.com>
>>>
>>> Seen with v4.18-rc7-139-gef46808 and v4.18-rc7-178-g0b5b1f9a78b5 when
>>> booting sh4 images in qemu:
>>
>> Thanks!  It's under discussion here:
>>
>> https://marc.info/?t\x153301426900002&r=1&w=2
> 
> and https://www.spinics.net/lists/linux-sh/msg53298.html
> 
>> also reported here with a bogus backtrace:
>>
>> https://marc.info/?l=linux-sh&m\x153305755505935&w=2
>>
>> Short version: It's a bug that's been present since 2009 and nobody
>> noticed until now.  And nobody's quite sure what the effect of this
>> bug is.

Though now it is making a lot of noise :-).

I just found two more 0-day bugs, so maybe improved testing and log messages
such as the one encountered here do help a bit.

Guenter

Re: [PATCH v3 2/2] slab: __GFP_ZERO is incompatible with a constructor

[Guenter Roeck]

On 08/04/2018 02:28 AM, Geert Uytterhoeven wrote:
> On Sat, Aug 4, 2018 at 12:34 AM Matthew Wilcox <willy@infradead.org> wrote:
>> On Fri, Aug 03, 2018 at 02:22:57PM -0700, Guenter Roeck wrote:
>>> On Thu, Apr 12, 2018 at 12:13:22PM -0700, Matthew Wilcox wrote:
>>>> From: Matthew Wilcox <mawilcox@microsoft.com>
>>>> __GFP_ZERO requests that the object be initialised to all-zeroes,
>>>> while the purpose of a constructor is to initialise an object to a
>>>> particular pattern.  We cannot do both.  Add a warning to catch any
>>>> users who mistakenly pass a __GFP_ZERO flag when allocating a slab with
>>>> a constructor.
>>>>
>>>> Fixes: d07dbea46405 ("Slab allocators: support __GFP_ZERO in all allocators")
>>>> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
>>>> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>>>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>>>> Acked-by: Michal Hocko <mhocko@suse.com>
>>>
>>> Seen with v4.18-rc7-139-gef46808 and v4.18-rc7-178-g0b5b1f9a78b5 when
>>> booting sh4 images in qemu:
>>
>> Thanks!  It's under discussion here:
>>
>> https://marc.info/?t=153301426900002&r=1&w=2
> 
> and https://www.spinics.net/lists/linux-sh/msg53298.html
> 
>> also reported here with a bogus backtrace:
>>
>> https://marc.info/?l=linux-sh&m=153305755505935&w=2
>>
>> Short version: It's a bug that's been present since 2009 and nobody
>> noticed until now.  And nobody's quite sure what the effect of this
>> bug is.

Though now it is making a lot of noise :-).

I just found two more 0-day bugs, so maybe improved testing and log messages
such as the one encountered here do help a bit.

Guenter

Re: [PATCH v2 0/2] Fix __GFP_ZERO vs constructor

[Minchan Kim]

Matthew,

Please Cced relevant people so they know what's going on the problem
they spent on much time. Everyone doesn't keep an eye on mailing list.

On Tue, Apr 10, 2018 at 11:03:18PM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> v1->v2:
>  - Added review/ack tags (thanks!)
>  - Switched the order of the patches
>  - Reworded commit message of the patch which actually fixes the bug
>  - Moved slab debug patches under CONFIG_DEBUG_VM to check _every_
>    allocation and added checks in the slowpath for the allocations which
>    end up allocating a page.
> 
> Matthew Wilcox (2):
>   Fix NULL pointer in page_cache_tree_insert
>   slab: __GFP_ZERO is incompatible with a constructor
> 
>  mm/filemap.c | 9 ++++-----
>  mm/slab.c    | 7 ++++---
>  mm/slab.h    | 7 +++++++
>  mm/slob.c    | 4 +++-
>  mm/slub.c    | 5 +++--
>  5 files changed, 21 insertions(+), 11 deletions(-)
> 
> -- 
> 2.16.3
> 

Re: [PATCH v2 0/2] Fix __GFP_ZERO vs constructor

[Matthew Wilcox]

On Thu, Apr 12, 2018 at 09:54:51AM +0900, Minchan Kim wrote:
> Matthew,
> 
> Please Cced relevant people so they know what's going on the problem
> they spent on much time. Everyone doesn't keep an eye on mailing list.

My apologies; I assumed that git send-email would pick up the people
named in the changelog.  I have now read the source code and discovered
it only picks up the people listed in Signed-off-by: and Cc:.  That
surprises me; I'll submit a patch.

> On Tue, Apr 10, 2018 at 11:03:18PM -0700, Matthew Wilcox wrote:
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > v1->v2:
> >  - Added review/ack tags (thanks!)
> >  - Switched the order of the patches
> >  - Reworded commit message of the patch which actually fixes the bug
> >  - Moved slab debug patches under CONFIG_DEBUG_VM to check _every_
> >    allocation and added checks in the slowpath for the allocations which
> >    end up allocating a page.
> > 
> > Matthew Wilcox (2):
> >   Fix NULL pointer in page_cache_tree_insert
> >   slab: __GFP_ZERO is incompatible with a constructor
> > 
> >  mm/filemap.c | 9 ++++-----
> >  mm/slab.c    | 7 ++++---
> >  mm/slab.h    | 7 +++++++
> >  mm/slob.c    | 4 +++-
> >  mm/slub.c    | 5 +++--
> >  5 files changed, 21 insertions(+), 11 deletions(-)
> > 
> > -- 
> > 2.16.3
> > 
> 

Re: [PATCH v2 0/2] Fix __GFP_ZERO vs constructor

[Michal Hocko]

On Thu 12-04-18 12:24:24, Matthew Wilcox wrote:
> On Thu, Apr 12, 2018 at 09:54:51AM +0900, Minchan Kim wrote:
> > Matthew,
> > 
> > Please Cced relevant people so they know what's going on the problem
> > they spent on much time. Everyone doesn't keep an eye on mailing list.
> 
> My apologies; I assumed that git send-email would pick up the people
> named in the changelog.  I have now read the source code and discovered
> it only picks up the people listed in Signed-off-by: and Cc:.  That
> surprises me; I'll submit a patch.

I remember that there was a discussion to add support for more
$Foo-by: $EMAIL

but I do not remember the outcome of the discussion and from a quick
glance into the perl disaster it doesn't seem to handle generic tags.
I am using the following
$ cat cc-cmd.sh
#!/bin/bash

if [[ $1 == *gitsendemail.msg* || $1 == *cover-letter* ]]; then
        grep '<.*@.*>' -h *.patch | sed 's/^.*: //' | sort | uniq
else
        grep '<.*@.*>' -h $1 | sed 's/^.*: //' | sort | uniq
fi

and use it as --cc-cmd=
-- 
Michal Hocko
SUSE Labs