* [U-Boot] [PATCH] linux_compat: fix potential NULL pointer access
2019-10-02 12:37 ` [U-Boot] [PATCH] linux_compat: fix potential NULL pointer access Marek Szyprowski
@ 2019-10-03 14:23 ` Ralph Siemsen
2019-11-01 13:29 ` Tom Rini
1 sibling, 0 replies; 3+ messages in thread
From: Ralph Siemsen @ 2019-10-03 14:23 UTC (permalink / raw)
To: u-boot
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@samsung.com>
Reviewed-by: Ralph Siemsen <ralph.siemsen@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
^ permalink raw reply [flat|nested] 3+ messages in thread