Align kvrealloc() with krealloc()

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]>

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }

mm/vmalloc.c +51 -0

+ *
+ * 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;
+
+	if (!size) {
+		vfree(p);
+		return NULL;
+	}
+
+	if (p) {
+		struct vm_struct *vm;
+
+		vm = find_vm_area(p);
+		if (unlikely(!vm)) {
+			WARN(1, "Trying to vrealloc() nonexistent vm area (%p)\n", p);
+			return NULL;
+		}
+
+		old_size = get_vm_area_size(vm);
+	}
+
+	if (size <= old_size) {
+		/*
+		 * TODO: Shrink the vm_area, i.e. unmap and free unused pages.
+		 * What would be a good heuristic for when to shrink the
+		 * vm_area?
+		 */
+		return (void *)p;
+	}
+
+	/* TODO: Grow the vm_area, i.e. allocate and map additional pages. */
+	n = __vmalloc_noprof(size, flags);
+	if (!n)
+		return NULL;
+
+	if (p) {
+		memcpy(n, p, old_size);
+		vfree(p);
+	}
+
+	return n;
+}
+
 #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
 #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)

Quoted 5 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.

Quoted 8 lines

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]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 6 lines

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 

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

Quoted 8 lines

mm/vmalloc.c +8 -0

+/**
+ * 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to 4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not new, kvrealloc() did the same before patch 2/2.

But it's also fundamentally not true for krealloc(), or kvrealloc() switching from a kmalloc to valloc. ksize() returns the size of the kmalloc bucket, we don't know what was the exact prior allocation size.

Worse, we
started poisoning the padding in debug configurations, so even a kmalloc(__GFP_ZERO) followed by krealloc(__GFP_ZERO) can give you unexpected poison now...

I guess we should just document __GFP_ZERO is not honored at all for realloc, and maybe start even warning :/ Hopefully nobody relies on that.

Quoted 55 lines

mm/vmalloc.c +51 -0

+ *
+ * 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;
+
+	if (!size) {
+		vfree(p);
+		return NULL;
+	}
+
+	if (p) {
+		struct vm_struct *vm;
+
+		vm = find_vm_area(p);
+		if (unlikely(!vm)) {
+			WARN(1, "Trying to vrealloc() nonexistent vm area (%p)\n", p);
+			return NULL;
+		}
+
+		old_size = get_vm_area_size(vm);
+	}
+
+	if (size <= old_size) {
+		/*
+		 * TODO: Shrink the vm_area, i.e. unmap and free unused pages.
+		 * What would be a good heuristic for when to shrink the
+		 * vm_area?
+		 */
+		return (void *)p;
+	}
+
+	/* TODO: Grow the vm_area, i.e. allocate and map additional pages. */
+	n = __vmalloc_noprof(size, flags);
+	if (!n)
+		return NULL;
+
+	if (p) {
+		memcpy(n, p, old_size);
+		vfree(p);
+	}
+
+	return n;
+}
+
 #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
 #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)

Quoted 23 lines · Vlastimil Babka

Quoted 5 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.

Quoted 8 lines

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]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 6 lines

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 

Quoted 12 lines

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

Quoted 8 lines

mm/vmalloc.c +8 -0

+/**
+ * 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to 4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not new, kvrealloc() did the same before patch 2/2.

Taking it (too) literal, it's not wrong. The contents of the object pointed to are indeed preserved up to the lesser of the new and old size. It's just that the rest may be "preserved" as well.

I work on implementing shrink and grow for vrealloc(). In the meantime I think we could probably just memset() spare memory to zero.

nommu would still uses krealloc() though...

Quoted 4 lines

But it's also fundamentally not true for krealloc(), or kvrealloc() switching from a kmalloc to valloc. ksize() returns the size of the kmalloc bucket, we don't know what was the exact prior allocation size.

Probably a stupid question, but can't we just zero the full bucket initially and make sure to memset() spare memory in the bucket to zero when krealloc() is called with new_size < ksize()?

Quoted 4 lines

Worse, we
started poisoning the padding in debug configurations, so even a kmalloc(__GFP_ZERO) followed by krealloc(__GFP_ZERO) can give you unexpected poison now...

As in writing magics directly to the spare memory in the bucket? Which would then also be copied over to a new buffer in __do_krealloc()?

Quoted 3 lines

I guess we should just document __GFP_ZERO is not honored at all for realloc, and maybe start even warning :/ Hopefully nobody relies on that.

I think it'd be great to make __GFP_ZERO work in all cases. However, if that's really not possible, I'd prefer if we could at least gurantee that *realloc(NULL, size, flags | __GFP_ZERO) is a valid call, i.e. WARN_ON(p && flags & __GFP_ZERO).

Quoted 56 lines

Quoted 55 lines

mm/vmalloc.c +51 -0

+ *
+ * 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;
+
+	if (!size) {
+		vfree(p);
+		return NULL;
+	}
+
+	if (p) {
+		struct vm_struct *vm;
+
+		vm = find_vm_area(p);
+		if (unlikely(!vm)) {
+			WARN(1, "Trying to vrealloc() nonexistent vm area (%p)\n", p);
+			return NULL;
+		}
+
+		old_size = get_vm_area_size(vm);
+	}
+
+	if (size <= old_size) {
+		/*
+		 * TODO: Shrink the vm_area, i.e. unmap and free unused pages.
+		 * What would be a good heuristic for when to shrink the
+		 * vm_area?
+		 */
+		return (void *)p;
+	}
+
+	/* TODO: Grow the vm_area, i.e. allocate and map additional pages. */
+	n = __vmalloc_noprof(size, flags);
+	if (!n)
+		return NULL;
+
+	if (p) {
+		memcpy(n, p, old_size);
+		vfree(p);
+	}
+
+	return n;
+}
+
 #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
 #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)

Quoted 37 lines · Danilo Krummrich

Quoted 23 lines · Vlastimil Babka

Quoted 5 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.

Quoted 8 lines

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]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 6 lines

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 

Quoted 12 lines

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

Quoted 8 lines

mm/vmalloc.c +8 -0

+/**
+ * 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to 4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not new, kvrealloc() did the same before patch 2/2.

Quoted 6 lines

Taking it (too) literal, it's not wrong. The contents of the object pointed to are indeed preserved up to the lesser of the new and old size. It's just that the rest may be "preserved" as well.

I work on implementing shrink and grow for vrealloc(). In the meantime I think we could probably just memset() spare memory to zero.

Probably, this was a bad idea. Even with shrinking implemented we'd need to memset() potential spare memory of the last page to zero, when new_size < old_size.

Analogously, the same would be true for krealloc() buckets. That's probably not worth it.

I think we should indeed just document that __GFP_ZERO doesn't work for re-allocating memory and start to warn about it. As already mentioned, I think we should at least gurantee that *realloc(NULL, size, flags | __GFP_ZERO) is valid, i.e. WARN_ON(p && flags & __GFP_ZERO).

Quoted 86 lines

nommu would still uses krealloc() though...

Quoted 4 lines

But it's also fundamentally not true for krealloc(), or kvrealloc() switching from a kmalloc to valloc. ksize() returns the size of the kmalloc bucket, we don't know what was the exact prior allocation size.

Probably a stupid question, but can't we just zero the full bucket initially and make sure to memset() spare memory in the bucket to zero when krealloc() is called with new_size < ksize()?

Quoted 4 lines

Worse, we
started poisoning the padding in debug configurations, so even a kmalloc(__GFP_ZERO) followed by krealloc(__GFP_ZERO) can give you unexpected poison now...

As in writing magics directly to the spare memory in the bucket? Which would then also be copied over to a new buffer in __do_krealloc()?

Quoted 3 lines

I guess we should just document __GFP_ZERO is not honored at all for realloc, and maybe start even warning :/ Hopefully nobody relies on that.

I think it'd be great to make __GFP_ZERO work in all cases. However, if that's really not possible, I'd prefer if we could at least gurantee that *realloc(NULL, size, flags | __GFP_ZERO) is a valid call, i.e. WARN_ON(p && flags & __GFP_ZERO).

Quoted 56 lines

Quoted 55 lines

mm/vmalloc.c +51 -0

+ *
+ * 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;
+
+	if (!size) {
+		vfree(p);
+		return NULL;
+	}
+
+	if (p) {
+		struct vm_struct *vm;
+
+		vm = find_vm_area(p);
+		if (unlikely(!vm)) {
+			WARN(1, "Trying to vrealloc() nonexistent vm area (%p)\n", p);
+			return NULL;
+		}
+
+		old_size = get_vm_area_size(vm);
+	}
+
+	if (size <= old_size) {
+		/*
+		 * TODO: Shrink the vm_area, i.e. unmap and free unused pages.
+		 * What would be a good heuristic for when to shrink the
+		 * vm_area?
+		 */
+		return (void *)p;
+	}
+
+	/* TODO: Grow the vm_area, i.e. allocate and map additional pages. */
+	n = __vmalloc_noprof(size, flags);
+	if (!n)
+		return NULL;
+
+	if (p) {
+		memcpy(n, p, old_size);
+		vfree(p);
+	}
+
+	return n;
+}
+
 #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
 #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)

Quoted 45 lines · Danilo Krummrich

Quoted 43 lines · Danilo Krummrich

Quoted 23 lines · Vlastimil Babka

Quoted 5 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.

Quoted 8 lines

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]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 6 lines

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 

Quoted 12 lines

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

Quoted 8 lines

mm/vmalloc.c +8 -0

+/**
+ * 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to 4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not new, kvrealloc() did the same before patch 2/2.

Taking it (too) literal, it's not wrong. The contents of the object pointed to are indeed preserved up to the lesser of the new and old size. It's just that the rest may be "preserved" as well.

I work on implementing shrink and grow for vrealloc(). In the meantime I think we could probably just memset() spare memory to zero.

Quoted 6 lines

Probably, this was a bad idea. Even with shrinking implemented we'd need to memset() potential spare memory of the last page to zero, when new_size < old_size.

Analogously, the same would be true for krealloc() buckets. That's probably not worth it.

Quoted 4 lines

I think we should indeed just document that __GFP_ZERO doesn't work for re-allocating memory and start to warn about it. As already mentioned, I think we should at least gurantee that *realloc(NULL, size, flags | __GFP_ZERO) is valid, i.e. WARN_ON(p && flags & __GFP_ZERO).

Maybe I spoke a bit to soon with this last paragraph. I think continuously gowing something with __GFP_ZERO is a legitimate use case. I just did a quick grep for users of krealloc() with __GFP_ZERO and found 18 matches.

So, I think, at least for now, we should instead document that __GFP_ZERO is only fully honored when the buffer is grown continuously (without intermediate shrinking) and __GFP_ZERO is supplied in every iteration.

In case I miss something here, and not even this case is safe, it looks like we have 18 broken users of krealloc().

Quoted 86 lines

Quoted 86 lines

nommu would still uses krealloc() though...

Quoted 4 lines

But it's also fundamentally not true for krealloc(), or kvrealloc() switching from a kmalloc to valloc. ksize() returns the size of the kmalloc bucket, we don't know what was the exact prior allocation size.

Probably a stupid question, but can't we just zero the full bucket initially and make sure to memset() spare memory in the bucket to zero when krealloc() is called with new_size < ksize()?

Quoted 4 lines

Worse, we
started poisoning the padding in debug configurations, so even a kmalloc(__GFP_ZERO) followed by krealloc(__GFP_ZERO) can give you unexpected poison now...

As in writing magics directly to the spare memory in the bucket? Which would then also be copied over to a new buffer in __do_krealloc()?

Quoted 3 lines

I guess we should just document __GFP_ZERO is not honored at all for realloc, and maybe start even warning :/ Hopefully nobody relies on that.

I think it'd be great to make __GFP_ZERO work in all cases. However, if that's really not possible, I'd prefer if we could at least gurantee that *realloc(NULL, size, flags | __GFP_ZERO) is a valid call, i.e. WARN_ON(p && flags & __GFP_ZERO).

Quoted 56 lines

Quoted 55 lines

mm/vmalloc.c +51 -0

+ *
+ * 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;
+
+	if (!size) {
+		vfree(p);
+		return NULL;
+	}
+
+	if (p) {
+		struct vm_struct *vm;
+
+		vm = find_vm_area(p);
+		if (unlikely(!vm)) {
+			WARN(1, "Trying to vrealloc() nonexistent vm area (%p)\n", p);
+			return NULL;
+		}
+
+		old_size = get_vm_area_size(vm);
+	}
+
+	if (size <= old_size) {
+		/*
+		 * TODO: Shrink the vm_area, i.e. unmap and free unused pages.
+		 * What would be a good heuristic for when to shrink the
+		 * vm_area?
+		 */
+		return (void *)p;
+	}
+
+	/* TODO: Grow the vm_area, i.e. allocate and map additional pages. */
+	n = __vmalloc_noprof(size, flags);
+	if (!n)
+		return NULL;
+
+	if (p) {
+		memcpy(n, p, old_size);
+		vfree(p);
+	}
+
+	return n;
+}
+
 #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
 #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)

Quoted 50 lines · Danilo Krummrich

Quoted 49 lines · Danilo Krummrich

Quoted 37 lines · Danilo Krummrich

Quoted 23 lines · Vlastimil Babka

Quoted 5 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.

Quoted 8 lines

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]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 6 lines

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 

Quoted 12 lines

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

Quoted 8 lines

mm/vmalloc.c +8 -0

+/**
+ * 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to 4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not new, kvrealloc() did the same before patch 2/2.

Quoted 6 lines

Taking it (too) literal, it's not wrong. The contents of the object pointed to are indeed preserved up to the lesser of the new and old size. It's just that the rest may be "preserved" as well.

I work on implementing shrink and grow for vrealloc(). In the meantime I think we could probably just memset() spare memory to zero.

Probably, this was a bad idea. Even with shrinking implemented we'd need to memset() potential spare memory of the last page to zero, when new_size < old_size.

Quoted 2 lines

Quoted 2 lines

Analogously, the same would be true for krealloc() buckets. That's probably not worth it.

I think it could remove unexpected bad surprises with the API so why not do it.

Quoted 10 lines

Quoted 5 lines

I think we should indeed just document that __GFP_ZERO doesn't work for re-allocating memory and start to warn about it. As already mentioned, I think we should at least gurantee that *realloc(NULL, size, flags | __GFP_ZERO) is valid, i.e. WARN_ON(p && flags & __GFP_ZERO).

Maybe I spoke a bit to soon with this last paragraph. I think continuously gowing something with __GFP_ZERO is a legitimate use case. I just did a quick grep for users of krealloc() with __GFP_ZERO and found 18 matches.

Quoted 6 lines

So, I think, at least for now, we should instead document that __GFP_ZERO is only fully honored when the buffer is grown continuously (without intermediate shrinking) and __GFP_ZERO is supplied in every iteration.

In case I miss something here, and not even this case is safe, it looks like we have 18 broken users of krealloc().

+CC Feng Tang

Let's say we kmalloc(56, __GFP_ZERO), we get an object from kmalloc-64 cache. Since commit 946fa0dbf2d89 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") and preceding commits, if slub_debug is enabled (red zoning or user tracking), only the 56 bytes will be zeroed. The rest will be either unknown garbage, or redzone.

Then we might e.g. krealloc(120) and get a kmalloc-128 object and 64 bytes (result of ksize()) will be copied, including the garbage/redzone. I think it's fixable because when we do this in slub_debug, we also store the original size in the metadata, so we could read it back and adjust how many bytes are copied.

Then we could guarantee that if __GFP_ZERO is used consistently on initial kmalloc() and on krealloc() and the user doesn't corrupt the extra space themselves (which is a bug anyway that the redzoning is supposed to catch) all will be fine.

There might be also KASAN side to this, I see poison_kmalloc_redzone() is also redzoning the area between requested size and cache's object_size?

Quoted 86 lines

Quoted 86 lines

Quoted 86 lines

nommu would still uses krealloc() though...

Quoted 4 lines

But it's also fundamentally not true for krealloc(), or kvrealloc() switching from a kmalloc to valloc. ksize() returns the size of the kmalloc bucket, we don't know what was the exact prior allocation size.

Probably a stupid question, but can't we just zero the full bucket initially and make sure to memset() spare memory in the bucket to zero when krealloc() is called with new_size < ksize()?

Quoted 4 lines

Worse, we
started poisoning the padding in debug configurations, so even a kmalloc(__GFP_ZERO) followed by krealloc(__GFP_ZERO) can give you unexpected poison now...

As in writing magics directly to the spare memory in the bucket? Which would then also be copied over to a new buffer in __do_krealloc()?

Quoted 3 lines

I guess we should just document __GFP_ZERO is not honored at all for realloc, and maybe start even warning :/ Hopefully nobody relies on that.

I think it'd be great to make __GFP_ZERO work in all cases. However, if that's really not possible, I'd prefer if we could at least gurantee that *realloc(NULL, size, flags | __GFP_ZERO) is a valid call, i.e. WARN_ON(p && flags & __GFP_ZERO).

Quoted 56 lines

Quoted 55 lines

mm/vmalloc.c +51 -0

+ *
+ * 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;
+
+	if (!size) {
+		vfree(p);
+		return NULL;
+	}
+
+	if (p) {
+		struct vm_struct *vm;
+
+		vm = find_vm_area(p);
+		if (unlikely(!vm)) {
+			WARN(1, "Trying to vrealloc() nonexistent vm area (%p)\n", p);
+			return NULL;
+		}
+
+		old_size = get_vm_area_size(vm);
+	}
+
+	if (size <= old_size) {
+		/*
+		 * TODO: Shrink the vm_area, i.e. unmap and free unused pages.
+		 * What would be a good heuristic for when to shrink the
+		 * vm_area?
+		 */
+		return (void *)p;
+	}
+
+	/* TODO: Grow the vm_area, i.e. allocate and map additional pages. */
+	n = __vmalloc_noprof(size, flags);
+	if (!n)
+		return NULL;
+
+	if (p) {
+		memcpy(n, p, old_size);
+		vfree(p);
+	}
+
+	return n;
+}
+
 #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
 #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)

Quoted 54 lines · Vlastimil Babka

Quoted 52 lines · Danilo Krummrich

Quoted 51 lines · Danilo Krummrich

Quoted 37 lines · Danilo Krummrich

Quoted 23 lines · Vlastimil Babka

Quoted 5 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.

Quoted 8 lines

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]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 6 lines

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 

Quoted 12 lines

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

Quoted 8 lines

mm/vmalloc.c +8 -0

+/**
+ * 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to 4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not new, kvrealloc() did the same before patch 2/2.

Quoted 6 lines

Taking it (too) literal, it's not wrong. The contents of the object pointed to are indeed preserved up to the lesser of the new and old size. It's just that the rest may be "preserved" as well.

I work on implementing shrink and grow for vrealloc(). In the meantime I think we could probably just memset() spare memory to zero.

Probably, this was a bad idea. Even with shrinking implemented we'd need to memset() potential spare memory of the last page to zero, when new_size < old_size.

Analogously, the same would be true for krealloc() buckets. That's probably not worth it.

Quoted 2 lines

I think it could remove unexpected bad surprises with the API so why not do it.

We'd either need to do it *every* time we shrink an allocation on spec, or we only do it when shrinking with __GFP_ZERO flag set, which might be a bit counter-intuitive.

If we do it, I'd probably vote for the latter semantics. While it sounds more error prone, it's less wasteful and enough to cover the most common case where the actual *realloc() call is always with the same parameters, but a changing size.

Quoted 55 lines · Danilo Krummrich

Quoted 54 lines · Vlastimil Babka

Quoted 52 lines · Danilo Krummrich

Quoted 51 lines · Danilo Krummrich

Quoted 37 lines · Danilo Krummrich

Quoted 23 lines · Vlastimil Babka

Quoted 5 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.

Quoted 8 lines

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]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 6 lines

mm/vmalloc.c +0 -0

@@ -4037,6 +4037,65 @@ void *vzalloc_node_noprof(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node_noprof);
 

Quoted 12 lines

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

Quoted 8 lines

mm/vmalloc.c +8 -0

+/**
+ * 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to 4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not new, kvrealloc() did the same before patch 2/2.

Quoted 6 lines

Taking it (too) literal, it's not wrong. The contents of the object pointed to are indeed preserved up to the lesser of the new and old size. It's just that the rest may be "preserved" as well.

I work on implementing shrink and grow for vrealloc(). In the meantime I think we could probably just memset() spare memory to zero.

Probably, this was a bad idea. Even with shrinking implemented we'd need to memset() potential spare memory of the last page to zero, when new_size < old_size.

Analogously, the same would be true for krealloc() buckets. That's probably not worth it.

Quoted 6 lines

Quoted 2 lines

I think it could remove unexpected bad surprises with the API so why not do it.

We'd either need to do it *every* time we shrink an allocation on spec, or we only do it when shrinking with __GFP_ZERO flag set, which might be a bit counter-intuitive.

I don't think it is that much counterintuitive.

Quoted 4 lines

If we do it, I'd probably vote for the latter semantics. While it sounds more error prone, it's less wasteful and enough to cover the most common case where the actual *realloc() call is always with the same parameters, but a changing size.

Yeah. Or with hardening enabled (init_on_alloc) it could be done always.

On 7/30/24 3:35 AM, Danilo Krummrich wrote:

Quoted 5 lines

Maybe I spoke a bit to soon with this last paragraph. I think continuously gowing something with __GFP_ZERO is a legitimate use case. I just did a quick grep for users of krealloc() with __GFP_ZERO and found 18 matches.

Quoted 6 lines

So, I think, at least for now, we should instead document that __GFP_ZERO is only fully honored when the buffer is grown continuously (without intermediate shrinking) and __GFP_ZERO is supplied in every iteration.

In case I miss something here, and not even this case is safe, it looks like we have 18 broken users of krealloc().

+CC Feng Tang

Let's say we kmalloc(56, __GFP_ZERO), we get an object from kmalloc-64 cache. Since commit 946fa0dbf2d89 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") and preceding commits, if slub_debug is enabled (red zoning or user tracking), only the 56 bytes will be zeroed. The rest will be either unknown garbage, or redzone.

Then we might e.g. krealloc(120) and get a kmalloc-128 object and 64 bytes (result of ksize()) will be copied, including the garbage/redzone. I think it's fixable because when we do this in slub_debug, we also store the original size in the metadata, so we could read it back and adjust how many bytes are copied.

Then we could guarantee that if __GFP_ZERO is used consistently on initial kmalloc() and on krealloc() and the user doesn't corrupt the extra space themselves (which is a bug anyway that the redzoning is supposed to catch) all will be fine.

There might be also KASAN side to this, I see poison_kmalloc_redzone() is also redzoning the area between requested size and cache's object_size?

Quoted 1 line · Vlastimil Babka

On 7/30/24 3:35 AM, Danilo Krummrich wrote:

Quoted 6 lines

Let's say we kmalloc(56, __GFP_ZERO), we get an object from kmalloc-64 cache. Since commit 946fa0dbf2d89 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") and preceding commits, if slub_debug is enabled (red zoning or user tracking), only the 56 bytes will be zeroed. The rest will be either unknown garbage, or redzone.

Yes.

Quoted 6 lines

Then we might e.g. krealloc(120) and get a kmalloc-128 object and 64 bytes (result of ksize()) will be copied, including the garbage/redzone. I think it's fixable because when we do this in slub_debug, we also store the original size in the metadata, so we could read it back and adjust how many bytes are copied.

krealloc() --> __do_krealloc() --> ksize()
When ksize() is called, as we don't know what user will do with the extra space ([57, 64] here), the orig_size check will be unset by __ksize() calling skip_orig_size_check().

And if the newsize is bigger than the old 'ksize', the 'orig_size' will be correctly set for the newly allocated kmalloc object.

For the 'unstable' branch of -mm tree, which has all latest patches from Danilo, I run some basic test and it seems to be fine.

Quoted 2 lines · Feng

Quoted 1 line · Vlastimil Babka

On 7/30/24 3:35 AM, Danilo Krummrich wrote:

[...]

Quoted 16 lines

Quoted 6 lines

Let's say we kmalloc(56, __GFP_ZERO), we get an object from kmalloc-64 cache. Since commit 946fa0dbf2d89 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") and preceding commits, if slub_debug is enabled (red zoning or user tracking), only the 56 bytes will be zeroed. The rest will be either unknown garbage, or redzone.

Yes.

Quoted 6 lines

Then we might e.g. krealloc(120) and get a kmalloc-128 object and 64 bytes (result of ksize()) will be copied, including the garbage/redzone. I think it's fixable because when we do this in slub_debug, we also store the original size in the metadata, so we could read it back and adjust how many bytes are copied.

Quoted 7 lines

krealloc() --> __do_krealloc() --> ksize()
When ksize() is called, as we don't know what user will do with the extra space ([57, 64] here), the orig_size check will be unset by __ksize() calling skip_orig_size_check().

And if the newsize is bigger than the old 'ksize', the 'orig_size' will be correctly set for the newly allocated kmalloc object.

Quoted 2 lines

For the 'unstable' branch of -mm tree, which has all latest patches from Danilo, I run some basic test and it seems to be fine.

when doing more test, I found one case matching Vlastimil's previous concern, that if we kzalloc a small object, and then krealloc with a slightly bigger size which can still reuse the kmalloc object, some redzone will be preserved.

With test code like:

	buf = kzalloc(36, GFP_KERNEL);
	memset(buf, 0xff, 36);

	buf = krealloc(buf, 48, GFP_KERNEL | __GFP_ZERO);

Data after kzalloc+memset :

ffff88802189b040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b060: ff ff ff ff cc cc cc cc cc cc cc cc cc cc cc cc
ffff88802189b070: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc

Data after krealloc:

ffff88802189b040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b060: ff ff ff ff cc cc cc cc cc cc cc cc cc cc cc cc
ffff88802189b070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

If we really want to make [37, 48] to be zeroed too, we can lift the get_orig_size() from slub.c to slab_common.c and use it as the start of zeroing in krealloc().

Thanks,
Feng

Quoted 25 lines · Feng Tang

Quoted 2 lines · Feng

Quoted 1 line · Vlastimil Babka

On 7/30/24 3:35 AM, Danilo Krummrich wrote:

[...]

Quoted 21 lines

Quoted 6 lines

Let's say we kmalloc(56, __GFP_ZERO), we get an object from kmalloc-64 cache. Since commit 946fa0dbf2d89 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") and preceding commits, if slub_debug is enabled (red zoning or user tracking), only the 56 bytes will be zeroed. The rest will be either unknown garbage, or redzone.

Yes.

Quoted 6 lines

Then we might e.g. krealloc(120) and get a kmalloc-128 object and 64 bytes (result of ksize()) will be copied, including the garbage/redzone. I think it's fixable because when we do this in slub_debug, we also store the original size in the metadata, so we could read it back and adjust how many bytes are copied.

krealloc() --> __do_krealloc() --> ksize()
When ksize() is called, as we don't know what user will do with the extra space ([57, 64] here), the orig_size check will be unset by __ksize() calling skip_orig_size_check().

Quoted 2 lines

Quoted 2 lines

And if the newsize is bigger than the old 'ksize', the 'orig_size' will be correctly set for the newly allocated kmalloc object.

Yes, but the memcpy() to the new object will be done using ksize() thus include the redzone, e.g. [57, 64]

Quoted 3 lines

Quoted 3 lines

For the 'unstable' branch of -mm tree, which has all latest patches from Danilo, I run some basic test and it seems to be fine.

To test it would not always be enough to expect some slub_debug to fail, you'd e.g. have to kmalloc(48, GFP_KERNEL | GFP_ZERO), krealloc(128, GFP_KERNEL | GFP_ZERO) and then verify there are zeroes from 48 to 128. I suspect there won't be zeroes from 48 to 64 due to redzone.

(this would have made a great lib/slub_kunit.c test :))

Quoted 21 lines

when doing more test, I found one case matching Vlastimil's previous concern, that if we kzalloc a small object, and then krealloc with a slightly bigger size which can still reuse the kmalloc object, some redzone will be preserved.

With test code like:

	buf = kzalloc(36, GFP_KERNEL);
	memset(buf, 0xff, 36);

	buf = krealloc(buf, 48, GFP_KERNEL | __GFP_ZERO);

Data after kzalloc+memset :

ffff88802189b040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b060: ff ff ff ff cc cc cc cc cc cc cc cc cc cc cc cc
ffff88802189b070: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc

Data after krealloc:

Quoted 8 lines

ffff88802189b040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff88802189b060: ff ff ff ff cc cc cc cc cc cc cc cc cc cc cc cc
ffff88802189b070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

If we really want to make [37, 48] to be zeroed too, we can lift the get_orig_size() from slub.c to slab_common.c and use it as the start of zeroing in krealloc().

Or maybe just move krealloc() to mm/slub.c so there are no unnecessary calls between the files.

We should also set a new orig_size in cases we are shrinking or enlarging within same object (i.e. 48->40 or 48->64). In case of shrinking, we also might need to redzone the shrinked area (i.e. [40, 48]) or later checks will fail. But if the current object is from kfence, then probably not do any of this... sigh this gets complicated. And really we need kunit tests for all the scenarios :/

On Mon, Sep 02, 2024 at 10:56:57AM +0200, Vlastimil Babka wrote:

Quoted 7 lines

Quoted 3 lines

If we really want to make [37, 48] to be zeroed too, we can lift the get_orig_size() from slub.c to slab_common.c and use it as the start of zeroing in krealloc().

Or maybe just move krealloc() to mm/slub.c so there are no unnecessary calls between the files.

Quoted 6 lines

We should also set a new orig_size in cases we are shrinking or enlarging within same object (i.e. 48->40 or 48->64). In case of shrinking, we also might need to redzone the shrinked area (i.e. [40, 48]) or later checks will fail. But if the current object is from kfence, then probably not do any of this... sigh this gets complicated. And really we need kunit tests for all the scenarios :/

Good point! will think about and try to implement it to ensure the orig_size and kmalloc-redzone check setting is kept.

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.

Quoted 6 lines · On Mon, 22 Jul 2024 18:29:24 +0200 Danilo Krummrich <[email protected]> wrote:

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.

The old kvrealloc() behavior actually sounds somewhat useful. You've checked that no existing sites were relying on this?

And that all existing kvrealloc() callers were (incorrectly) checking for NULL? Seems that way.

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() can be optimized to allocate and map additional pages to grow the allocation or unmap and free unused pages to shrink the allocation.

mm/util.c +9 -1

-void *kvrealloc_noprof(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
+/**
+ * kvrealloc - reallocate 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).
+ *

mm/util.c +10 -8

+		if (!n)
+			return NULL;
+
+		if (p) {
+			/* We already know that `p` is not a vmalloc address. */
+			memcpy(n, p, ksize(p));
+			kfree(p);
+		}
+	}
 
-	if (oldsize >= newsize)
-		return (void *)p;
-	newp = kvmalloc_noprof(newsize, flags);
-	if (!newp)
-		return NULL;
-	memcpy(newp, p, oldsize);
-	kvfree(p);
-	return newp;
+	return n;
 }
 EXPORT_SYMBOL(kvrealloc_noprof);

Signature · 2 lines

-- 
2.45.2

Quoted 11 lines · 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.

Quoted 6 lines

- 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.

I completely agree with this. Fortunately the number of existing callers is small and none of them really seem to depend on the current behavior in that aspect.

Quoted 5 lines

Besides that, implementing kvrealloc() by making use of krealloc() and vrealloc() provides oppertunities to grow (and shrink) allocations more efficiently. For instance, vrealloc() can be optimized to allocate and map additional pages to grow the allocation or unmap and free unused pages to shrink the allocation.

This seems like a change that is independent on the above and should be a patch on its own.

[...]

Quoted 8 lines

mm/util.c +0 -0

index bc488f0121a7..0ff5898cc6de 100644
@@ -608,6 +608,28 @@ unsigned long vm_mmap(struct file *file, unsigned long addr,
 }
 EXPORT_SYMBOL(vm_mmap);
 

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

Quoted 1 line

mm/util.c +1 -0

+static gfp_t kmalloc_gfp_adjust(gfp_t flags, size_t size)

This seems like a generally useful helper which it is not. I would call it something like __kvmalloc_gfp_adjust or something similar so that it is clear that this is just a helper to adjust gfp flag for slab allocator path

[...]

Quoted 10 lines

mm/util.c +9 -1

-void *kvrealloc_noprof(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
+/**
+ * kvrealloc - reallocate 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).
+ *

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 740

Quoted 10 lines

mm/util.c +8 -1

+ * If @p is %NULL, kvrealloc() behaves exactly like kvmalloc(). If @size is 0
+ * and @p is not a %NULL pointer, the object pointed to is freed.
+ *
+ * Return: pointer to the allocated memory or %NULL in case of error
+ */
+void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
 {
-	void *newp;
+	void *n;
+

	if (!size && p) {
		kvfree(p);
		return NULL;
	}

would make this code flow slightly easier to read because the freeing path would be shared for all compbinations IMO.

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 747

Quoted 8 lines

mm/util.c +7 -0

+	if (is_vmalloc_addr(p))
+		return vrealloc_noprof(p, size, flags);
+
+	n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
+	if (!n) {
+		/* We failed to krealloc(), fall back to kvmalloc(). */
+		n = kvmalloc_noprof(size, flags);

Why don't you simply use vrealloc_noprof here?

Quoted 25 lines

mm/util.c +10 -8

+		if (!n)
+			return NULL;
+
+		if (p) {
+			/* We already know that `p` is not a vmalloc address. */
+			memcpy(n, p, ksize(p));
+			kfree(p);
+		}
+	}
 
-	if (oldsize >= newsize)
-		return (void *)p;
-	newp = kvmalloc_noprof(newsize, flags);
-	if (!newp)
-		return NULL;
-	memcpy(newp, p, oldsize);
-	kvfree(p);
-	return newp;
+	return n;
 }
 EXPORT_SYMBOL(kvrealloc_noprof);

Signature · 2 lines

-- 
2.45.2

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 1 line · Michal Hocko

On Mon 22-07-24 18:29:24, Danilo Krummrich wrote:

Quoted 8 lines

Quoted 5 lines

Besides that, implementing kvrealloc() by making use of krealloc() and vrealloc() provides oppertunities to grow (and shrink) allocations more efficiently. For instance, vrealloc() can be optimized to allocate and map additional pages to grow the allocation or unmap and free unused pages to shrink the allocation.

This seems like a change that is independent on the above and should be a patch on its own.

The optimizations you mean? Yes, I intend to do this in a separate series. For now, I put TODOs in vrealloc.

Quoted 11 lines

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 740

Quoted 5 lines

mm/util.c +3 -1

+void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
 {
-	void *newp;
+	void *n;
+

	if (!size && p) {
		kvfree(p);
		return NULL;
	}

Quoted 2 lines

would make this code flow slightly easier to read because the freeing path would be shared for all compbinations IMO.

Personally, I like it without. For me the simplicity comes from directing things to either krealloc() or vrealloc(). But I'd be open to change it however.

Quoted 9 lines

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 747

Quoted 7 lines

mm/util.c +7 -0

+	if (is_vmalloc_addr(p))
+		return vrealloc_noprof(p, size, flags);
+
+	n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
+	if (!n) {
+		/* We failed to krealloc(), fall back to kvmalloc(). */
+		n = kvmalloc_noprof(size, flags);

Why don't you simply use vrealloc_noprof here?

We could do that, but we'd also need to do the same checks kvmalloc() does, i.e.

	/*
	 * It doesn't really make sense to fallback to vmalloc for sub page
	 * requests
	 */
	if (ret || size <= PAGE_SIZE)
		return ret;

	/* non-sleeping allocations are not supported by vmalloc */
	if (!gfpflags_allow_blocking(flags))
		return NULL;

	/* Don't even allow crazy sizes */
	if (unlikely(size > INT_MAX)) {
		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
		return NULL;
	}

Does the kmalloc() retry through kvmalloc() hurt us enough to do that? This should only ever happen when we switch from a kmalloc buffer to a vmalloc buffer, which we only do once, we never switch back.

Quoted 2 lines · Danilo Krummrich

Quoted 1 line · Michal Hocko

On Mon 22-07-24 18:29:24, Danilo Krummrich wrote:

[...]

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

Besides that, implementing kvrealloc() by making use of krealloc() and vrealloc() provides oppertunities to grow (and shrink) allocations more efficiently. For instance, vrealloc() can be optimized to allocate and map additional pages to grow the allocation or unmap and free unused pages to shrink the allocation.

This seems like a change that is independent on the above and should be a patch on its own.

Quoted 2 lines

The optimizations you mean? Yes, I intend to do this in a separate series. For now, I put TODOs in vrealloc.

No I mean, that the change of the signature and semantic should be done along with update to callers and the new implementation of the function itself should be done in its own patch.

[...]

Quoted 11 lines

Quoted 11 lines

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 740

Quoted 5 lines

mm/util.c +3 -1

+void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
 {
-	void *newp;
+	void *n;
+

	if (!size && p) {
		kvfree(p);
		return NULL;
	}

Quoted 5 lines

Quoted 2 lines

would make this code flow slightly easier to read because the freeing path would be shared for all compbinations IMO.

Personally, I like it without. For me the simplicity comes from directing things to either krealloc() or vrealloc(). But I'd be open to change it however.

I would really prefer to have it there because it makes the follow up code easier.

Quoted 13 lines

Quoted 10 lines

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 747

Quoted 8 lines

mm/util.c +7 -0

+	if (is_vmalloc_addr(p))
+		return vrealloc_noprof(p, size, flags);
+
+	n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
+	if (!n) {
+		/* We failed to krealloc(), fall back to kvmalloc(). */
+		n = kvmalloc_noprof(size, flags);

Why don't you simply use vrealloc_noprof here?

We could do that, but we'd also need to do the same checks kvmalloc() does, i.e.

Quoted 6 lines

	/*
	 * It doesn't really make sense to fallback to vmalloc for sub page
	 * requests
	 */
	if (ret || size <= PAGE_SIZE)
		return ret;

With the early !size && p check we wouldn't right?

Quoted 6 lines

	/* non-sleeping allocations are not supported by vmalloc */
	if (!gfpflags_allow_blocking(flags))
		return NULL;

Quoted 5 lines

	/* Don't even allow crazy sizes */
	if (unlikely(size > INT_MAX)) {
		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
		return NULL;
	}

I do not see why kvrealloc should have different set of constrains than vrealloc in this regards.

Quoted 4 lines

Does the kmalloc() retry through kvmalloc() hurt us enough to do that? This should only ever happen when we switch from a kmalloc buffer to a vmalloc buffer, which we only do once, we never switch back.

This is effectively open coding part of vrealloc without any good reason. Please get rid of that.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 13 lines · Michal Hocko

Quoted 2 lines · Danilo Krummrich

Quoted 1 line · Michal Hocko

On Mon 22-07-24 18:29:24, Danilo Krummrich wrote:

[...]

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

Besides that, implementing kvrealloc() by making use of krealloc() and vrealloc() provides oppertunities to grow (and shrink) allocations more efficiently. For instance, vrealloc() can be optimized to allocate and map additional pages to grow the allocation or unmap and free unused pages to shrink the allocation.

This seems like a change that is independent on the above and should be a patch on its own.

Quoted 6 lines

Quoted 2 lines

The optimizations you mean? Yes, I intend to do this in a separate series. For now, I put TODOs in vrealloc.

No I mean, that the change of the signature and semantic should be done along with update to callers and the new implementation of the function itself should be done in its own patch.

Sorry, it seems like you lost me a bit.

There is one patch that implements vrealloc() and one patch that does the change of krealloc()'s signature, semantics and the corresponding update to the callers.

Isn't that already what you ask for?

Quoted 18 lines

[...]

Quoted 16 lines

Quoted 11 lines

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 740

Quoted 5 lines

mm/util.c +3 -1

+void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
 {
-	void *newp;
+	void *n;
+

	if (!size && p) {
		kvfree(p);
		return NULL;
	}

Quoted 2 lines

would make this code flow slightly easier to read because the freeing path would be shared for all compbinations IMO.

Personally, I like it without. For me the simplicity comes from directing things to either krealloc() or vrealloc(). But I'd be open to change it however.

Quoted 2 lines

I would really prefer to have it there because it makes the follow up code easier.

I don't think it does (see below).

Either way, I got notified that Andrew applied the patches to mm-unstable. How to proceed from there for further changes, if any?

Quoted 20 lines

Quoted 19 lines

Quoted 10 lines

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 747

Quoted 8 lines

mm/util.c +7 -0

+	if (is_vmalloc_addr(p))
+		return vrealloc_noprof(p, size, flags);
+
+	n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
+	if (!n) {
+		/* We failed to krealloc(), fall back to kvmalloc(). */
+		n = kvmalloc_noprof(size, flags);

Why don't you simply use vrealloc_noprof here?

We could do that, but we'd also need to do the same checks kvmalloc() does, i.e.

	/*
	 * It doesn't really make sense to fallback to vmalloc for sub page
	 * requests
	 */
	if (ret || size <= PAGE_SIZE)
		return ret;

Quoted 1 line

With the early !size && p check we wouldn't right?

I think that's unrelated. Your proposed early check checks for size == 0 to free and return early. Whereas this check bails out if we fail kmalloc() with size <= PAGE_SIZE, because a subsequent vmalloc() wouldn't make a lot of sense.

Quoted 23 lines · Danilo Krummrich

Quoted 13 lines · Michal Hocko

Quoted 2 lines · Danilo Krummrich

Quoted 1 line · Michal Hocko

On Mon 22-07-24 18:29:24, Danilo Krummrich wrote:

[...]

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

Besides that, implementing kvrealloc() by making use of krealloc() and vrealloc() provides oppertunities to grow (and shrink) allocations more efficiently. For instance, vrealloc() can be optimized to allocate and map additional pages to grow the allocation or unmap and free unused pages to shrink the allocation.

This seems like a change that is independent on the above and should be a patch on its own.

Quoted 6 lines

Quoted 2 lines

The optimizations you mean? Yes, I intend to do this in a separate series. For now, I put TODOs in vrealloc.

No I mean, that the change of the signature and semantic should be done along with update to callers and the new implementation of the function itself should be done in its own patch.

Sorry, it seems like you lost me a bit.

Quoted 5 lines

There is one patch that implements vrealloc() and one patch that does the change of krealloc()'s signature, semantics and the corresponding update to the callers.

Isn't that already what you ask for?

No, because this second patch reimplements kvrealloc wo to use krealloc and vrealloc fallback. More clear now?

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

Quoted 9 lines

mm/util.c void kvfree_sensitive(const void *addr, size_t len) · line 747

Quoted 7 lines

mm/util.c +7 -0

+	if (is_vmalloc_addr(p))
+		return vrealloc_noprof(p, size, flags);
+
+	n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
+	if (!n) {
+		/* We failed to krealloc(), fall back to kvmalloc(). */
+		n = kvmalloc_noprof(size, flags);

Why don't you simply use vrealloc_noprof here?

We could do that, but we'd also need to do the same checks kvmalloc() does, i.e.

	/*
	 * It doesn't really make sense to fallback to vmalloc for sub page
	 * requests
	 */
	if (ret || size <= PAGE_SIZE)
		return ret;

Quoted 5 lines

Quoted 1 line

With the early !size && p check we wouldn't right?

I think that's unrelated. Your proposed early check checks for size == 0 to free and return early. Whereas this check bails out if we fail kmalloc() with size <= PAGE_SIZE, because a subsequent vmalloc() wouldn't make a lot of sense.

It seems we are not on the same page here. Here is what I would like kvrealloc to look like in the end:

void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)

{
        void *newp;

        if (!size && p) {
                kvfree(p);
                return NULL;
        }

        if (!is_vmalloc_addr(p))
                newp = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));

        if (newp)
                return newp;

        return vrealloc_noprof(p, size, flags);
}

EXPORT_SYMBOL(kvrealloc_noprof);

krealloc_noprof should be extended for the maximum allowed size

and so does vrealloc_noprof.

The implementation of the kvrealloc cannot get any easier and more straightforward AFAICS. See my point?

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 1 line · Michal Hocko

On Tue 23-07-24 13:55:48, Danilo Krummrich wrote:

Quoted 15 lines

void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)

{
        void *newp;

        if (!size && p) {
                kvfree(p);
                return NULL;
        }

        if (!is_vmalloc_addr(p))
                newp = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));

        if (newp)
                return newp;

Quoted 3 lines

        return vrealloc_noprof(p, size, flags);
}

EXPORT_SYMBOL(kvrealloc_noprof);

This looks weird. The fact that you're passing p to vrealloc_noprof() if krealloc_noprof() fails, implies that vrealloc_noprof() must be able to deal with pointers to kmalloc'd memory.

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() can be optimized to allocate and map additional pages to grow the allocation or unmap and free unused pages to shrink the allocation.

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

To both patches:

Acked-by Michal Hocko <[email protected]>

Sorry, I was a bit dense today.

Signature · 3 lines

-- 
Michal Hocko
SUSE Labs