[PATCH v1] mm:free unused pages in kmalloc_order

[PATCH v1] mm:free unused pages in kmalloc_order

[Long Li]

Environment using the slub allocator, 1G memory in my ARM32.
kmalloc(1024, GFP_HIGHUSER) can allocate memory normally,
kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because
alloc_pages returns highmem physical pages, but it cannot be directly
converted into a virtual address and return NULL, the pages has not
been released. Usually driver developers will not use the
GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this
memory leak is not perfect, it is best to be fixed. This is the
first time I have posted a patch, there may be something wrong.

Signed-off-by: Long Li <lonuxli.64@gmail.com>
---
 mm/slab_common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index a143a8c8f874..d2c53b980ab3 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -819,8 +819,12 @@ void *kmalloc_order(size_t size, gfp_t flags, unsigned int order)
 	page = alloc_pages(flags, order);
 	if (likely(page)) {
 		ret = page_address(page);
-		mod_node_page_state(page_pgdat(page), NR_SLAB_UNRECLAIMABLE_B,
-				    PAGE_SIZE << order);
+		if (ret)
+			mod_node_page_state(page_pgdat(page),
+					NR_SLAB_UNRECLAIMABLE_B,
+					PAGE_SIZE << order);
+		else
+			__free_pages(page, order);
 	}
 	ret = kasan_kmalloc_large(ret, size, flags);
 	/* As ret might get tagged, call kmemleak hook after KASAN. */
-- 
2.17.1

Re: [PATCH v1] mm:free unused pages in kmalloc_order

[Matthew Wilcox]

On Sat, Jun 27, 2020 at 04:55:07AM +0000, Long Li wrote:
> Environment using the slub allocator, 1G memory in my ARM32.
> kmalloc(1024, GFP_HIGHUSER) can allocate memory normally,
> kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because
> alloc_pages returns highmem physical pages, but it cannot be directly
> converted into a virtual address and return NULL, the pages has not
> been released. Usually driver developers will not use the
> GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this
> memory leak is not perfect, it is best to be fixed. This is the
> first time I have posted a patch, there may be something wrong.

Slab used to disallow GFP_HIGHMEM allocations earlier than this,
so you'd never get here.  See one moron's patch here, 20 years ago ...
https://lore.kernel.org/lkml/20001228145801.C19693@parcelfarce.linux.theplanet.co.uk/

Things changed a bit since then.  SLAB_LEVEL_MASK became GFP_SLAB_BUG_MASK,
then GFP_SLAB_BUG_MASK moved to mm/internal.h.  Nowadays, GFP_SLAB_BUG_MASK
is checked only in new_slab(), and it is definitely skipped when we go through
the kmalloc_large() patch.

Could you send a replacement patch which checks GFP_SLAB_BUG_MASK before
calling alloc_pages()?

> +++ b/mm/slab_common.c
> @@ -819,8 +819,12 @@ void *kmalloc_order(size_t size, gfp_t flags, unsigned int order)
>  	page = alloc_pages(flags, order);
>  	if (likely(page)) {
>  		ret = page_address(page);
> -		mod_node_page_state(page_pgdat(page), NR_SLAB_UNRECLAIMABLE_B,
> -				    PAGE_SIZE << order);
> +		if (ret)
> +			mod_node_page_state(page_pgdat(page),
> +					NR_SLAB_UNRECLAIMABLE_B,
> +					PAGE_SIZE << order);
> +		else
> +			__free_pages(page, order);
>  	}
>  	ret = kasan_kmalloc_large(ret, size, flags);
>  	/* As ret might get tagged, call kmemleak hook after KASAN. */
> -- 
> 2.17.1
> 
> 

Re: [PATCH v1] mm:free unused pages in kmalloc_order

[Christopher Lameter]

On Sat, 27 Jun 2020, Long Li wrote:

> Environment using the slub allocator, 1G memory in my ARM32.
> kmalloc(1024, GFP_HIGHUSER) can allocate memory normally,
> kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because
> alloc_pages returns highmem physical pages, but it cannot be directly
> converted into a virtual address and return NULL, the pages has not
> been released. Usually driver developers will not use the
> GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this
> memory leak is not perfect, it is best to be fixed. This is the
> first time I have posted a patch, there may be something wrong.

Highmem is not supported by the slab allocators. Please ensure that there
is a warning generated if someone attempts to do such an allocation. We
used to check for that.

In order to make such allocations possible one would have to create yet
another kmalloc array for high memory.

Re: [PATCH v1] mm:free unused pages in kmalloc_order

[Christopher Lameter]

On Mon, 29 Jun 2020, Matthew Wilcox wrote:

> Slab used to disallow GFP_HIGHMEM allocations earlier than this,

It is still not allowed and not supported.

Re: [PATCH v1] mm:free unused pages in kmalloc_order

[Matthew Wilcox]

On Mon, Jun 29, 2020 at 02:48:06PM +0000, Christopher Lameter wrote:
> On Sat, 27 Jun 2020, Long Li wrote:
> > Environment using the slub allocator, 1G memory in my ARM32.
> > kmalloc(1024, GFP_HIGHUSER) can allocate memory normally,
> > kmalloc(64*1024, GFP_HIGHUSER) will cause a memory leak, because
> > alloc_pages returns highmem physical pages, but it cannot be directly
> > converted into a virtual address and return NULL, the pages has not
> > been released. Usually driver developers will not use the
> > GFP_HIGHUSER flag to allocate memory in kmalloc, but I think this
> > memory leak is not perfect, it is best to be fixed. This is the
> > first time I have posted a patch, there may be something wrong.
> 
> Highmem is not supported by the slab allocators. Please ensure that there
> is a warning generated if someone attempts to do such an allocation. We
> used to check for that.

Sounds like we need a test somewhere that checks this behaviour.

> In order to make such allocations possible one would have to create yet
> another kmalloc array for high memory.

Not for this case because it goes straight to kmalloc_order().  What does
make this particular case impossible is that we can't kmap() a compound
page.  We could vmap it, but why are we bothering?

Re: [PATCH v1] mm:free unused pages in kmalloc_order

[Christopher Lameter]

On Mon, 29 Jun 2020, Matthew Wilcox wrote:

> Sounds like we need a test somewhere that checks this behaviour.
>
> > In order to make such allocations possible one would have to create yet
> > another kmalloc array for high memory.
>
> Not for this case because it goes straight to kmalloc_order().  What does
> make this particular case impossible is that we can't kmap() a compound
> page.  We could vmap it, but why are we bothering?

Well yes it will work if the slab allocator falls back to the page
allocator.  Higher order allocation through kmalloc ;-). How much fun
and uselessness ....

Why not call the page allocator directly and play with all the bits you
want? Any regular small object allocation with GFP_HIGH will lead to
strange effects if the bit is not checked.