[PATCH] mm/slub: disallow obj's allocation on page with mismatched pfmemalloc purpose

[PATCH] mm/slub: disallow obj's allocation on page with mismatched pfmemalloc purpose

[Pingfan Liu]

new_slab_objects() always return c->page matching the required gfpflags,
but the current code is misleading and ___slab_alloc->deactivate_slab seems
to allow not-pfmemalloc purpose obj to be allocated from pfmemalloc-purpose
page. This patch re-organize the code to eliminate the confusion.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/slub.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index a68c2ae..e152634 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2551,23 +2551,21 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 	}
 
 	freelist = new_slab_objects(s, gfpflags, node, &c);
-
 	if (unlikely(!freelist)) {
 		slab_out_of_memory(s, gfpflags, node);
 		return NULL;
 	}
 
+	VM_BUG_ON(!pfmemalloc_match(page, gfpflags));
 	page = c->page;
-	if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags)))
+	if (likely(!kmem_cache_debug(s))
 		goto load_freelist;
 
 	/* Only entered in the debug case */
-	if (kmem_cache_debug(s) &&
-			!alloc_debug_processing(s, page, freelist, addr))
+	if (!alloc_debug_processing(s, page, freelist, addr))
 		goto new_slab;	/* Slab failed checks. Next slab needed */
-
-	deactivate_slab(s, page, get_freepointer(s, freelist), c);
-	return freelist;
+	else
+		goto load_freelist;
 }
 
 /*
-- 
2.7.4

Re: [PATCH] mm/slub: disallow obj's allocation on page with mismatched pfmemalloc purpose

[Christopher Lameter]

On Wed, 26 Sep 2018, Pingfan Liu wrote:

> -
>  	if (unlikely(!freelist)) {
>  		slab_out_of_memory(s, gfpflags, node);
>  		return NULL;
>  	}
>
> +	VM_BUG_ON(!pfmemalloc_match(page, gfpflags));
>  	page = c->page;
> -	if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags)))
> +	if (likely(!kmem_cache_debug(s))
>  		goto load_freelist;
>
>  	/* Only entered in the debug case */
> -	if (kmem_cache_debug(s) &&
> -			!alloc_debug_processing(s, page, freelist, addr))
> +	if (!alloc_debug_processing(s, page, freelist, addr))
>  		goto new_slab;	/* Slab failed checks. Next slab needed */
> -
> -	deactivate_slab(s, page, get_freepointer(s, freelist), c);

In the debug case the slab needs to be deactivated. Otherwise the
slowpath will not be used and debug checks on the following objects will
not be done.

> -	return freelist;
> +	else
> +		goto load_freelist;
>  }
>
>  /*
>

Re: [PATCH] mm/slub: disallow obj's allocation on page with mismatched pfmemalloc purpose

[Pingfan Liu]

On Thu, Sep 27, 2018 at 12:14 AM Christopher Lameter <cl@linux.com> wrote:
>
> On Wed, 26 Sep 2018, Pingfan Liu wrote:
>
> > -
> >       if (unlikely(!freelist)) {
> >               slab_out_of_memory(s, gfpflags, node);
> >               return NULL;
> >       }
> >
> > +     VM_BUG_ON(!pfmemalloc_match(page, gfpflags));
> >       page = c->page;
> > -     if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags)))
> > +     if (likely(!kmem_cache_debug(s))
> >               goto load_freelist;
> >
> >       /* Only entered in the debug case */
> > -     if (kmem_cache_debug(s) &&
> > -                     !alloc_debug_processing(s, page, freelist, addr))
> > +     if (!alloc_debug_processing(s, page, freelist, addr))
> >               goto new_slab;  /* Slab failed checks. Next slab needed */
> > -
> > -     deactivate_slab(s, page, get_freepointer(s, freelist), c);
>
> In the debug case the slab needs to be deactivated. Otherwise the
> slowpath will not be used and debug checks on the following objects will
> not be done.
>
Got it.

Thanks,
Pingfan

Re: [PATCH] mm/slub: disallow obj's allocation on page with mismatched pfmemalloc purpose

[Pingfan Liu]

On Thu, Sep 27, 2018 at 9:15 PM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> On Thu, Sep 27, 2018 at 12:14 AM Christopher Lameter <cl@linux.com> wrote:
> >
> > On Wed, 26 Sep 2018, Pingfan Liu wrote:
> >
> > > -
> > >       if (unlikely(!freelist)) {
> > >               slab_out_of_memory(s, gfpflags, node);
> > >               return NULL;
> > >       }
> > >
> > > +     VM_BUG_ON(!pfmemalloc_match(page, gfpflags));
> > >       page = c->page;
> > > -     if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags)))
> > > +     if (likely(!kmem_cache_debug(s))
> > >               goto load_freelist;
> > >
> > >       /* Only entered in the debug case */
> > > -     if (kmem_cache_debug(s) &&
> > > -                     !alloc_debug_processing(s, page, freelist, addr))
> > > +     if (!alloc_debug_processing(s, page, freelist, addr))
> > >               goto new_slab;  /* Slab failed checks. Next slab needed */
> > > -
> > > -     deactivate_slab(s, page, get_freepointer(s, freelist), c);
> >
> > In the debug case the slab needs to be deactivated. Otherwise the
> > slowpath will not be used and debug checks on the following objects will
> > not be done.
> >
After taking a more closely look at the debug code, I consider whether
the alloc_debug_processing() can be also called after get_freelist(s,
page), then deactivate_slab() is not required . My justification is
the debug code will take the same code path as the non-debug,  hence
the page will experience the same transition on different list like
the non-debug code, and help to detect the bug, also it will improve
scalability on SMP.
Besides this, I found the debug code is not scalable well, is it worth
to work on it?

Thanks,
Pingfan

Re: [PATCH] mm/slub: disallow obj's allocation on page with mismatched pfmemalloc purpose

[Christopher Lameter]

On Sun, 30 Sep 2018, Pingfan Liu wrote:

> > > In the debug case the slab needs to be deactivated. Otherwise the
> > > slowpath will not be used and debug checks on the following objects will
> > > not be done.
> > >
> After taking a more closely look at the debug code, I consider whether
> the alloc_debug_processing() can be also called after get_freelist(s,
> page), then deactivate_slab() is not required . My justification is
> the debug code will take the same code path as the non-debug,  hence
> the page will experience the same transition on different list like
> the non-debug code, and help to detect the bug, also it will improve
> scalability on SMP.
> Besides this, I found the debug code is not scalable well, is it worth
> to work on it?

The debug code is kept out of the hot path intentionally because it does
not scale well and reduces performance. Its compiled in in case we have
to track down a nasty memory corruption bug on a prod kernel that cannot
be easily rebuilt.