[PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Cyrill Gorcunov]

We may reach NULL dereference oops if kmalloc failed.
Lets do panic better with sensible message.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

Actually there is a dubious place as well at early_get_nodeid.
Is there a guarantee that we _never_ fail in early_ioremap?

 arch/x86/kernel/apic/x2apic_uv_x.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
+++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -584,15 +584,21 @@ void __init uv_system_init(void)
 
 	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
 	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
+	if (!uv_blade_info)
+		goto err_nomem;
 
 	get_lowmem_redirect(&lowmem_redir_base, &lowmem_redir_size);
 
 	bytes = sizeof(uv_node_to_blade[0]) * num_possible_nodes();
 	uv_node_to_blade = kmalloc(bytes, GFP_KERNEL);
+	if (!uv_node_to_blade)
+		goto err_nomem;
 	memset(uv_node_to_blade, 255, bytes);
 
 	bytes = sizeof(uv_cpu_to_blade[0]) * num_possible_cpus();
 	uv_cpu_to_blade = kmalloc(bytes, GFP_KERNEL);
+	if (!uv_cpu_to_blade)
+		goto err_nomem;
 	memset(uv_cpu_to_blade, 255, bytes);
 
 	blade = 0;
@@ -667,4 +673,7 @@ void __init uv_system_init(void)
 	uv_cpu_init();
 	uv_scir_register_cpu_notifier();
 	proc_mkdir("sgi_uv", NULL);
+
+err_nomem:
+	panic("UV: Out of memory\n");
 }

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> We may reach NULL dereference oops if kmalloc failed.
> Lets do panic better with sensible message.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
> 
> Actually there is a dubious place as well at early_get_nodeid.
> Is there a guarantee that we _never_ fail in early_ioremap?
> 
>  arch/x86/kernel/apic/x2apic_uv_x.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> =====================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -584,15 +584,21 @@ void __init uv_system_init(void)
>  
>  	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
>  	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
> +	if (!uv_blade_info)
> +		goto err_nomem;

hm, i think a BUG_ON() might be shorter and more appropriate here. 
We really shouldnt be running out of memory during system init.

	Ingo

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Cyrill Gorcunov]

[Ingo Molnar - Fri, May 01, 2009 at 10:03:31PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
| 
| > We may reach NULL dereference oops if kmalloc failed.
| > Lets do panic better with sensible message.
| > 
| > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
| > ---
| > 
| > Actually there is a dubious place as well at early_get_nodeid.
| > Is there a guarantee that we _never_ fail in early_ioremap?
| > 
| >  arch/x86/kernel/apic/x2apic_uv_x.c |    9 +++++++++
| >  1 file changed, 9 insertions(+)
| > 
| > Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
| > =====================================================================
| > --- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
| > +++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
| > @@ -584,15 +584,21 @@ void __init uv_system_init(void)
| >  
| >  	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
| >  	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
| > +	if (!uv_blade_info)
| > +		goto err_nomem;
| 
| hm, i think a BUG_ON() might be shorter and more appropriate here. 
| We really shouldnt be running out of memory during system init.
| 
| 	Ingo
| 

Yeah, indeed! I was thinking of __GPF_NOFAIL here as well with
message on top like pr_debug("UV: allocating memory\n") or something
like that. It would make it even cleaner I guess. Hmm?

	-- Cyrill

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sat, May 02, 2009 at 12:09:37AM +0400]
| [Ingo Molnar - Fri, May 01, 2009 at 10:03:31PM +0200]
| | 
| | * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
| | 
| | > We may reach NULL dereference oops if kmalloc failed.
| | > Lets do panic better with sensible message.
| | > 
| | > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
| | > ---
| | > 
| | > Actually there is a dubious place as well at early_get_nodeid.
| | > Is there a guarantee that we _never_ fail in early_ioremap?
| | > 
| | >  arch/x86/kernel/apic/x2apic_uv_x.c |    9 +++++++++
| | >  1 file changed, 9 insertions(+)
| | > 
| | > Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
| | > =====================================================================
| | > --- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
| | > +++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
| | > @@ -584,15 +584,21 @@ void __init uv_system_init(void)
| | >  
| | >  	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
| | >  	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
| | > +	if (!uv_blade_info)
| | > +		goto err_nomem;
| | 
| | hm, i think a BUG_ON() might be shorter and more appropriate here. 
| | We really shouldnt be running out of memory during system init.
| | 
| | 	Ingo
| | 
| 
| Yeah, indeed! I was thinking of __GPF_NOFAIL here as well with
| message on top like pr_debug("UV: allocating memory\n") or something
| like that. It would make it even cleaner I guess. Hmm?
| 
| 	-- Cyrill

Here is an updated one.

	-- Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

We may reach NULL dereference oops if kmalloc failed.
Prevent it with explisit BUG_ON.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 arch/x86/kernel/apic/x2apic_uv_x.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
+++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -584,15 +584,18 @@ void __init uv_system_init(void)
 
 	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
 	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
+	BUG_ON(!uv_blade_info);
 
 	get_lowmem_redirect(&lowmem_redir_base, &lowmem_redir_size);
 
 	bytes = sizeof(uv_node_to_blade[0]) * num_possible_nodes();
 	uv_node_to_blade = kmalloc(bytes, GFP_KERNEL);
+	BUG_ON(!uv_node_to_blade);
 	memset(uv_node_to_blade, 255, bytes);
 
 	bytes = sizeof(uv_cpu_to_blade[0]) * num_possible_cpus();
 	uv_cpu_to_blade = kmalloc(bytes, GFP_KERNEL);
+	BUG_ON(!uv_cpu_to_blade);
 	memset(uv_cpu_to_blade, 255, bytes);
 
 	blade = 0;

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Jack Steiner]

On Sat, May 02, 2009 at 12:25:11AM +0400, Cyrill Gorcunov wrote:
> [Cyrill Gorcunov - Sat, May 02, 2009 at 12:09:37AM +0400]
> | [Ingo Molnar - Fri, May 01, 2009 at 10:03:31PM +0200]
> | | 
> | | * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> | | 
> | | > We may reach NULL dereference oops if kmalloc failed.
> | | > Lets do panic better with sensible message.
> | | > 
> | | > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> | | > ---
> | | > 
> | | > Actually there is a dubious place as well at early_get_nodeid.
> | | > Is there a guarantee that we _never_ fail in early_ioremap?
> | | > 
> | | >  arch/x86/kernel/apic/x2apic_uv_x.c |    9 +++++++++
> | | >  1 file changed, 9 insertions(+)
> | | > 
> | | > Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> | | > =====================================================================
> | | > --- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> | | > +++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> | | > @@ -584,15 +584,21 @@ void __init uv_system_init(void)
> | | >  
> | | >  	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
> | | >  	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
> | | > +	if (!uv_blade_info)
> | | > +		goto err_nomem;
> | | 
> | | hm, i think a BUG_ON() might be shorter and more appropriate here. 
> | | We really shouldnt be running out of memory during system init.
> | | 
> | | 	Ingo
> | | 
> | 
> | Yeah, indeed! I was thinking of __GPF_NOFAIL here as well with
> | message on top like pr_debug("UV: allocating memory\n") or something
> | like that. It would make it even cleaner I guess. Hmm?
> | 
> | 	-- Cyrill
> 
> Here is an updated one.


Acked-by: Jack Steiner <steiner@sgi.com>


> 
> 	-- Cyrill
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init
> 
> We may reach NULL dereference oops if kmalloc failed.
> Prevent it with explisit BUG_ON.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  arch/x86/kernel/apic/x2apic_uv_x.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> =====================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -584,15 +584,18 @@ void __init uv_system_init(void)
>  
>  	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
>  	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
> +	BUG_ON(!uv_blade_info);
>  
>  	get_lowmem_redirect(&lowmem_redir_base, &lowmem_redir_size);
>  
>  	bytes = sizeof(uv_node_to_blade[0]) * num_possible_nodes();
>  	uv_node_to_blade = kmalloc(bytes, GFP_KERNEL);
> +	BUG_ON(!uv_node_to_blade);
>  	memset(uv_node_to_blade, 255, bytes);
>  
>  	bytes = sizeof(uv_cpu_to_blade[0]) * num_possible_cpus();
>  	uv_cpu_to_blade = kmalloc(bytes, GFP_KERNEL);
> +	BUG_ON(!uv_cpu_to_blade);
>  	memset(uv_cpu_to_blade, 255, bytes);
>  
>  	blade = 0;

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Ingo Molnar]

* Jack Steiner <steiner@sgi.com> wrote:

> >  arch/x86/kernel/apic/x2apic_uv_x.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> > =====================================================================
> > --- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
> > +++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
> > @@ -584,15 +584,18 @@ void __init uv_system_init(void)
> >  
> >  	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
> >  	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
> > +	BUG_ON(!uv_blade_info);
> >  
> >  	get_lowmem_redirect(&lowmem_redir_base, &lowmem_redir_size);
> >  
> >  	bytes = sizeof(uv_node_to_blade[0]) * num_possible_nodes();
> >  	uv_node_to_blade = kmalloc(bytes, GFP_KERNEL);
> > +	BUG_ON(!uv_node_to_blade);
> >  	memset(uv_node_to_blade, 255, bytes);
> >  
> >  	bytes = sizeof(uv_cpu_to_blade[0]) * num_possible_cpus();
> >  	uv_cpu_to_blade = kmalloc(bytes, GFP_KERNEL);
> > +	BUG_ON(!uv_cpu_to_blade);
> >  	memset(uv_cpu_to_blade, 255, bytes);

Hm, would be nice if we had a __GFP_PANIC variant in kmalloc that 
would just panic straight in the allocator, when allocation failure 
is not acceptable. (Andrew Cc:-ed)

It does not increase the priority of the allocation nor does it 
trigger any 'dont fail' logic - it is simply the central expression 
of 'this should not have failed, panic'.

Thus we'd avoid all these duplicated BUG_ON() places.

	Ingo

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Cyrill Gorcunov]

[Ingo Molnar - Sun, May 03, 2009 at 10:48:47AM +0200]
| 
| * Jack Steiner <steiner@sgi.com> wrote:
| 
| > >  arch/x86/kernel/apic/x2apic_uv_x.c |    3 +++
| > >  1 file changed, 3 insertions(+)
| > > 
| > > Index: linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
| > > =====================================================================
| > > --- linux-2.6.git.orig/arch/x86/kernel/apic/x2apic_uv_x.c
| > > +++ linux-2.6.git/arch/x86/kernel/apic/x2apic_uv_x.c
| > > @@ -584,15 +584,18 @@ void __init uv_system_init(void)
| > >  
| > >  	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
| > >  	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
| > > +	BUG_ON(!uv_blade_info);
| > >  
| > >  	get_lowmem_redirect(&lowmem_redir_base, &lowmem_redir_size);
| > >  
| > >  	bytes = sizeof(uv_node_to_blade[0]) * num_possible_nodes();
| > >  	uv_node_to_blade = kmalloc(bytes, GFP_KERNEL);
| > > +	BUG_ON(!uv_node_to_blade);
| > >  	memset(uv_node_to_blade, 255, bytes);
| > >  
| > >  	bytes = sizeof(uv_cpu_to_blade[0]) * num_possible_cpus();
| > >  	uv_cpu_to_blade = kmalloc(bytes, GFP_KERNEL);
| > > +	BUG_ON(!uv_cpu_to_blade);
| > >  	memset(uv_cpu_to_blade, 255, bytes);
| 
| Hm, would be nice if we had a __GFP_PANIC variant in kmalloc that 
| would just panic straight in the allocator, when allocation failure 
| is not acceptable. (Andrew Cc:-ed)
| 
| It does not increase the priority of the allocation nor does it 
| trigger any 'dont fail' logic - it is simply the central expression 
| of 'this should not have failed, panic'.
| 
| Thus we'd avoid all these duplicated BUG_ON() places.
| 
| 	Ingo
| 

Yes, we have SLAB_PANIC, the similar (in behaviour) could be
done for kmalloc. (Pekka and Rik CC'ed as well)

	-- Cyrill

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[David Rientjes]

On Sun, 3 May 2009, Ingo Molnar wrote:

> Hm, would be nice if we had a __GFP_PANIC variant in kmalloc that 
> would just panic straight in the allocator, when allocation failure 
> is not acceptable. (Andrew Cc:-ed)
> 
> It does not increase the priority of the allocation nor does it 
> trigger any 'dont fail' logic - it is simply the central expression 
> of 'this should not have failed, panic'.
> 

SLUB stores two new slab allocation orders: the cache's adjustable order 
which is calculated at kmem_cache_create(), and the smallest order that 
can accommodate at least one object allocation.  The latter is used as a 
fallback when the former fails in the page allocator.

So for __GFP_PANIC to work in this case, it could not be implemented in 
the page allocator (SLUB also passes __GFP_NORETRY for new slabs) but 
rather above it in allocate_slab().  It would then be a no-op for 
alloc_pages().

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Cyrill Gorcunov]

[David Rientjes - Sun, May 03, 2009 at 02:09:50AM -0700]
| On Sun, 3 May 2009, Ingo Molnar wrote:
| 
| > Hm, would be nice if we had a __GFP_PANIC variant in kmalloc that 
| > would just panic straight in the allocator, when allocation failure 
| > is not acceptable. (Andrew Cc:-ed)
| > 
| > It does not increase the priority of the allocation nor does it 
| > trigger any 'dont fail' logic - it is simply the central expression 
| > of 'this should not have failed, panic'.
| > 
| 
| SLUB stores two new slab allocation orders: the cache's adjustable order 
| which is calculated at kmem_cache_create(), and the smallest order that 
| can accommodate at least one object allocation.  The latter is used as a 
| fallback when the former fails in the page allocator.
| 
| So for __GFP_PANIC to work in this case, it could not be implemented in 
| the page allocator (SLUB also passes __GFP_NORETRY for new slabs) but 
| rather above it in allocate_slab().  It would then be a no-op for 
| alloc_pages().
| 

What if instead of tear thru all these cases we implement
a special wrapper (say kmalloc_panic)? Almost the same
are done for bootmem allocator (__alloc_bootmem_nopanic).

It seems __GFP_PANIC would not be that popular anyway
and what is more important -- we would add additional
check the flag somewhere inside mm code (which will
be not needed most the time).

	-- Cyrill

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[David Rientjes]

On Sun, 3 May 2009, Cyrill Gorcunov wrote:

> What if instead of tear thru all these cases we implement
> a special wrapper (say kmalloc_panic)? Almost the same
> are done for bootmem allocator (__alloc_bootmem_nopanic).
> 

This is certainly what would be required to avoid a largely unnecessary 
conditional in a very hot path.  __GFP_PANIC couldn't be implemented any 
lower than the kmalloc() functions if it is to be consistency because this 
is the only point where we decide whether to go through the slab allocator 
or do pass-through to the page allocator (SLUB_MAX_SIZE in slub).  With 
the work currently being done to make the page allocator faster, 
SLUB_MAX_SIZE is bound to be reduced at some point.

> It seems __GFP_PANIC would not be that popular anyway
> and what is more important -- we would add additional
> check the flag somewhere inside mm code (which will
> be not needed most the time).
> 

The advantage of implementing your kmalloc_panic() idea is that it 
provides a centralized place where diagostic information could be emitted 
that shows, for instance, the allocation order and gfp flags for code 
paths where it isn't immediately obvious.  Even with that information, 
however, it doesn't provide any additional insight that could help to fix 
the problem other than insufficient memory of that type and order.

I don't think it's worth implementing the necessary variations just to 
panic on NULL such as kmalloc_node, etc.

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Pekka Enberg]

Hi David,

On Sun, May 3, 2009 at 12:09 PM, David Rientjes <rientjes@google.com> wrote:
> SLUB stores two new slab allocation orders: the cache's adjustable order
> which is calculated at kmem_cache_create(), and the smallest order that
> can accommodate at least one object allocation.  The latter is used as a
> fallback when the former fails in the page allocator.
>
> So for __GFP_PANIC to work in this case, it could not be implemented in
> the page allocator (SLUB also passes __GFP_NORETRY for new slabs) but
> rather above it in allocate_slab().  It would then be a no-op for
> alloc_pages().

It's probably better to implement __GFP_PANIC in alloc_pages() because
of kmalloc_large(). You can easily mask the __GFP_PANIC from the first
call to alloc_slab_page() where we use __GFP_NOWARN to suppress
out-of-memory warnings.

But anyway, enough talk, show me the patch! :-)

                                          Pekka

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Cyrill Gorcunov]

[Pekka Enberg - Sun, May 03, 2009 at 12:59:13PM +0300]
| Hi David,
| 
| On Sun, May 3, 2009 at 12:09 PM, David Rientjes <rientjes@google.com> wrote:
| > SLUB stores two new slab allocation orders: the cache's adjustable order
| > which is calculated at kmem_cache_create(), and the smallest order that
| > can accommodate at least one object allocation.  The latter is used as a
| > fallback when the former fails in the page allocator.
| >
| > So for __GFP_PANIC to work in this case, it could not be implemented in
| > the page allocator (SLUB also passes __GFP_NORETRY for new slabs) but
| > rather above it in allocate_slab().  It would then be a no-op for
| > alloc_pages().
| 
| It's probably better to implement __GFP_PANIC in alloc_pages() because
| of kmalloc_large(). You can easily mask the __GFP_PANIC from the first
| call to alloc_slab_page() where we use __GFP_NOWARN to suppress
| out-of-memory warnings.
| 
| But anyway, enough talk, show me the patch! :-)
| 
|                                           Pekka
| 

I was thinking about the approach showed below.

Note even if we will agree on this idea a number
of questions remain opened -- like where is a better
place to define kmalloc_panic in slub/slab_def.h
or rather in slab.h. Should we include kernel.h
to have panic and pr_ properly defined?

I don't dare start/introduce handling of __GFP_PANIC
flag since it would require more efforts to be done
correctly and what is more important -- for most
cases we would just don't need it.

	-- Cyrill

---
 include/linux/slab_def.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: linux-2.6.git/include/linux/slab_def.h
=====================================================================
--- linux-2.6.git.orig/include/linux/slab_def.h
+++ linux-2.6.git/include/linux/slab_def.h
@@ -220,4 +220,16 @@ found:
 
 #endif	/* CONFIG_NUMA */
 
+static inline void *kmalloc_panic(size_t size, gfp_t flags)
+{
+	void *p = kmalloc(size, flags);
+
+	if (size && ZERO_OR_NULL_PTR(p)) {
+		pr_emerg("Failed to allocate: %z bytes\n", size);
+		panic("Out of memory\n");
+	}
+
+	return p;
+}
+
 #endif	/* _LINUX_SLAB_DEF_H */

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Pekka Enberg]

Hi Cyrill,

On Sun, 2009-05-03 at 16:12 +0400, Cyrill Gorcunov wrote:
> [Pekka Enberg - Sun, May 03, 2009 at 12:59:13PM +0300]
> | Hi David,
> | 
> | On Sun, May 3, 2009 at 12:09 PM, David Rientjes <rientjes@google.com> wrote:
> | > SLUB stores two new slab allocation orders: the cache's adjustable order
> | > which is calculated at kmem_cache_create(), and the smallest order that
> | > can accommodate at least one object allocation.  The latter is used as a
> | > fallback when the former fails in the page allocator.
> | >
> | > So for __GFP_PANIC to work in this case, it could not be implemented in
> | > the page allocator (SLUB also passes __GFP_NORETRY for new slabs) but
> | > rather above it in allocate_slab().  It would then be a no-op for
> | > alloc_pages().
> | 
> | It's probably better to implement __GFP_PANIC in alloc_pages() because
> | of kmalloc_large(). You can easily mask the __GFP_PANIC from the first
> | call to alloc_slab_page() where we use __GFP_NOWARN to suppress
> | out-of-memory warnings.
> | 
> | But anyway, enough talk, show me the patch! :-)
> | 
> |                                           Pekka
> | 
> 
> I was thinking about the approach showed below.
> 
> Note even if we will agree on this idea a number
> of questions remain opened -- like where is a better
> place to define kmalloc_panic in slub/slab_def.h
> or rather in slab.h. Should we include kernel.h
> to have panic and pr_ properly defined?
> 
> I don't dare start/introduce handling of __GFP_PANIC
> flag since it would require more efforts to be done
> correctly and what is more important -- for most
> cases we would just don't need it.
> 
> 	-- Cyrill
> 
> ---
>  include/linux/slab_def.h |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> Index: linux-2.6.git/include/linux/slab_def.h
> =====================================================================
> --- linux-2.6.git.orig/include/linux/slab_def.h
> +++ linux-2.6.git/include/linux/slab_def.h
> @@ -220,4 +220,16 @@ found:
>  
>  #endif	/* CONFIG_NUMA */
>  
> +static inline void *kmalloc_panic(size_t size, gfp_t flags)
> +{
> +	void *p = kmalloc(size, flags);
> +
> +	if (size && ZERO_OR_NULL_PTR(p)) {
> +		pr_emerg("Failed to allocate: %z bytes\n", size);
> +		panic("Out of memory\n");
> +	}
> +
> +	return p;
> +}
> +
>  #endif	/* _LINUX_SLAB_DEF_H */

I don't like this approach because you'd need to do a kzalloc_panic()
and so on for it to be truly useful. What's wrong with adding a
__GFP_PANIC check in __alloc_pages_internal() (or whatever it's called
in -mm now) next to __GFP_NOWARN?

			Pekka

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Cyrill Gorcunov]

[Pekka Enberg - Sun, May 03, 2009 at 03:27:00PM +0300]
| Hi Cyrill,
| 
| On Sun, 2009-05-03 at 16:12 +0400, Cyrill Gorcunov wrote:
| > [Pekka Enberg - Sun, May 03, 2009 at 12:59:13PM +0300]
| > | Hi David,
| > | 
| > | On Sun, May 3, 2009 at 12:09 PM, David Rientjes <rientjes@google.com> wrote:
| > | > SLUB stores two new slab allocation orders: the cache's adjustable order
| > | > which is calculated at kmem_cache_create(), and the smallest order that
| > | > can accommodate at least one object allocation.  The latter is used as a
| > | > fallback when the former fails in the page allocator.
| > | >
| > | > So for __GFP_PANIC to work in this case, it could not be implemented in
| > | > the page allocator (SLUB also passes __GFP_NORETRY for new slabs) but
| > | > rather above it in allocate_slab().  It would then be a no-op for
| > | > alloc_pages().
| > | 
| > | It's probably better to implement __GFP_PANIC in alloc_pages() because
| > | of kmalloc_large(). You can easily mask the __GFP_PANIC from the first
| > | call to alloc_slab_page() where we use __GFP_NOWARN to suppress
| > | out-of-memory warnings.
| > | 
| > | But anyway, enough talk, show me the patch! :-)
| > | 
| > |                                           Pekka
| > | 
| > 
| > I was thinking about the approach showed below.
| > 
| > Note even if we will agree on this idea a number
| > of questions remain opened -- like where is a better
| > place to define kmalloc_panic in slub/slab_def.h
| > or rather in slab.h. Should we include kernel.h
| > to have panic and pr_ properly defined?
| > 
| > I don't dare start/introduce handling of __GFP_PANIC
| > flag since it would require more efforts to be done
| > correctly and what is more important -- for most
| > cases we would just don't need it.
| > 
| > 	-- Cyrill
| > 
| > ---
| >  include/linux/slab_def.h |   12 ++++++++++++
| >  1 file changed, 12 insertions(+)
| > 
| > Index: linux-2.6.git/include/linux/slab_def.h
| > =====================================================================
| > --- linux-2.6.git.orig/include/linux/slab_def.h
| > +++ linux-2.6.git/include/linux/slab_def.h
| > @@ -220,4 +220,16 @@ found:
| >  
| >  #endif	/* CONFIG_NUMA */
| >  
| > +static inline void *kmalloc_panic(size_t size, gfp_t flags)
| > +{
| > +	void *p = kmalloc(size, flags);
| > +
| > +	if (size && ZERO_OR_NULL_PTR(p)) {
| > +		pr_emerg("Failed to allocate: %z bytes\n", size);
| > +		panic("Out of memory\n");
| > +	}
| > +
| > +	return p;
| > +}
| > +
| >  #endif	/* _LINUX_SLAB_DEF_H */
| 
| I don't like this approach because you'd need to do a kzalloc_panic()
| and so on for it to be truly useful. What's wrong with adding a
| __GFP_PANIC check in __alloc_pages_internal() (or whatever it's called
| in -mm now) next to __GFP_NOWARN?
| 
| 			Pekka
| 

Hi Pekka,

ufortunatelly __alloc_pages_internal is not the only place where
we do return NULL from kmalloc. As example - failslab facility
(in slab_alloc call). Anyway -- I'll take a closer look.

	-- Cyrill

Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init

[Pekka Enberg]

On Sun, May 3, 2009 at 5:38 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> [Pekka Enberg - Sun, May 03, 2009 at 03:27:00PM +0300]
> | Hi Cyrill,
> |
> | On Sun, 2009-05-03 at 16:12 +0400, Cyrill Gorcunov wrote:
> | > [Pekka Enberg - Sun, May 03, 2009 at 12:59:13PM +0300]
> | > | Hi David,
> | > |
> | > | On Sun, May 3, 2009 at 12:09 PM, David Rientjes <rientjes@google.com> wrote:
> | > | > SLUB stores two new slab allocation orders: the cache's adjustable order
> | > | > which is calculated at kmem_cache_create(), and the smallest order that
> | > | > can accommodate at least one object allocation.  The latter is used as a
> | > | > fallback when the former fails in the page allocator.
> | > | >
> | > | > So for __GFP_PANIC to work in this case, it could not be implemented in
> | > | > the page allocator (SLUB also passes __GFP_NORETRY for new slabs) but
> | > | > rather above it in allocate_slab().  It would then be a no-op for
> | > | > alloc_pages().
> | > |
> | > | It's probably better to implement __GFP_PANIC in alloc_pages() because
> | > | of kmalloc_large(). You can easily mask the __GFP_PANIC from the first
> | > | call to alloc_slab_page() where we use __GFP_NOWARN to suppress
> | > | out-of-memory warnings.
> | > |
> | > | But anyway, enough talk, show me the patch! :-)
> | > |
> | > |                                           Pekka
> | > |
> | >
> | > I was thinking about the approach showed below.
> | >
> | > Note even if we will agree on this idea a number
> | > of questions remain opened -- like where is a better
> | > place to define kmalloc_panic in slub/slab_def.h
> | > or rather in slab.h. Should we include kernel.h
> | > to have panic and pr_ properly defined?
> | >
> | > I don't dare start/introduce handling of __GFP_PANIC
> | > flag since it would require more efforts to be done
> | > correctly and what is more important -- for most
> | > cases we would just don't need it.
> | >
> | >     -- Cyrill
> | >
> | > ---
> | >  include/linux/slab_def.h |   12 ++++++++++++
> | >  1 file changed, 12 insertions(+)
> | >
> | > Index: linux-2.6.git/include/linux/slab_def.h
> | > =====================================================================
> | > --- linux-2.6.git.orig/include/linux/slab_def.h
> | > +++ linux-2.6.git/include/linux/slab_def.h
> | > @@ -220,4 +220,16 @@ found:
> | >
> | >  #endif     /* CONFIG_NUMA */
> | >
> | > +static inline void *kmalloc_panic(size_t size, gfp_t flags)
> | > +{
> | > +   void *p = kmalloc(size, flags);
> | > +
> | > +   if (size && ZERO_OR_NULL_PTR(p)) {
> | > +           pr_emerg("Failed to allocate: %z bytes\n", size);
> | > +           panic("Out of memory\n");
> | > +   }
> | > +
> | > +   return p;
> | > +}
> | > +
> | >  #endif     /* _LINUX_SLAB_DEF_H */
> |
> | I don't like this approach because you'd need to do a kzalloc_panic()
> | and so on for it to be truly useful. What's wrong with adding a
> | __GFP_PANIC check in __alloc_pages_internal() (or whatever it's called
> | in -mm now) next to __GFP_NOWARN?
> |
> |                       Pekka
> |
>
> Hi Pekka,
>
> ufortunatelly __alloc_pages_internal is not the only place where
> we do return NULL from kmalloc. As example - failslab facility
> (in slab_alloc call). Anyway -- I'll take a closer look.

Right. I think failslab needs some fixing _not_ to return NULL if
__GFP_PANIC is set.

introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Sun, May 03, 2009 at 07:54:38PM +0300]
...
| > | I don't like this approach because you'd need to do a kzalloc_panic()
| > | and so on for it to be truly useful. What's wrong with adding a
| > | __GFP_PANIC check in __alloc_pages_internal() (or whatever it's called
| > | in -mm now) next to __GFP_NOWARN?
| > |
| > |                       Pekka
| > |
| >
| > Hi Pekka,
| >
| > ufortunatelly __alloc_pages_internal is not the only place where
| > we do return NULL from kmalloc. As example - failslab facility
| > (in slab_alloc call). Anyway -- I'll take a closer look.
| 
| Right. I think failslab needs some fixing _not_ to return NULL if
| __GFP_PANIC is set.
| 

Ok, as a first raw draft (_not_ covering all the cases) it could
be something like this. It touches only __alloc_pages_internal
and we have to bespread as well:

1) alloc_pages with order >= MAX_ORDER (gfp.h)
2) the same for alloc_pages_node (both used by SLOB)
3) all __kmalloc should be explored as well.
4) ???

Anyway -- take a look on __alloc_pages_internal part :)

	-- Cyrill

---
 include/linux/gfp.h |    4 +++-
 mm/page_alloc.c     |    8 +++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

Index: linux-2.6.git/include/linux/gfp.h
=====================================================================
--- linux-2.6.git.orig/include/linux/gfp.h
+++ linux-2.6.git/include/linux/gfp.h
@@ -58,7 +58,9 @@ struct vm_area_struct;
 #define __GFP_NOTRACK	((__force gfp_t)0)
 #endif
 
-#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
+#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
+
+#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /* This equals 0, but use constants in case they ever change */
Index: linux-2.6.git/mm/page_alloc.c
=====================================================================
--- linux-2.6.git.orig/mm/page_alloc.c
+++ linux-2.6.git/mm/page_alloc.c
@@ -1496,7 +1496,7 @@ __alloc_pages_internal(gfp_t gfp_mask, u
 	might_sleep_if(wait);
 
 	if (should_fail_alloc_page(gfp_mask, order))
-		return NULL;
+		goto nopage;
 
 restart:
 	z = zonelist->_zonerefs;  /* the list of zones suitable for gfp_mask */
@@ -1506,7 +1506,7 @@ restart:
 		 * Happens if we have an empty zonelist as a result of
 		 * GFP_THISNODE being used on a memoryless node
 		 */
-		return NULL;
+		goto nopage;
 	}
 
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
@@ -1685,7 +1685,9 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
-	return page;
+	if (unlikely(gfp_mask & __GFP_PANIC))
+		panic("Out of memory: panic due to __GFP_PANIC\n");
+	return NULL;
 got_pg:
 	if (kmemcheck_enabled)
 		kmemcheck_pagealloc_alloc(page, order, gfp_mask);

Re: introducing __GFP_PANIC

[Pekka Enberg]

On Sun, May 3, 2009 at 8:23 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | > Hi Pekka,
> | >
> | > ufortunatelly __alloc_pages_internal is not the only place where
> | > we do return NULL from kmalloc. As example - failslab facility
> | > (in slab_alloc call). Anyway -- I'll take a closer look.
> |
> | Right. I think failslab needs some fixing _not_ to return NULL if
> | __GFP_PANIC is set.
> |
>
> Ok, as a first raw draft (_not_ covering all the cases) it could
> be something like this. It touches only __alloc_pages_internal
> and we have to bespread as well:
>
> 1) alloc_pages with order >= MAX_ORDER (gfp.h)
> 2) the same for alloc_pages_node (both used by SLOB)
> 3) all __kmalloc should be explored as well.
> 4) ???
>
> Anyway -- take a look on __alloc_pages_internal part :)
>
>        -- Cyrill
>
> ---
>  include/linux/gfp.h |    4 +++-
>  mm/page_alloc.c     |    8 +++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.git/include/linux/gfp.h
> =====================================================================
> --- linux-2.6.git.orig/include/linux/gfp.h
> +++ linux-2.6.git/include/linux/gfp.h
> @@ -58,7 +58,9 @@ struct vm_area_struct;
>  #define __GFP_NOTRACK  ((__force gfp_t)0)
>  #endif
>
> -#define __GFP_BITS_SHIFT 22    /* Room for 22 __GFP_FOO bits */
> +#define __GFP_PANIC    ((__force gfp_t)0x400000u) /* Panic on page alloction failure */
> +
> +#define __GFP_BITS_SHIFT 23    /* Room for 23 __GFP_FOO bits */
>  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
>
>  /* This equals 0, but use constants in case they ever change */
> Index: linux-2.6.git/mm/page_alloc.c
> =====================================================================
> --- linux-2.6.git.orig/mm/page_alloc.c
> +++ linux-2.6.git/mm/page_alloc.c
> @@ -1496,7 +1496,7 @@ __alloc_pages_internal(gfp_t gfp_mask, u
>        might_sleep_if(wait);
>
>        if (should_fail_alloc_page(gfp_mask, order))
> -               return NULL;
> +               goto nopage;

The point of fault injection is to increase coverage out-of-memory
error handling code. So this doesn't make much sense to me. Why would
you want to cause a __GFP_PANIC call-sites to panic()? It doesn't help
testing one bit.

So I still think you should just fix up should_fail_alloc_page() _not_
to return true if __GFP_PANIC is set.

>
>  restart:
>        z = zonelist->_zonerefs;  /* the list of zones suitable for gfp_mask */
> @@ -1506,7 +1506,7 @@ restart:
>                 * Happens if we have an empty zonelist as a result of
>                 * GFP_THISNODE being used on a memoryless node
>                 */
> -               return NULL;
> +               goto nopage;
>        }
>
>        page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
> @@ -1685,7 +1685,9 @@ nopage:
>                dump_stack();
>                show_mem();
>        }
> -       return page;
> +       if (unlikely(gfp_mask & __GFP_PANIC))
> +               panic("Out of memory: panic due to __GFP_PANIC\n");
> +       return NULL;
>  got_pg:
>        if (kmemcheck_enabled)
>                kmemcheck_pagealloc_alloc(page, order, gfp_mask);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Sun, May 03, 2009 at 08:38:01PM +0300]
| On Sun, May 3, 2009 at 8:23 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | > Hi Pekka,
| > | >
| > | > ufortunatelly __alloc_pages_internal is not the only place where
| > | > we do return NULL from kmalloc. As example - failslab facility
| > | > (in slab_alloc call). Anyway -- I'll take a closer look.
| > |
| > | Right. I think failslab needs some fixing _not_ to return NULL if
| > | __GFP_PANIC is set.
| > |
| >
| > Ok, as a first raw draft (_not_ covering all the cases) it could
| > be something like this. It touches only __alloc_pages_internal
| > and we have to bespread as well:
| >
| > 1) alloc_pages with order >= MAX_ORDER (gfp.h)
| > 2) the same for alloc_pages_node (both used by SLOB)
| > 3) all __kmalloc should be explored as well.
| > 4) ???
| >
| > Anyway -- take a look on __alloc_pages_internal part :)
| >
| >        -- Cyrill
| >
| > ---
| >  include/linux/gfp.h |    4 +++-
| >  mm/page_alloc.c     |    8 +++++---
| >  2 files changed, 8 insertions(+), 4 deletions(-)
| >
| > Index: linux-2.6.git/include/linux/gfp.h
| > =====================================================================
| > --- linux-2.6.git.orig/include/linux/gfp.h
| > +++ linux-2.6.git/include/linux/gfp.h
| > @@ -58,7 +58,9 @@ struct vm_area_struct;
| >  #define __GFP_NOTRACK  ((__force gfp_t)0)
| >  #endif
| >
| > -#define __GFP_BITS_SHIFT 22    /* Room for 22 __GFP_FOO bits */
| > +#define __GFP_PANIC    ((__force gfp_t)0x400000u) /* Panic on page alloction failure */
| > +
| > +#define __GFP_BITS_SHIFT 23    /* Room for 23 __GFP_FOO bits */
| >  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
| >
| >  /* This equals 0, but use constants in case they ever change */
| > Index: linux-2.6.git/mm/page_alloc.c
| > =====================================================================
| > --- linux-2.6.git.orig/mm/page_alloc.c
| > +++ linux-2.6.git/mm/page_alloc.c
| > @@ -1496,7 +1496,7 @@ __alloc_pages_internal(gfp_t gfp_mask, u
| >        might_sleep_if(wait);
| >
| >        if (should_fail_alloc_page(gfp_mask, order))
| > -               return NULL;
| > +               goto nopage;
| 
| The point of fault injection is to increase coverage out-of-memory
| error handling code. So this doesn't make much sense to me. Why would
| you want to cause a __GFP_PANIC call-sites to panic()? It doesn't help
| testing one bit.
| 
| So I still think you should just fix up should_fail_alloc_page() _not_
| to return true if __GFP_PANIC is set.

Ah, yes, I see. I just though that caller requested to panic
if allocation failed and it''ll got it regardless of the reason.
But for fail injection it doesn't make sence at all indeed. Will
fix, thanks!

...
 
	-- Cyrill

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Sun, May 03, 2009 at 08:38:01PM +0300]
| On Sun, May 3, 2009 at 8:23 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | > Hi Pekka,
| > | >
| > | > ufortunatelly __alloc_pages_internal is not the only place where
| > | > we do return NULL from kmalloc. As example - failslab facility
| > | > (in slab_alloc call). Anyway -- I'll take a closer look.
| > |
| > | Right. I think failslab needs some fixing _not_ to return NULL if
| > | __GFP_PANIC is set.
| > |
| >
| > Ok, as a first raw draft (_not_ covering all the cases) it could
| > be something like this. It touches only __alloc_pages_internal
| > and we have to bespread as well:
| >
| > 1) alloc_pages with order >= MAX_ORDER (gfp.h)
| > 2) the same for alloc_pages_node (both used by SLOB)
| > 3) all __kmalloc should be explored as well.
| > 4) ???
| >
| > Anyway -- take a look on __alloc_pages_internal part :)
| >
| >        -- Cyrill
| >
| > ---
| >  include/linux/gfp.h |    4 +++-
| >  mm/page_alloc.c     |    8 +++++---
| >  2 files changed, 8 insertions(+), 4 deletions(-)
| >
| > Index: linux-2.6.git/include/linux/gfp.h
| > =====================================================================
| > --- linux-2.6.git.orig/include/linux/gfp.h
| > +++ linux-2.6.git/include/linux/gfp.h
| > @@ -58,7 +58,9 @@ struct vm_area_struct;
| >  #define __GFP_NOTRACK  ((__force gfp_t)0)
| >  #endif
| >
| > -#define __GFP_BITS_SHIFT 22    /* Room for 22 __GFP_FOO bits */
| > +#define __GFP_PANIC    ((__force gfp_t)0x400000u) /* Panic on page alloction failure */
| > +
| > +#define __GFP_BITS_SHIFT 23    /* Room for 23 __GFP_FOO bits */
| >  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
| >
| >  /* This equals 0, but use constants in case they ever change */
| > Index: linux-2.6.git/mm/page_alloc.c
| > =====================================================================
| > --- linux-2.6.git.orig/mm/page_alloc.c
| > +++ linux-2.6.git/mm/page_alloc.c
| > @@ -1496,7 +1496,7 @@ __alloc_pages_internal(gfp_t gfp_mask, u
| >        might_sleep_if(wait);
| >
| >        if (should_fail_alloc_page(gfp_mask, order))
| > -               return NULL;
| > +               goto nopage;
| 
| The point of fault injection is to increase coverage out-of-memory
| error handling code. So this doesn't make much sense to me. Why would
| you want to cause a __GFP_PANIC call-sites to panic()? It doesn't help
| testing one bit.
| 
| So I still think you should just fix up should_fail_alloc_page() _not_
| to return true if __GFP_PANIC is set.
| 

Just before I get some sleep. Here is an another attempt.

Not covered cases I know of:

1) SLAB with builtin constant as size and we don't find appropriate
   cache for it do return NULL
2) alloc_pages_node and alloc_pages do check for MAX_ORDER which
   could be exceeded as well and return NULL then.

ok, here is a preliminary patch just to hear some complains :)

	-- Cyrill
---
 include/linux/gfp.h |    4 +++-
 mm/failslab.c       |    3 +++
 mm/page_alloc.c     |    8 ++++++--
 3 files changed, 12 insertions(+), 3 deletions(-)

Index: linux-2.6.git/include/linux/gfp.h
=====================================================================
--- linux-2.6.git.orig/include/linux/gfp.h
+++ linux-2.6.git/include/linux/gfp.h
@@ -58,7 +58,9 @@ struct vm_area_struct;
 #define __GFP_NOTRACK	((__force gfp_t)0)
 #endif
 
-#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
+#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
+
+#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /* This equals 0, but use constants in case they ever change */
Index: linux-2.6.git/mm/failslab.c
=====================================================================
--- linux-2.6.git.orig/mm/failslab.c
+++ linux-2.6.git/mm/failslab.c
@@ -17,6 +17,9 @@ bool should_failslab(size_t size, gfp_t 
 	if (gfpflags & __GFP_NOFAIL)
 		return false;
 
+	if (gfpflags & __GFP_PANIC)
+		return false;
+
         if (failslab.ignore_gfp_wait && (gfpflags & __GFP_WAIT))
 		return false;
 
Index: linux-2.6.git/mm/page_alloc.c
=====================================================================
--- linux-2.6.git.orig/mm/page_alloc.c
+++ linux-2.6.git/mm/page_alloc.c
@@ -1185,6 +1185,8 @@ static int should_fail_alloc_page(gfp_t 
 		return 0;
 	if (gfp_mask & __GFP_NOFAIL)
 		return 0;
+	if (gfp_mask & __GFP_PANIC)
+		return 0;
 	if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
 		return 0;
 	if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & __GFP_WAIT))
@@ -1506,7 +1508,7 @@ restart:
 		 * Happens if we have an empty zonelist as a result of
 		 * GFP_THISNODE being used on a memoryless node
 		 */
-		return NULL;
+		goto nopage;
 	}
 
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
@@ -1685,7 +1687,9 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
-	return page;
+	if (unlikely(gfp_mask & __GFP_PANIC))
+		panic("Out of memory: panic due to __GFP_PANIC\n");
+	return NULL;
 got_pg:
 	if (kmemcheck_enabled)
 		kmemcheck_pagealloc_alloc(page, order, gfp_mask);

Re: introducing __GFP_PANIC

[David Rientjes]

On Mon, 4 May 2009, Cyrill Gorcunov wrote:

> @@ -1685,7 +1687,9 @@ nopage:
>  		dump_stack();
>  		show_mem();
>  	}
> -	return page;
> +	if (unlikely(gfp_mask & __GFP_PANIC))
> +		panic("Out of memory: panic due to __GFP_PANIC\n");
> +	return NULL;
>  got_pg:
>  	if (kmemcheck_enabled)
>  		kmemcheck_pagealloc_alloc(page, order, gfp_mask);
> 

If we really want a __GFP_PANIC flag for this purpose, I'd recommend also 
emitting the order and gfpmask in the panic() message since it may not be 
immediately obvious from the caller.

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[David Rientjes - Sun, May 03, 2009 at 01:58:49PM -0700]
| On Mon, 4 May 2009, Cyrill Gorcunov wrote:
| 
| > @@ -1685,7 +1687,9 @@ nopage:
| >  		dump_stack();
| >  		show_mem();
| >  	}
| > -	return page;
| > +	if (unlikely(gfp_mask & __GFP_PANIC))
| > +		panic("Out of memory: panic due to __GFP_PANIC\n");
| > +	return NULL;
| >  got_pg:
| >  	if (kmemcheck_enabled)
| >  		kmemcheck_pagealloc_alloc(page, order, gfp_mask);
| > 
| 
| If we really want a __GFP_PANIC flag for this purpose, I'd recommend also 
| emitting the order and gfpmask in the panic() message since it may not be 
| immediately obvious from the caller.
|

Thanks for note, David!

Here is an updated version for review. I hope I've covered
all cases. Complains are welcome :)
 
	-- Cyrill
---
mm: introduce __GFP_PANIC

Sometime we need that memory obtained via kmalloc
should always be granted. If there is no enough
memory we just can't go further.

For such a case we introduce __GFP_PANIC panic
modificator. If memory can't be granted -- we just
panic.

Note that __GFP_PANIC implicitly turn off failslab
facility on such kind calls.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 include/linux/gfp.h      |   13 ++++++++++---
 include/linux/slab_def.h |    1 +
 mm/failslab.c            |    3 +++
 mm/page_alloc.c          |   17 +++++++++++++++--
 4 files changed, 29 insertions(+), 5 deletions(-)

Index: linux-2.6.git/include/linux/gfp.h
=====================================================================
--- linux-2.6.git.orig/include/linux/gfp.h
+++ linux-2.6.git/include/linux/gfp.h
@@ -7,6 +7,7 @@
 #include <linux/topology.h>
 
 struct vm_area_struct;
+void oom_panic(gfp_t gfp_mask, unsigned int order);
 
 /*
  * GFP bitmasks..
@@ -58,7 +59,9 @@ struct vm_area_struct;
 #define __GFP_NOTRACK	((__force gfp_t)0)
 #endif
 
-#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
+#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
+
+#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /* This equals 0, but use constants in case they ever change */
@@ -196,8 +199,10 @@ __alloc_pages_nodemask(gfp_t gfp_mask, u
 static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
 						unsigned int order)
 {
-	if (unlikely(order >= MAX_ORDER))
+	if (unlikely(order >= MAX_ORDER)) {
+		oom_panic(gfp_mask, order);
 		return NULL;
+	}
 
 	/* Unknown node is current node */
 	if (nid < 0)
@@ -212,8 +217,10 @@ extern struct page *alloc_pages_current(
 static inline struct page *
 alloc_pages(gfp_t gfp_mask, unsigned int order)
 {
-	if (unlikely(order >= MAX_ORDER))
+	if (unlikely(order >= MAX_ORDER)) {
+		oom_panic(gfp_mask, order);
 		return NULL;
+	}
 
 	return alloc_pages_current(gfp_mask, order);
 }
Index: linux-2.6.git/include/linux/slab_def.h
=====================================================================
--- linux-2.6.git.orig/include/linux/slab_def.h
+++ linux-2.6.git/include/linux/slab_def.h
@@ -143,6 +143,7 @@ static __always_inline void *kmalloc(siz
 			i++;
 #include <linux/kmalloc_sizes.h>
 #undef CACHE
+		oom_panic(flags, get_order(size));
 		return NULL;
 found:
 #ifdef CONFIG_ZONE_DMA
Index: linux-2.6.git/mm/failslab.c
=====================================================================
--- linux-2.6.git.orig/mm/failslab.c
+++ linux-2.6.git/mm/failslab.c
@@ -17,6 +17,9 @@ bool should_failslab(size_t size, gfp_t 
 	if (gfpflags & __GFP_NOFAIL)
 		return false;
 
+	if (gfpflags & __GFP_PANIC)
+		return false;
+
         if (failslab.ignore_gfp_wait && (gfpflags & __GFP_WAIT))
 		return false;
 
Index: linux-2.6.git/mm/page_alloc.c
=====================================================================
--- linux-2.6.git.orig/mm/page_alloc.c
+++ linux-2.6.git/mm/page_alloc.c
@@ -1185,6 +1185,8 @@ static int should_fail_alloc_page(gfp_t 
 		return 0;
 	if (gfp_mask & __GFP_NOFAIL)
 		return 0;
+	if (gfp_mask & __GFP_PANIC)
+		return 0;
 	if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
 		return 0;
 	if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & __GFP_WAIT))
@@ -1472,6 +1474,16 @@ try_next_zone:
 	return page;
 }
 
+void oom_panic(gfp_t gfp_mask, unsigned int order)
+{
+	if (likely(!(gfp_mask & __GFP_PANIC)))
+		return;
+
+	panic("Out of memory: panic due to __GFP_PANIC.\n"
+		"%s order:%d, mode:0x%x\n", current->comm,
+		order, gfp_mask);
+}
+
 /*
  * This is the 'heart' of the zoned buddy allocator.
  */
@@ -1506,7 +1518,7 @@ restart:
 		 * Happens if we have an empty zonelist as a result of
 		 * GFP_THISNODE being used on a memoryless node
 		 */
-		return NULL;
+		goto nopage;
 	}
 
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
@@ -1685,7 +1697,8 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
-	return page;
+	oom_panic(gfp_mask, order);
+	return NULL;
 got_pg:
 	if (kmemcheck_enabled)
 		kmemcheck_pagealloc_alloc(page, order, gfp_mask);

Re: introducing __GFP_PANIC

[Pekka Enberg]

Hi Cyrill,

On Mon, 2009-05-04 at 12:14 +0400, Cyrill Gorcunov wrote:
> mm: introduce __GFP_PANIC
> 
> Sometime we need that memory obtained via kmalloc
> should always be granted. If there is no enough
> memory we just can't go further.
> 
> For such a case we introduce __GFP_PANIC panic
> modificator. If memory can't be granted -- we just
> panic.
> 
> Note that __GFP_PANIC implicitly turn off failslab
> facility on such kind calls.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  include/linux/gfp.h      |   13 ++++++++++---
>  include/linux/slab_def.h |    1 +
>  mm/failslab.c            |    3 +++
>  mm/page_alloc.c          |   17 +++++++++++++++--
>  4 files changed, 29 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6.git/include/linux/gfp.h
> =====================================================================
> --- linux-2.6.git.orig/include/linux/gfp.h
> +++ linux-2.6.git/include/linux/gfp.h
> @@ -7,6 +7,7 @@
>  #include <linux/topology.h>
>  
>  struct vm_area_struct;
> +void oom_panic(gfp_t gfp_mask, unsigned int order);
>  
>  /*
>   * GFP bitmasks..
> @@ -58,7 +59,9 @@ struct vm_area_struct;
>  #define __GFP_NOTRACK	((__force gfp_t)0)
>  #endif
>  
> -#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
> +#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
> +
> +#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
>  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
>  
>  /* This equals 0, but use constants in case they ever change */
> @@ -196,8 +199,10 @@ __alloc_pages_nodemask(gfp_t gfp_mask, u
>  static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
>  						unsigned int order)
>  {
> -	if (unlikely(order >= MAX_ORDER))
> +	if (unlikely(order >= MAX_ORDER)) {
> +		oom_panic(gfp_mask, order);

This...

>  		return NULL;
> +	}
>  
>  	/* Unknown node is current node */
>  	if (nid < 0)
> @@ -212,8 +217,10 @@ extern struct page *alloc_pages_current(
>  static inline struct page *
>  alloc_pages(gfp_t gfp_mask, unsigned int order)
>  {
> -	if (unlikely(order >= MAX_ORDER))
> +	if (unlikely(order >= MAX_ORDER)) {
> +		oom_panic(gfp_mask, order);

...this...

>  		return NULL;
> +	}
>  
>  	return alloc_pages_current(gfp_mask, order);
>  }
> Index: linux-2.6.git/include/linux/slab_def.h
> =====================================================================
> --- linux-2.6.git.orig/include/linux/slab_def.h
> +++ linux-2.6.git/include/linux/slab_def.h
> @@ -143,6 +143,7 @@ static __always_inline void *kmalloc(siz
>  			i++;
>  #include <linux/kmalloc_sizes.h>
>  #undef CACHE
> +		oom_panic(flags, get_order(size));

...and this look fishy. They're static inlines that get expanded
everywhere and they're known to be performance sensitive paths. I don't
see much point in checking for >= MAX_ORDER at all because we will get a
nice oops anyway for that.

__GFP_PANIC is an annotation saying that it's okay for a particular
call-site not to check for NULL because we never expect to run out of
memory at that point. But we don't really need to panic() for all the
possible *errors*, just for the out-of-memory case.

			Pekka

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Mon, May 04, 2009 at 11:32:21AM +0300]
| Hi Cyrill,
| 
| On Mon, 2009-05-04 at 12:14 +0400, Cyrill Gorcunov wrote:
| > mm: introduce __GFP_PANIC
| > 
| > Sometime we need that memory obtained via kmalloc
| > should always be granted. If there is no enough
| > memory we just can't go further.
| > 
| > For such a case we introduce __GFP_PANIC panic
| > modificator. If memory can't be granted -- we just
| > panic.
| > 
| > Note that __GFP_PANIC implicitly turn off failslab
| > facility on such kind calls.
| > 
| > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
| > ---
| >  include/linux/gfp.h      |   13 ++++++++++---
| >  include/linux/slab_def.h |    1 +
| >  mm/failslab.c            |    3 +++
| >  mm/page_alloc.c          |   17 +++++++++++++++--
| >  4 files changed, 29 insertions(+), 5 deletions(-)
| > 
| > Index: linux-2.6.git/include/linux/gfp.h
| > =====================================================================
| > --- linux-2.6.git.orig/include/linux/gfp.h
| > +++ linux-2.6.git/include/linux/gfp.h
| > @@ -7,6 +7,7 @@
| >  #include <linux/topology.h>
| >  
| >  struct vm_area_struct;
| > +void oom_panic(gfp_t gfp_mask, unsigned int order);
| >  
| >  /*
| >   * GFP bitmasks..
| > @@ -58,7 +59,9 @@ struct vm_area_struct;
| >  #define __GFP_NOTRACK	((__force gfp_t)0)
| >  #endif
| >  
| > -#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
| > +#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
| > +
| > +#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
| >  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
| >  
| >  /* This equals 0, but use constants in case they ever change */
| > @@ -196,8 +199,10 @@ __alloc_pages_nodemask(gfp_t gfp_mask, u
| >  static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
| >  						unsigned int order)
| >  {
| > -	if (unlikely(order >= MAX_ORDER))
| > +	if (unlikely(order >= MAX_ORDER)) {
| > +		oom_panic(gfp_mask, order);
| 
| This...
| 
| >  		return NULL;
| > +	}
| >  
| >  	/* Unknown node is current node */
| >  	if (nid < 0)
| > @@ -212,8 +217,10 @@ extern struct page *alloc_pages_current(
| >  static inline struct page *
| >  alloc_pages(gfp_t gfp_mask, unsigned int order)
| >  {
| > -	if (unlikely(order >= MAX_ORDER))
| > +	if (unlikely(order >= MAX_ORDER)) {
| > +		oom_panic(gfp_mask, order);
| 
| ...this...
| 
| >  		return NULL;
| > +	}
| >  
| >  	return alloc_pages_current(gfp_mask, order);
| >  }
| > Index: linux-2.6.git/include/linux/slab_def.h
| > =====================================================================
| > --- linux-2.6.git.orig/include/linux/slab_def.h
| > +++ linux-2.6.git/include/linux/slab_def.h
| > @@ -143,6 +143,7 @@ static __always_inline void *kmalloc(siz
| >  			i++;
| >  #include <linux/kmalloc_sizes.h>
| >  #undef CACHE
| > +		oom_panic(flags, get_order(size));
| 
| ...and this look fishy. They're static inlines that get expanded
| everywhere and they're known to be performance sensitive paths. I don't
| see much point in checking for >= MAX_ORDER at all because we will get a
| nice oops anyway for that.

Yep, I noticed that it is really ugly cases to put oom_panic
here. But shouldn't we cover all the sites? If I'm not missing
something, for slab with builtin constant greater then KMALLOC_MAX_SIZE
the kmalloc caller could get NULL even if __GFP_PANIC specified.
Which in turn means __GFP_PANIC just doesn't work properly here.

Anyway, I'll update the patch with fixes you proposed should be done.
Wait a bit :)

| 
| __GFP_PANIC is an annotation saying that it's okay for a particular
| call-site not to check for NULL because we never expect to run out of
| memory at that point. But we don't really need to panic() for all the
| possible *errors*, just for the out-of-memory case.
| 
| 			Pekka
| 

Which means, __GFP_PANIC will not grant the guarantee to a caller
that he will not get NULL deref even with __GFP_PANIC specified?

Perhaps I missing something...

	-- Cyrill

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Mon, May 04, 2009 at 11:32:21AM +0300]
...
| > Index: linux-2.6.git/include/linux/slab_def.h
| > =====================================================================
| > --- linux-2.6.git.orig/include/linux/slab_def.h
| > +++ linux-2.6.git/include/linux/slab_def.h
| > @@ -143,6 +143,7 @@ static __always_inline void *kmalloc(siz
| >  			i++;
| >  #include <linux/kmalloc_sizes.h>
| >  #undef CACHE
| > +		oom_panic(flags, get_order(size));
| 
| ...and this look fishy. They're static inlines that get expanded
| everywhere and they're known to be performance sensitive paths. I don't
| see much point in checking for >= MAX_ORDER at all because we will get a
| nice oops anyway for that.
| 
| __GFP_PANIC is an annotation saying that it's okay for a particular
| call-site not to check for NULL because we never expect to run out of
| memory at that point. But we don't really need to panic() for all the
| possible *errors*, just for the out-of-memory case.
| 
| 			Pekka
| 

A new one. Take a look please. I decided to put oom_panic
to oom_kill.c, since it seems to be appropriate.

	-- Cyrill

---
 include/linux/gfp.h |    4 +++-
 include/linux/oom.h |    1 +
 mm/failslab.c       |    3 +++
 mm/oom_kill.c       |   10 ++++++++++
 mm/page_alloc.c     |    7 +++++--
 5 files changed, 22 insertions(+), 3 deletions(-)

Index: linux-2.6.git/include/linux/gfp.h
=====================================================================
--- linux-2.6.git.orig/include/linux/gfp.h
+++ linux-2.6.git/include/linux/gfp.h
@@ -58,7 +58,9 @@ struct vm_area_struct;
 #define __GFP_NOTRACK	((__force gfp_t)0)
 #endif
 
-#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
+#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
+
+#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /* This equals 0, but use constants in case they ever change */
Index: linux-2.6.git/include/linux/oom.h
=====================================================================
--- linux-2.6.git.orig/include/linux/oom.h
+++ linux-2.6.git/include/linux/oom.h
@@ -29,6 +29,7 @@ extern void clear_zonelist_oom(struct zo
 extern void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, int order);
 extern int register_oom_notifier(struct notifier_block *nb);
 extern int unregister_oom_notifier(struct notifier_block *nb);
+extern void oom_panic(gfp_t gfp_mask, unsigned int order);
 
 #endif /* __KERNEL__*/
 #endif /* _INCLUDE_LINUX_OOM_H */
Index: linux-2.6.git/mm/failslab.c
=====================================================================
--- linux-2.6.git.orig/mm/failslab.c
+++ linux-2.6.git/mm/failslab.c
@@ -17,6 +17,9 @@ bool should_failslab(size_t size, gfp_t 
 	if (gfpflags & __GFP_NOFAIL)
 		return false;
 
+	if (gfpflags & __GFP_PANIC)
+		return false;
+
         if (failslab.ignore_gfp_wait && (gfpflags & __GFP_WAIT))
 		return false;
 
Index: linux-2.6.git/mm/oom_kill.c
=====================================================================
--- linux-2.6.git.orig/mm/oom_kill.c
+++ linux-2.6.git/mm/oom_kill.c
@@ -422,6 +422,16 @@ static int oom_kill_process(struct task_
 	return oom_kill_task(p);
 }
 
+void oom_panic(gfp_t gfp_mask, unsigned int order)
+{
+	if (likely(!(gfp_mask & __GFP_PANIC)))
+		return;
+
+	panic("Out of memory: panic due to __GFP_PANIC.\n"
+		"%s order:%d, mode:0x%x\n", current->comm,
+		order, gfp_mask);
+}
+
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 {
Index: linux-2.6.git/mm/page_alloc.c
=====================================================================
--- linux-2.6.git.orig/mm/page_alloc.c
+++ linux-2.6.git/mm/page_alloc.c
@@ -1185,6 +1185,8 @@ static int should_fail_alloc_page(gfp_t 
 		return 0;
 	if (gfp_mask & __GFP_NOFAIL)
 		return 0;
+	if (gfp_mask & __GFP_PANIC)
+		return 0;
 	if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
 		return 0;
 	if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & __GFP_WAIT))
@@ -1506,7 +1508,7 @@ restart:
 		 * Happens if we have an empty zonelist as a result of
 		 * GFP_THISNODE being used on a memoryless node
 		 */
-		return NULL;
+		goto nopage;
 	}
 
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
@@ -1685,7 +1687,8 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
-	return page;
+	oom_panic(gfp_mask, order);
+	return NULL;
 got_pg:
 	if (kmemcheck_enabled)
 		kmemcheck_pagealloc_alloc(page, order, gfp_mask);

Re: introducing __GFP_PANIC

[Pekka Enberg]

On Mon, 2009-05-04 at 12:49 +0400, Cyrill Gorcunov wrote:
> | __GFP_PANIC is an annotation saying that it's okay for a particular
> | call-site not to check for NULL because we never expect to run out of
> | memory at that point. But we don't really need to panic() for all the
> | possible *errors*, just for the out-of-memory case.
> | 
> | 			Pekka
> | 
> 
> Which means, __GFP_PANIC will not grant the guarantee to a caller
> that he will not get NULL deref even with __GFP_PANIC specified?

Yes, __GFP_PANIC will guarantee that you never return NULL for
_out-of-memory_ but makes no guarantees what happens if you pass bogus
size or order.

Re: introducing __GFP_PANIC

[Pekka Enberg]

On Mon, 2009-05-04 at 13:08 +0400, Cyrill Gorcunov wrote:
> Index: linux-2.6.git/mm/oom_kill.c
> =====================================================================
> --- linux-2.6.git.orig/mm/oom_kill.c
> +++ linux-2.6.git/mm/oom_kill.c
> @@ -422,6 +422,16 @@ static int oom_kill_process(struct task_
>  	return oom_kill_task(p);
>  }
>  
> +void oom_panic(gfp_t gfp_mask, unsigned int order)
> +{
> +	if (likely(!(gfp_mask & __GFP_PANIC)))
> +		return;
> +
> +	panic("Out of memory: panic due to __GFP_PANIC.\n"
> +		"%s order:%d, mode:0x%x\n", current->comm,
> +		order, gfp_mask);
> +}

I think this just makes things harder to follow. It has one call-site so
why not inline this there?

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Mon, May 04, 2009 at 12:57:11PM +0300]
| On Mon, 2009-05-04 at 13:08 +0400, Cyrill Gorcunov wrote:
| > Index: linux-2.6.git/mm/oom_kill.c
| > =====================================================================
| > --- linux-2.6.git.orig/mm/oom_kill.c
| > +++ linux-2.6.git/mm/oom_kill.c
| > @@ -422,6 +422,16 @@ static int oom_kill_process(struct task_
| >  	return oom_kill_task(p);
| >  }
| >  
| > +void oom_panic(gfp_t gfp_mask, unsigned int order)
| > +{
| > +	if (likely(!(gfp_mask & __GFP_PANIC)))
| > +		return;
| > +
| > +	panic("Out of memory: panic due to __GFP_PANIC.\n"
| > +		"%s order:%d, mode:0x%x\n", current->comm,
| > +		order, gfp_mask);
| > +}
| 
| I think this just makes things harder to follow. It has one call-site so
| why not inline this there?
| 

Indeed, will fix shortly, thanks.

	-- Cyrill

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Cyrill Gorcunov - Mon, May 04, 2009 at 02:01:06PM +0400]
| [Pekka Enberg - Mon, May 04, 2009 at 12:57:11PM +0300]
| | On Mon, 2009-05-04 at 13:08 +0400, Cyrill Gorcunov wrote:
| | > Index: linux-2.6.git/mm/oom_kill.c
| | > =====================================================================
| | > --- linux-2.6.git.orig/mm/oom_kill.c
| | > +++ linux-2.6.git/mm/oom_kill.c
| | > @@ -422,6 +422,16 @@ static int oom_kill_process(struct task_
| | >  	return oom_kill_task(p);
| | >  }
| | >  
| | > +void oom_panic(gfp_t gfp_mask, unsigned int order)
| | > +{
| | > +	if (likely(!(gfp_mask & __GFP_PANIC)))
| | > +		return;
| | > +
| | > +	panic("Out of memory: panic due to __GFP_PANIC.\n"
| | > +		"%s order:%d, mode:0x%x\n", current->comm,
| | > +		order, gfp_mask);
| | > +}
| | 
| | I think this just makes things harder to follow. It has one call-site so
| | why not inline this there?
| | 
| 
| Indeed, will fix shortly, thanks.
| 
| 	-- Cyrill

I believe this version should be correct (still RFC).
__GFP_NOWARN has printk limit so instead of making
additional checks (while combining those prink in
original code) just adding panic with order and flags
should be cleaner (as it done at moment :)

	-- Cyrill
---
 include/linux/gfp.h |    4 +++-
 mm/failslab.c       |    3 +++
 mm/page_alloc.c     |   11 +++++++++--
 3 files changed, 15 insertions(+), 3 deletions(-)

Index: linux-2.6.git/include/linux/gfp.h
=====================================================================
--- linux-2.6.git.orig/include/linux/gfp.h
+++ linux-2.6.git/include/linux/gfp.h
@@ -58,7 +58,9 @@ struct vm_area_struct;
 #define __GFP_NOTRACK	((__force gfp_t)0)
 #endif
 
-#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
+#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
+
+#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /* This equals 0, but use constants in case they ever change */
Index: linux-2.6.git/mm/failslab.c
=====================================================================
--- linux-2.6.git.orig/mm/failslab.c
+++ linux-2.6.git/mm/failslab.c
@@ -17,6 +17,9 @@ bool should_failslab(size_t size, gfp_t 
 	if (gfpflags & __GFP_NOFAIL)
 		return false;
 
+	if (gfpflags & __GFP_PANIC)
+		return false;
+
         if (failslab.ignore_gfp_wait && (gfpflags & __GFP_WAIT))
 		return false;
 
Index: linux-2.6.git/mm/page_alloc.c
=====================================================================
--- linux-2.6.git.orig/mm/page_alloc.c
+++ linux-2.6.git/mm/page_alloc.c
@@ -1185,6 +1185,8 @@ static int should_fail_alloc_page(gfp_t 
 		return 0;
 	if (gfp_mask & __GFP_NOFAIL)
 		return 0;
+	if (gfp_mask & __GFP_PANIC)
+		return 0;
 	if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
 		return 0;
 	if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & __GFP_WAIT))
@@ -1506,7 +1508,7 @@ restart:
 		 * Happens if we have an empty zonelist as a result of
 		 * GFP_THISNODE being used on a memoryless node
 		 */
-		return NULL;
+		goto nopage;
 	}
 
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
@@ -1685,7 +1687,12 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
-	return page;
+	if (unlikely((gfp_mask & __GFP_PANIC))) {
+		panic("Out of memory: panic due to __GFP_PANIC.\n"
+			"%s order:%d, mode:0x%x\n", p->comm,
+			order, gfp_mask);
+	}
+	return NULL;
 got_pg:
 	if (kmemcheck_enabled)
 		kmemcheck_pagealloc_alloc(page, order, gfp_mask);

Re: introducing __GFP_PANIC

[Pekka Enberg]

On Mon, 2009-05-04 at 14:11 +0400, Cyrill Gorcunov wrote:
> I believe this version should be correct (still RFC).
> __GFP_NOWARN has printk limit so instead of making
> additional checks (while combining those prink in
> original code) just adding panic with order and flags
> should be cleaner (as it done at moment :)

Looks good to me!

Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

I suspect the patch needs some additional work for -mm due to Mel
Gorman's page allocator cleanup and optimization patches.

			Pekka

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Mon, May 04, 2009 at 01:33:08PM +0300]
| On Mon, 2009-05-04 at 14:11 +0400, Cyrill Gorcunov wrote:
| > I believe this version should be correct (still RFC).
| > __GFP_NOWARN has printk limit so instead of making
| > additional checks (while combining those prink in
| > original code) just adding panic with order and flags
| > should be cleaner (as it done at moment :)
| 
| Looks good to me!
| 
| Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
| 
| I suspect the patch needs some additional work for -mm due to Mel
| Gorman's page allocator cleanup and optimization patches.
| 
| 			Pekka
| 

Thanks a lot, Pekka! I'll resend this one with Mel CC'ed so he
could take a look (and a couple of people I know involved in
memory management as well).

	-- Cyrill

Re: introducing __GFP_PANIC

[David Rientjes]

On Mon, 4 May 2009, Cyrill Gorcunov wrote:

> @@ -1685,7 +1687,12 @@ nopage:
>  		dump_stack();
>  		show_mem();
>  	}
> -	return page;
> +	if (unlikely((gfp_mask & __GFP_PANIC))) {

Too many parentheses and this doesn't need the unnecessary braces.

> +		panic("Out of memory: panic due to __GFP_PANIC.\n"
> +			"%s order:%d, mode:0x%x\n", p->comm,
> +			order, gfp_mask);

The extra newline in this statement doesn't seem necessary, it's just one 
less line that will be visible on the frozen screen.  I also think calling 
this 'gfp_mask' instead of 'mode' is more appropriate just like the oom 
killer does.

> +	}
> +	return NULL;
>  got_pg:
>  	if (kmemcheck_enabled)
>  		kmemcheck_pagealloc_alloc(page, order, gfp_mask);
> 

Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[David Rientjes - Mon, May 04, 2009 at 03:56:10AM -0700]
| On Mon, 4 May 2009, Cyrill Gorcunov wrote:
| 
| > @@ -1685,7 +1687,12 @@ nopage:
| >  		dump_stack();
| >  		show_mem();
| >  	}
| > -	return page;
| > +	if (unlikely((gfp_mask & __GFP_PANIC))) {
| 
| Too many parentheses and this doesn't need the unnecessary braces.

Thanks David, fixed (old games /with other flags/ rudiment).

| 
| > +		panic("Out of memory: panic due to __GFP_PANIC.\n"
| > +			"%s order:%d, mode:0x%x\n", p->comm,
| > +			order, gfp_mask);
| 
| The extra newline in this statement doesn't seem necessary, it's just one 
| less line that will be visible on the frozen screen.  I also think calling 
| this 'gfp_mask' instead of 'mode' is more appropriate just like the oom 
| killer does.

ok, convinced. Maybe even make it shorter like
"Panic due to __GFP_PANIC: %s order:%d, gfp_mask:0x%x\n"
Hmm?

| 
| > +	}
| > +	return NULL;
| >  got_pg:
| >  	if (kmemcheck_enabled)
| >  		kmemcheck_pagealloc_alloc(page, order, gfp_mask);
| > 
| 
	-- Cyrill

[tip:x86/apic] x86: uv - prevent NULL dereference in uv_system_init()

[tip-bot for Cyrill Gorcunov]

Commit-ID:  9a8709d44139748fe2e0ab56d20d8c384c8b65ad
Gitweb:     http://git.kernel.org/tip/9a8709d44139748fe2e0ab56d20d8c384c8b65ad
Author:     Cyrill Gorcunov <gorcunov@gmail.com>
AuthorDate: Sat, 2 May 2009 00:25:11 +0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 3 May 2009 10:49:31 +0200

x86: uv - prevent NULL dereference in uv_system_init()

We may reach NULL dereference oops if kmalloc failed.
Prevent it with explicit BUG_ON.

[ Impact: more controlled assert in 'impossible' scenario ]

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Jack Steiner <steiner@sgi.com>
LKML-Reference: <20090501202511.GE4633@lenovo>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/apic/x2apic_uv_x.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 873bf71..9d9e228 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -569,15 +569,18 @@ void __init uv_system_init(void)
 
 	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
 	uv_blade_info = kmalloc(bytes, GFP_KERNEL);
+	BUG_ON(!uv_blade_info);
 
 	get_lowmem_redirect(&lowmem_redir_base, &lowmem_redir_size);
 
 	bytes = sizeof(uv_node_to_blade[0]) * num_possible_nodes();
 	uv_node_to_blade = kmalloc(bytes, GFP_KERNEL);
+	BUG_ON(!uv_node_to_blade);
 	memset(uv_node_to_blade, 255, bytes);
 
 	bytes = sizeof(uv_cpu_to_blade[0]) * num_possible_cpus();
 	uv_cpu_to_blade = kmalloc(bytes, GFP_KERNEL);
+	BUG_ON(!uv_cpu_to_blade);
 	memset(uv_cpu_to_blade, 255, bytes);
 
 	blade = 0;