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

Pekka Enberg
2009-05-03T12:27:18Z

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