Messages in this thread | | | Date | Sun, 3 May 2009 19:54:38 +0300 | Subject | Re: [PATCH -tip] x86: uv - prevent NULL dereference in uv_system_init | From | 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. -- 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/
| |