Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1559541 > unrolled thread

[PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Borislav Petkov <bp@alien8.de> - 2017-01-16 10:20 +0100
    Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Leon Romanovsky <leon@kernel.org> - 2017-01-16 10:30 +0100
      Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Borislav Petkov <bp@alien8.de> - 2017-01-16 10:40 +0100
        Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Leon Romanovsky <leon@kernel.org> - 2017-01-16 10:50 +0100
          Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Michal Hocko <mhocko@kernel.org> - 2017-01-16 11:00 +0100
          Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Borislav Petkov <bp@alien8.de> - 2017-01-16 11:00 +0100
            Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Leon Romanovsky <leon@kernel.org> - 2017-01-16 11:20 +0100
              Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Leon Romanovsky <leon@kernel.org> - 2017-01-16 11:20 +0100
              Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Borislav Petkov <bp@alien8.de> - 2017-01-16 11:20 +0100
    Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Michal Hocko <mhocko@kernel.org> - 2017-01-16 10:40 +0100
    Re: [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check Vlastimil Babka <vbabka@suse.cz> - 2017-01-16 11:10 +0100

#1559541 — [PATCH] mm/slub: Add a dump_stack() to the unexpected GFP check

From: Borislav Petkov <bp@suse.de>

We wanna know who's doing such a thing. Like slab.c does that.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 mm/slub.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/slub.c b/mm/slub.c
index 067598a00849..1b0fa7625d6d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1623,6 +1623,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
 		flags &= ~GFP_SLAB_BUG_MASK;
 		pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
 				invalid_mask, &invalid_mask, flags, &flags);
+		dump_stack();
 	}
 
 	return allocate_slab(s,
-- 
2.11.0

[toc] | [next] | [standalone]

#1559548

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jan 16, 2017 at 10:16:43AM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> We wanna know who's doing such a thing. Like slab.c does that.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  mm/slub.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 067598a00849..1b0fa7625d6d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1623,6 +1623,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
>  		flags &= ~GFP_SLAB_BUG_MASK;
>  		pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
>  				invalid_mask, &invalid_mask, flags, &flags);
> +		dump_stack();

Will it make sense to change these two lines above to WARN(true, .....)?

>  	}
>
>  	return allocate_slab(s,
> --
> 2.11.0
>

[toc] | [prev] | [next] | [standalone]

#1559558

On Mon, Jan 16, 2017 at 11:28:40AM +0200, Leon Romanovsky wrote:
> On Mon, Jan 16, 2017 at 10:16:43AM +0100, Borislav Petkov wrote:
> > From: Borislav Petkov <bp@suse.de>
> >
> > We wanna know who's doing such a thing. Like slab.c does that.
> >
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> > ---
> >  mm/slub.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 067598a00849..1b0fa7625d6d 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1623,6 +1623,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
> >  		flags &= ~GFP_SLAB_BUG_MASK;
> >  		pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
> >  				invalid_mask, &invalid_mask, flags, &flags);
> > +		dump_stack();
> 
> Will it make sense to change these two lines above to WARN(true, .....)?

Should be equivalent.

I'd even go a step further and make this a small inline function,
something like warn_unexpected_gfp(flags) or so and call it from both
from slab.c and slub.c.

Depending on what mm folks prefer, that is.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1559565

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jan 16, 2017 at 10:37:02AM +0100, Borislav Petkov wrote:
> On Mon, Jan 16, 2017 at 11:28:40AM +0200, Leon Romanovsky wrote:
> > On Mon, Jan 16, 2017 at 10:16:43AM +0100, Borislav Petkov wrote:
> > > From: Borislav Petkov <bp@suse.de>
> > >
> > > We wanna know who's doing such a thing. Like slab.c does that.
> > >
> > > Signed-off-by: Borislav Petkov <bp@suse.de>
> > > ---
> > >  mm/slub.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index 067598a00849..1b0fa7625d6d 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -1623,6 +1623,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
> > >  		flags &= ~GFP_SLAB_BUG_MASK;
> > >  		pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
> > >  				invalid_mask, &invalid_mask, flags, &flags);
> > > +		dump_stack();
> >
> > Will it make sense to change these two lines above to WARN(true, .....)?
>
> Should be equivalent.

Almost, except one point - pr_warn and dump_stack have different log
levels. There is a chance that user won't see pr_warn message above, but
dump_stack will be always present.

For WARN_XXX, users will always see message and stack at the same time.

>
> I'd even go a step further and make this a small inline function,
> something like warn_unexpected_gfp(flags) or so and call it from both
> from slab.c and slub.c.
>
> Depending on what mm folks prefer, that is.
>
> --
> Regards/Gruss,
>     Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1559568

On Mon 16-01-17 11:48:51, Leon Romanovsky wrote:
> On Mon, Jan 16, 2017 at 10:37:02AM +0100, Borislav Petkov wrote:
> > On Mon, Jan 16, 2017 at 11:28:40AM +0200, Leon Romanovsky wrote:
> > > On Mon, Jan 16, 2017 at 10:16:43AM +0100, Borislav Petkov wrote:
> > > > From: Borislav Petkov <bp@suse.de>
> > > >
> > > > We wanna know who's doing such a thing. Like slab.c does that.
> > > >
> > > > Signed-off-by: Borislav Petkov <bp@suse.de>
> > > > ---
> > > >  mm/slub.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > index 067598a00849..1b0fa7625d6d 100644
> > > > --- a/mm/slub.c
> > > > +++ b/mm/slub.c
> > > > @@ -1623,6 +1623,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
> > > >  		flags &= ~GFP_SLAB_BUG_MASK;
> > > >  		pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
> > > >  				invalid_mask, &invalid_mask, flags, &flags);
> > > > +		dump_stack();
> > >
> > > Will it make sense to change these two lines above to WARN(true, .....)?
> >
> > Should be equivalent.
> 
> Almost, except one point - pr_warn and dump_stack have different log
> levels. There is a chance that user won't see pr_warn message above, but
> dump_stack will be always present.
> 
> For WARN_XXX, users will always see message and stack at the same time.

On the other hand WARN* will taint the kernel and this sounds a bit
overreacting for something like a wrong gfp mask which is perfectly
recoverable. Not to mention users who care configured to panic on
warning.

So while I do not have a strong opinion on this I would rather stay with
the dump_stack.


-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1559571

On Mon, Jan 16, 2017 at 11:48:51AM +0200, Leon Romanovsky wrote:
> Almost, except one point - pr_warn and dump_stack have different log

Actually, Michal pointed out on IRC a more relevant difference:

WARN() taints the kernel and we don't want that for GFP flags misuse.
Also, from looking at __warn(), it checks panic_on_warn and we explode
if set.

So no, we probably don't want WARN() here.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1559589

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jan 16, 2017 at 10:55:22AM +0100, Borislav Petkov wrote:
> On Mon, Jan 16, 2017 at 11:48:51AM +0200, Leon Romanovsky wrote:
> > Almost, except one point - pr_warn and dump_stack have different log
>
> Actually, Michal pointed out on IRC a more relevant difference:
>
> WARN() taints the kernel and we don't want that for GFP flags misuse.

And doesn't dump_stack do the same? It pollutes the log too.

> Also, from looking at __warn(), it checks panic_on_warn and we explode
> if set.

Right, it is very valid point.

>
> So no, we probably don't want WARN() here.

I understand, Thanks.

>
> --
> Regards/Gruss,
>     Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1559590

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jan 16, 2017 at 11:13:10AM +0100, Borislav Petkov wrote:
> On Mon, Jan 16, 2017 at 12:09:30PM +0200, Leon Romanovsky wrote:
> > And doesn't dump_stack do the same? It pollutes the log too.
>
> It is not about polluting the log - it is about tainting.
>
> __warn()->add_taint(taint, LOCKDEP_STILL_OK);

Thanks,
I had something different in mind for word "taint".
Sorry for that.

>
> --
> Regards/Gruss,
>     Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1559596

On Mon, Jan 16, 2017 at 12:09:30PM +0200, Leon Romanovsky wrote:
> And doesn't dump_stack do the same? It pollutes the log too.

It is not about polluting the log - it is about tainting.

__warn()->add_taint(taint, LOCKDEP_STILL_OK);

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1559554

[Let's add Andrew]

On Mon 16-01-17 10:16:43, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> We wanna know who's doing such a thing. Like slab.c does that.

Yes this was an omission on my side in 72baeef0c271 ("slab: do not panic
on invalid gfp_mask").
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/slub.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 067598a00849..1b0fa7625d6d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1623,6 +1623,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
>  		flags &= ~GFP_SLAB_BUG_MASK;
>  		pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
>  				invalid_mask, &invalid_mask, flags, &flags);
> +		dump_stack();
>  	}
>  
>  	return allocate_slab(s,
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1559579

On 01/16/2017 10:16 AM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> We wanna know who's doing such a thing. Like slab.c does that.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/slub.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 067598a00849..1b0fa7625d6d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1623,6 +1623,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
>  		flags &= ~GFP_SLAB_BUG_MASK;
>  		pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
>  				invalid_mask, &invalid_mask, flags, &flags);
> +		dump_stack();
>  	}
>  
>  	return allocate_slab(s,
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web