Align kvrealloc() with krealloc()

PATCH Superseded Danilo Krummrich 3 participants 10 messages Jul 17, 2024 – Jul 23, 2024

Subsystems (10)

DRM drivers Memory management DRM misc PowerPC +6 more subsystems

lore

Share & tools

lore

Danilo Krummrich

author reviewer

2024-07-17 22:25 UTC #0 lore raw

cover Align kvrealloc() with krealloc()

Hi,

Besides the obvious (and desired) difference between krealloc() and kvrealloc(), there is some inconsistency in their function signatures and behavior:

- krealloc() frees the memory when the requested size is zero, whereas kvrealloc() simply returns a pointer to the existing allocation.

- krealloc() behaves like kmalloc() if a NULL pointer is passed, whereas kvrealloc() does not accept a NULL pointer at all and, if passed, would fault instead.

- krealloc() is self-contained, whereas kvrealloc() relies on the caller to provide the size of the previous allocation.

Inconsistent behavior throughout allocation APIs is error prone, hence make kvrealloc() behave like krealloc(), which seems superior in all mentioned aspects.

In order to be able to get rid of kvrealloc()'s oldsize parameter, introduce vrealloc() and make use of it in kvrealloc().

Making use of vrealloc() in kvrealloc() also provides oppertunities to grow (and shrink) allocations more efficiently. For instance, vrealloc() could be optimized to extend the existing allocation if there is enough contiguous space left in the virtual address space at the end of the existing allocation.

Besides the above, those functions are required by Rust's allocator abstractons
[1] (rework based on this series in [2]). With Vec or KVec respectively, potentially growing (and shrinking) data structures are rather common.

The patches of this series can also be found in [3].

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/mm
[3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=mm/krealloc

Danilo Krummrich (2):
mm: vmalloc: implement vrealloc()
mm: kvmalloc: align kvrealloc() with krealloc()

 arch/powerpc/platforms/pseries/papr-vpd.c |  5 +-
 drivers/gpu/drm/drm_exec.c                |  3 +-
 fs/xfs/xfs_log_recover.c                  |  2 +-
 include/linux/slab.h                      |  3 +-
 include/linux/vmalloc.h                   |  4 ++
 kernel/resource.c                         |  3 +-
 lib/fortify_kunit.c                       |  3 +-
 mm/util.c                                 | 72 ++++++++++++++++-------
 mm/vmalloc.c                              | 58 ++++++++++++++++++
 9 files changed, 118 insertions(+), 35 deletions(-)

base-commit: 51835949dda3783d4639cfa74ce13a3c9829de00

Signature · 1 line

2.45.2

Danilo Krummrich

author reviewer

2024-07-17 22:25 UTC #1 lore raw

PATCH 1/2 mm: vmalloc: implement vrealloc()

Implement vrealloc() analogous to krealloc().

Currently, krealloc() requires the caller to pass the size of the previous memory allocation, which, instead, should be self-contained.

We attempt to fix this in a subsequent patch which, in order to do so, requires vrealloc().

Besides that, we need realloc() functions for kernel allocators in Rust too. With Vec or KVec respectively, potentially growing (and shrinking) data structures are rather common.

Signed-off-by Danilo Krummrich <[email protected]>

---
 include/linux/vmalloc.h |  4 +++
 mm/vmalloc.c            | 58 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

include/linux/vmalloc.h +4 -0

index e4a631ec430b..9ff0a8e5c323 100644
@@ -189,6 +189,10 @@ extern void *__vcalloc_noprof(size_t n, size_t size, gfp_t flags) __alloc_size(1
 extern void *vcalloc_noprof(size_t n, size_t size) __alloc_size(1, 2);
 #define vcalloc(...)		alloc_hooks(vcalloc_noprof(__VA_ARGS__))
 
+extern void * __must_check vrealloc_noprof(const void *p, size_t size,
+					   gfp_t flags) __realloc_size(2);
+#define vrealloc(...)		alloc_hooks(vrealloc_noprof(__VA_ARGS__))
+
 extern void vfree(const void *addr);
 extern void vfree_atomic(const void *addr);

mm/vmalloc.c +58 -0

64 lines not on this page · raw message

Signature · 1 line

2.45.2

Christoph Hellwig

2024-07-18 03:16 UTC #2 lore raw

Re: [PATCH 1/2] mm: vmalloc: implement vrealloc()

include/linux/vmalloc.h extern void *__vcalloc_noprof(size_t n, size_t size, gfp_t flags) __alloc_size(1 · line 193

Quoted 2 lines · Danilo Krummrich

include/linux/vmalloc.h +2 -0

+extern void * __must_check vrealloc_noprof(const void *p, size_t size,
+					   gfp_t flags) __realloc_size(2);

No need for the extern here.

It would also help readability a bit to keep the __realloc_size on a separate like, aka:

void *__must_check vrealloc_noprof(const void *p, size_t size, gfp_t flags)

		__realloc_size(2);

mm/vmalloc.c void *vzalloc_node_noprof(unsigned long size, int node) · line 4079

Quoted 4 lines

mm/vmalloc.c +4 -0

+	if (size <= old_size) {
+		/* TODO: Can we optimize and shrink the allocation? What would
+		 * be a good metric for when to shrink the vm_area?
+		 */

Kernel comment style is to keep the

/*

on a line of it's own. A realloc that doesn't shrink is a bit pointless. The same heuristics as for krealloc should probably apply here, adjust to the fact that we always deal with whole pages.

mm/vmalloc.c void *vzalloc_node_noprof(unsigned long size, int node) · line 4085

Quoted 3 lines

mm/vmalloc.c +3 -0

+	/* TODO: Can we optimize and extend the existing allocation if we have
+	 * enough contiguous space left in the virtual address space?
+	 */

I don't understand this comment.

Quoted 1 line

+EXPORT_SYMBOL(vrealloc_noprof);

New interfaces should be EXPORT_SYMBOL_GPL. That being said for this to be added an exported we'll need an actual user to start with anyway.

Danilo Krummrich

author reviewer

2024-07-18 11:43 UTC #3 lore raw

Re: [PATCH 1/2] mm: vmalloc: implement vrealloc()

Quoted 11 lines · Christoph Hellwig

include/linux/vmalloc.h extern void *__vcalloc_noprof(size_t n, size_t size, gfp_t flags) __alloc_size(1 · line 193
Quoted 2 lines · Danilo Krummrich
include/linux/vmalloc.h +2 -0
+extern void * __must_check vrealloc_noprof(const void *p, size_t size,
+					   gfp_t flags) __realloc_size(2);
No need for the extern here.
It would also help readability a bit to keep the __realloc_size on a separate like, aka:
void *__must_check vrealloc_noprof(const void *p, size_t size, gfp_t flags)
		__realloc_size(2);

Sure, will change that.

Quoted 11 lines

mm/vmalloc.c void *vzalloc_node_noprof(unsigned long size, int node) · line 4079
Quoted 5 lines
mm/vmalloc.c +4 -0
+	if (size <= old_size) {
+		/* TODO: Can we optimize and shrink the allocation? What would
+		 * be a good metric for when to shrink the vm_area?
+		 */
Kernel comment style is to keep the
		/*
on a line of it's own.

I think it differs a bit throughout subsystems - will change.

Quoted 1 line

A realloc that doesn't shrink is a bit pointless.

Indeed - the idea was to mostly clean up kvrealloc() in this series first and implement proper shrinking and growing in a subsequent one. Does that make sense?

Quoted 2 lines

The same heuristics as for krealloc should probably apply here, adjust to the fact that we always deal with whole pages.

Sounds reasonable, but are there any? In __do_krealloc() if ks > new_size we only call into kasan_krealloc() to poison things and return the original pointer. Do I miss anything?

Quoted 7 lines

mm/vmalloc.c void *vzalloc_node_noprof(unsigned long size, int node) · line 4085
Quoted 5 lines
mm/vmalloc.c +3 -0
+	/* TODO: Can we optimize and extend the existing allocation if we have
+	 * enough contiguous space left in the virtual address space?
+	 */
I don't understand this comment.

I meant to say that we could optimze by allocating additional pages and map them at the end of the existing allocation, if there is enough space in the VA space.

If that doesn't work, we can still allocate additional pages and remap everything.

Quoted 5 lines

Quoted 2 lines
+EXPORT_SYMBOL(vrealloc_noprof);
New interfaces should be EXPORT_SYMBOL_GPL. That being said for this to be added an exported we'll need an actual user to start with anyway.

Besides kvrealloc(), the Rust abstractions will be a user soon, but for that we probably don't need the export either. I will remove it for now.

Uladzislau Rezki

maintainer

2024-07-23 11:28 UTC #4 lore raw

Re: [PATCH 1/2] mm: vmalloc: implement vrealloc()

In reply to Danilo Krummrich

Quoted 61 lines · Danilo Krummrich

Implement vrealloc() analogous to krealloc().

Currently, krealloc() requires the caller to pass the size of the previous memory allocation, which, instead, should be self-contained.

We attempt to fix this in a subsequent patch which, in order to do so, requires vrealloc().

Besides that, we need realloc() functions for kernel allocators in Rust too. With Vec or KVec respectively, potentially growing (and shrinking) data structures are rather common.

Signed-off-by Danilo Krummrich <[email protected]>

---
 include/linux/vmalloc.h |  4 +++
 mm/vmalloc.c            | 58 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

include/linux/vmalloc.h +4 -0

index e4a631ec430b..9ff0a8e5c323 100644
@@ -189,6 +189,10 @@ extern void *__vcalloc_noprof(size_t n, size_t size, gfp_t flags) __alloc_size(1
 extern void *vcalloc_noprof(size_t n, size_t size) __alloc_size(1, 2);
 #define vcalloc(...)		alloc_hooks(vcalloc_noprof(__VA_ARGS__))
 
+extern void * __must_check vrealloc_noprof(const void *p, size_t size,
+					   gfp_t flags) __realloc_size(2);
+#define vrealloc(...)		alloc_hooks(vrealloc_noprof(__VA_ARGS__))
+
 extern void vfree(const void *addr);
 extern void vfree_atomic(const void *addr);

mm/vmalloc.c +20 -0

index e34ea860153f..4ec949ac9d9d 100644
@@ -4036,6 +4036,64 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 
+/**
+ * vrealloc - reallocate virtually contiguous memory; contents remain unchanged
+ * @p: object to reallocate memory for
+ * @size: the size to reallocate
+ * @flags: the flags for the page level allocator
+ *
+ * The contents of the object pointed to are preserved up to the lesser of the
+ * new and old size (__GFP_ZERO flag is effectively ignored).
+ *
+ * If @p is %NULL, vrealloc() behaves exactly like vmalloc(). If @size is 0 and
+ * @p is not a %NULL pointer, the object pointed to is freed.
+ *
+ * Return: pointer to the allocated memory; %NULL if @size is zero or in case of
+ *         failure
+ */
+void *vrealloc_noprof(const void *p, size_t size, gfp_t flags)
+{
+	size_t old_size = 0;
+	void *n;
+

mm/vmalloc.c void *vzalloc_node_noprof(unsigned long size, int node) · line 4068

Quoted 10 lines

mm/vmalloc.c +9 -0

+	if (!size) {
+		vfree(p);
+		return NULL;
+	}
+
+	if (p) {
+		struct vm_struct *vm;
+
+		vm = find_vm_area(p);

Concurrent vfree() will lead to use-after-free. Either add a comment that a user is responsible for not using vrealloc()/vfree() on the same pointer concurrently or use find_unlink_vmap_area() which might be more complex when it comes to design of the vrealloc().

--
Uladzislau Rezki

Christoph Hellwig

2024-07-23 13:45 UTC #5 lore raw

Re: [PATCH 1/2] mm: vmalloc: implement vrealloc()

Quoted 4 lines · Uladzislau Rezki

Concurrent vfree() will lead to use-after-free. Either add a comment that a user is responsible for not using vrealloc()/vfree() on the same pointer concurrently or use find_unlink_vmap_area() which might be more complex when it comes to design of the vrealloc().

You can never use *free concurrently with *realloc. I guess it doesn't hurt to clearly document that, but other than that we should not try to cater to that use pattern at all.

Uladzislau Rezki

maintainer

2024-07-23 15:54 UTC #6 lore raw

Re: [PATCH 1/2] mm: vmalloc: implement vrealloc()

Quoted 10 lines · Christoph Hellwig

Quoted 4 lines · Uladzislau Rezki
Concurrent vfree() will lead to use-after-free. Either add a comment that a user is responsible for not using vrealloc()/vfree() on the same pointer concurrently or use find_unlink_vmap_area() which might be more complex when it comes to design of the vrealloc().
You can never use *free concurrently with *realloc. I guess it doesn't hurt to clearly document that, but other than that we should not try to cater to that use pattern at all.

Agree, i mentioned that as a first option. I think, it is enough to document that.

Thanks!

--
Uladzislau Rezki

Danilo Krummrich

author reviewer

2024-07-17 22:25 UTC #7 lore raw

PATCH 2/2 mm: kvmalloc: align kvrealloc() with krealloc()

In reply to Danilo Krummrich

Besides the obvious (and desired) difference between krealloc() and kvrealloc(), there is some inconsistency in their function signatures and behavior:

- krealloc() frees the memory when the requested size is zero, whereas kvrealloc() simply returns a pointer to the existing allocation.

- krealloc() behaves like kmalloc() if a NULL pointer is passed, whereas kvrealloc() does not accept a NULL pointer at all and, if passed, would fault instead.

- krealloc() is self-contained, whereas kvrealloc() relies on the caller to provide the size of the previous allocation.

Inconsistent behavior throughout allocation APIs is error prone, hence make kvrealloc() behave like krealloc(), which seems superior in all mentioned aspects.

Besides that, implementing kvrealloc() by making use of krealloc() and vrealloc() provides oppertunities to grow (and shrink) allocations more efficiently. For instance, vrealloc() could be optimized to extend the existing allocation if there is enough contiguous space left in the virtual address space at the end of the existing allocation.

Signed-off-by Danilo Krummrich <[email protected]>

---
 arch/powerpc/platforms/pseries/papr-vpd.c |  5 +-
 drivers/gpu/drm/drm_exec.c                |  3 +-
 fs/xfs/xfs_log_recover.c                  |  2 +-
 include/linux/slab.h                      |  3 +-
 kernel/resource.c                         |  3 +-
 lib/fortify_kunit.c                       |  3 +-
 mm/util.c                                 | 72 ++++++++++++++++-------
 7 files changed, 56 insertions(+), 35 deletions(-)

arch/powerpc/platforms/pseries/papr-vpd.c +1 -4

index c29e85db5f35..1574176e3ffc 100644
@@ -156,10 +156,7 @@ static int vpd_blob_extend(struct vpd_blob *blob, const char *data, size_t len)
 	const char *old_ptr = blob->data;
 	char *new_ptr;
 
-	new_ptr = old_ptr ?
-		kvrealloc(old_ptr, old_len, new_len, GFP_KERNEL_ACCOUNT) :
-		kvmalloc(len, GFP_KERNEL_ACCOUNT);
-
+	new_ptr = kvrealloc(old_ptr, new_len, GFP_KERNEL_ACCOUNT);
 	if (!new_ptr)
 		return -ENOMEM;

drivers/gpu/drm/drm_exec.c +1 -2

index 2da094bdf8a4..18e366cc4993 100644
@@ -145,8 +145,7 @@ static int drm_exec_obj_locked(struct drm_exec *exec,
 		size_t size = exec->max_objects * sizeof(void *);
 		void *tmp;
 
-		tmp = kvrealloc(exec->objects, size, size + PAGE_SIZE,
-				GFP_KERNEL);
+		tmp = kvrealloc(exec->objects, size + PAGE_SIZE, GFP_KERNEL);
 		if (!tmp)
 			return -ENOMEM;

fs/xfs/xfs_log_recover.c +1 -1

index 4fe627991e86..bbe5ecb2345b 100644
@@ -2132,7 +2132,7 @@ xlog_recover_add_to_cont_trans(
 	old_ptr = item->ri_buf[item->ri_cnt-1].i_addr;
 	old_len = item->ri_buf[item->ri_cnt-1].i_len;
 
-	ptr = kvrealloc(old_ptr, old_len, len + old_len, GFP_KERNEL);
+	ptr = kvrealloc(old_ptr, len + old_len, GFP_KERNEL);
 	if (!ptr)
 		return -ENOMEM;
 	memcpy(&ptr[old_len], dp, len);

include/linux/slab.h +1 -2

index 7247e217e21b..41ede46d3bd2 100644
@@ -808,8 +808,7 @@ kvmalloc_array_node_noprof(size_t n, size_t size, gfp_t flags, int node)
 #define kvcalloc_node(...)			alloc_hooks(kvcalloc_node_noprof(__VA_ARGS__))
 #define kvcalloc(...)				alloc_hooks(kvcalloc_noprof(__VA_ARGS__))
 
-extern void *kvrealloc_noprof(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
-		      __realloc_size(3);
+extern void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags) __realloc_size(2);
 #define kvrealloc(...)				alloc_hooks(kvrealloc_noprof(__VA_ARGS__))
 
 extern void kvfree(const void *addr);

kernel/resource.c +1 -2

index fcbca39dbc45..46e064c8dce2 100644
@@ -458,8 +458,7 @@ int walk_system_ram_res_rev(u64 start, u64 end, void *arg,
 			/* re-alloc */
 			struct resource *rams_new;
 
-			rams_new = kvrealloc(rams, rams_size * sizeof(struct resource),
-					     (rams_size + 16) * sizeof(struct resource),
+			rams_new = kvrealloc(rams, (rams_size + 16) * sizeof(struct resource),
 					     GFP_KERNEL);
 			if (!rams_new)
 				goto out;

lib/fortify_kunit.c +1 -2

index 27ea8bf0252c..acbdc7855100 100644
@@ -308,8 +308,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(vmalloc)
 	orig = kvmalloc(prev_size, gfp);				\
 	KUNIT_EXPECT_TRUE(test, orig != NULL);				\
 	checker(((expected_pages) * PAGE_SIZE) * 2,			\
-		kvrealloc(orig, prev_size,				\
-			  ((alloc_pages) * PAGE_SIZE) * 2, gfp),	\
+		kvrealloc(orig, ((alloc_pages) * PAGE_SIZE) * 2, gfp),	\
 		kvfree(p));						\
 } while (0)
 DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)

mm/util.c +50 ~~-22~~

98 lines not on this page · raw message

Signature · 1 line

2.45.2

Christoph Hellwig

2024-07-18 03:19 UTC #8 lore raw

Re: [PATCH 2/2] mm: kvmalloc: align kvrealloc() with krealloc()

Quoted 1 line

+extern void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags) __realloc_size(2);

Please drop the extern while you're at it and move the __realloc_size attribute to a separate line.

mm/util.c unsigned long vm_mmap(struct file *file, unsigned long addr, · line 613

Quoted 13 lines

mm/util.c +13 -0

+static gfp_t to_kmalloc_flags(gfp_t flags, size_t size)
+{
+	if (size > PAGE_SIZE) {
+		flags |= __GFP_NOWARN;
+
+		if (!(flags & __GFP_RETRY_MAYFAIL))
+			flags |= __GFP_NORETRY;
+
+		/* nofail semantic is implemented by the vmalloc fallback */
+		flags &= ~__GFP_NOFAIL;
+	}
+
+	return flags;

The name for this function sounds a bit odd. Maybe kmalloc_gfp_adjust instead? Also the comment explaining these flags tweaks should move from the caller to this function.

Danilo Krummrich

author reviewer

2024-07-18 11:45 UTC #9 lore raw

Re: [PATCH 2/2] mm: kvmalloc: align kvrealloc() with krealloc()

Quoted 4 lines · Christoph Hellwig

Quoted 1 line
+extern void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags) __realloc_size(2);
Please drop the extern while you're at it and move the __realloc_size attribute to a separate line.

Will do.

Quoted 10 lines

mm/util.c +9 -0

+static gfp_t to_kmalloc_flags(gfp_t flags, size_t size)
+{
+	if (size > PAGE_SIZE) {
+		flags |= __GFP_NOWARN;
+
+		if (!(flags & __GFP_RETRY_MAYFAIL))
+			flags |= __GFP_NORETRY;
+
+		/* nofail semantic is implemented by the vmalloc fallback */

Quoted 8 lines

mm/util.c unsigned long vm_mmap(struct file *file, unsigned long addr, · line 613
Quoted 4 lines
mm/util.c +4 -0
+		flags &= ~__GFP_NOFAIL;
+	}
+
+	return flags;
The name for this function sounds a bit odd. Maybe kmalloc_gfp_adjust instead? Also the comment explaining these flags tweaks should move from the caller to this function.

kmalloc_gfp_adjust() sounds good to me. I will rename it and move the comment up.