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

[Cyrill Gorcunov <gorcunov@gmail.com>]

[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