[U-Boot] [PATCH] linux_compat: fix potential NULL pointer access

[Ralph Siemsen]

On Wed, Oct 02, 2019 at 02:37:20PM +0200, Marek Szyprowski wrote:
>malloc_cache_aligned() might return zero, so fix potential NULL pointer
>access if __GFP_ZERO flag is set.
>
>Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>

Reviewed-by: Ralph Siemsen <ralph.siemsen at linaro.org>

This looks reasonable to me. The memset() will happily scribble all over 
memory at address zero, which can be very hard to track down later on.
So adding a check seems prudent. Note that I am not a maintainer ;-)

>---
> lib/linux_compat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/lib/linux_compat.c b/lib/linux_compat.c
>index 6373b4451e..81ea8fb126 100644
>--- a/lib/linux_compat.c
>+++ b/lib/linux_compat.c
>@@ -20,7 +20,7 @@ void *kmalloc(size_t size, int flags)
> 	void *p;
>
> 	p = malloc_cache_aligned(size);
>-	if (flags & __GFP_ZERO)
>+	if (p && flags & __GFP_ZERO)
> 		memset(p, 0, size);
>
> 	return p;
>-- 
>2.17.1

Perhaps I can hijack this thread slightly: have others encountered 
problems due to the use of malloc_cache_aligned() in kmalloc() and 
kmem_cache_alloc()? This substantially increases memory usage, in 
particular for UBI which allocates two small structures for every 
physical erase block. This adds up quickly on large flash chips.

I understand the rationale for padding the allocation, as explained in 
commit e3332e1a1a04534225801c2710c6faef4809641c. However, it also seems 
that in Linux, allocations are aligned/padded only when certain flags 
are set (GFP_DMA or GFP_DMA32 in particular). These flags are not 
currently defined in u-boot.

For UBI at least, there is no need to align or pad the structures, they 
are not accessed by DMA. So it would be good if we could avoid memory 
overhead. Possible options would include:

- go back to plain memalign() or even just plain malloc(),
  with the caveat that callers who need specific alignment must
  handle it themselves (simplest option, no overhead)
- add the missing flags, and check them at runtime.
  (adding code and some overhead)
- hybrid versions, such as checking flags on kmalloc, but not doing
  any padding/alignment for kmem_cache_alloc

I would be happy to work on patches for this, in a separate thread of 
course. Would be helpful to know which option would be acceptable.

Cheers,
Ralph