(k)vrealloc (__GFP_ZERO) fixes

PATCH In review Danilo Krummrich 3 participants 12 messages Jul 30, 2024 – Jul 31, 2024

Subsystems (3)

Memory management MM core vmalloc

lore

Share & tools

lore

Danilo Krummrich

author

2024-07-30 18:50 UTC #0 lore raw

cover (k)vrealloc (__GFP_ZERO) fixes

Fixups for [1], as discussed.

[1] https://lore.kernel.org/linux-mm/[email protected]/

Danilo Krummrich (4):
mm: kvrealloc: disable KASAN when switching to vmalloc
mm: vrealloc: consider spare memory for __GFP_ZERO
mm: vrealloc: properly document __GFP_ZERO behavior
mm: kvrealloc: properly document __GFP_ZERO behavior

 mm/util.c    | 16 ++++++++++++----
 mm/vmalloc.c | 24 ++++++++++++++++--------
 2 files changed, 28 insertions(+), 12 deletions(-)

base-commit: 4152c2f7b8af8c270686a4aabda302ec22b0e099

Signature · 1 line

2.45.2

Danilo Krummrich

author

2024-07-30 18:50 UTC #1 lore raw

PATCH 1/4 mm: kvrealloc: disable KASAN when switching to vmalloc

Disable KASAN accessibility checks when switching from a kmalloc buffer to a vmalloc buffer.

Fixes 923a26b4c679 ("mm: kvmalloc: align kvrealloc() with krealloc()")

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

---
 mm/util.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

mm/util.c +4 -1

index 29ae93f6344f..bfb2d69b6434 100644
@@ -753,7 +753,10 @@ void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
 
 		if (p) {
 			/* We already know that `p` is not a vmalloc address. */
-			memcpy(n, p, ksize(p));
+			kasan_disable_current();
+			memcpy(n, kasan_reset_tag(p), ksize(p));
+			kasan_enable_current();
+
 			kfree(p);
 		}
 	}

Signature · 1 line

2.45.2

Andrew Morton

maintainer

2024-07-30 21:15 UTC #2 lore raw

Re: [PATCH 1/4] mm: kvrealloc: disable KASAN when switching to vmalloc

Quoted 2 lines · On Tue, 30 Jul 2024 20:49:41 +0200 Danilo Krummrich <[email protected]> wrote:

Disable KASAN accessibility checks when switching from a kmalloc buffer to a vmalloc buffer.

This text tells us "what", which was utterly evident from a cursory read of the code.

Please tell us the "why". Completely.

Quoted 1 line

Fixes 923a26b4c679 ("mm: kvmalloc: align kvrealloc() with krealloc()")

For those who are following along, this patch in mm-unstable so this patch will be squished into the above.

Danilo Krummrich

author

2024-07-30 22:48 UTC #3 lore raw

Re: [PATCH 1/4] mm: kvrealloc: disable KASAN when switching to vmalloc

Quoted 9 lines · Andrew Morton

Quoted 2 lines · On Tue, 30 Jul 2024 20:49:41 +0200 Danilo Krummrich <[email protected]> wrote:
Disable KASAN accessibility checks when switching from a kmalloc buffer to a vmalloc buffer.
This text tells us "what", which was utterly evident from a cursory read of the code.
Please tell us the "why". Completely.

As mentioned in the other reply. Since this is a fixup series for stuff that's in mm-unstable already, and hence gets squashed in later on, I treated this more like the list of changes one would also append when sending the next version of a series.

If you expect such fixup commits to have the same detail than regular ones, I'll surely add those details and resend. Please let me know.

Quoted 6 lines

Quoted 2 lines
Fixes 923a26b4c679 ("mm: kvmalloc: align kvrealloc() with krealloc()")
For those who are following along, this patch in mm-unstable so this patch will be squished into the above.

Danilo Krummrich

author

2024-07-30 18:50 UTC #4 lore raw

PATCH 2/4 mm: vrealloc: consider spare memory for __GFP_ZERO

In reply to Danilo Krummrich

Zero spare memory when shrinking a buffer with __GFP_ZERO.

Fixes 1f39ee9615a8 ("mm: vmalloc: implement vrealloc()")

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

---
 mm/vmalloc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

mm/vmalloc.c +8 -5

index 2a6d4ce57b73..6a2fef6378e4 100644
@@ -4077,12 +4077,15 @@ void *vrealloc_noprof(const void *p, size_t size, gfp_t flags)
 		old_size = get_vm_area_size(vm);
 	}
 
+	/*
+	 * 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?
+	 */
 	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?
-		 */
+		/* Zero out spare memory. */
+		if (want_init_on_alloc(flags))
+			memset((void *)p + size, 0, old_size - size);
+
 		return (void *)p;
 	}

Signature · 1 line

2.45.2

Andrew Morton

maintainer

2024-07-30 21:15 UTC #5 lore raw

Re: [PATCH 2/4] mm: vrealloc: consider spare memory for __GFP_ZERO

Quoted 1 line · On Tue, 30 Jul 2024 20:49:42 +0200 Danilo Krummrich <[email protected]> wrote:

Zero spare memory when shrinking a buffer with __GFP_ZERO.

Again, why?

Danilo Krummrich

author

2024-07-30 18:51 UTC #6 lore raw

PATCH 3/4 mm: vrealloc: properly document __GFP_ZERO behavior

In reply to Danilo Krummrich

Properly document that if __GFP_ZERO logic is requested, callers must ensure that, starting with the initial memory allocation, every subsequent call to this API for the same memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that __GFP_ZERO is not fully honored by this API.

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

---
 mm/vmalloc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

mm/vmalloc.c +8 -3

index 6a2fef6378e4..48cc10dd06c0 100644
@@ -4043,12 +4043,17 @@ EXPORT_SYMBOL(vzalloc_node_noprof);
  * @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.
  *
+ * If __GFP_ZERO logic is requested, callers must ensure that, starting with the
+ * initial memory allocation, every subsequent call to this API for the same
+ * memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that
+ * __GFP_ZERO is not fully honored by this API.
+ *
+ * In any case, the contents of the object pointed to are preserved up to the
+ * lesser of the new and old sizes.
+ *
  * This function must not be called concurrently with itself or vfree() for the
  * same memory allocation.
  *

Signature · 1 line

2.45.2

Andrew Morton

maintainer

2024-07-30 21:19 UTC #7 lore raw

Re: [PATCH 3/4] mm: vrealloc: properly document __GFP_ZERO behavior

Quoted 5 lines · On Tue, 30 Jul 2024 20:49:43 +0200 Danilo Krummrich <[email protected]> wrote:

Properly document that if __GFP_ZERO logic is requested, callers must ensure that, starting with the initial memory allocation, every subsequent call to this API for the same memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that __GFP_ZERO is not fully honored by this API.

I appear to have just seen this, in a separate mailing.

Please, slow down. We have two months. Await reviewer feedback, spend time over those changelogs, value clarity and accuracy and completeness over hastiness. The only reason for rushing things is if a patch is disrupting ongoing testing of the linux-next tree.

Danilo Krummrich

author

2024-07-30 22:43 UTC #8 lore raw

Re: [PATCH 3/4] mm: vrealloc: properly document __GFP_ZERO behavior

Quoted 9 lines · Andrew Morton

Quoted 5 lines · On Tue, 30 Jul 2024 20:49:43 +0200 Danilo Krummrich <[email protected]> wrote:
Properly document that if __GFP_ZERO logic is requested, callers must ensure that, starting with the initial memory allocation, every subsequent call to this API for the same memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that __GFP_ZERO is not fully honored by this API.
I appear to have just seen this, in a separate mailing.

What you have seen in a separate mail is a similar patch for krealloc() [1]. This one is a fixup for vrealloc() from a previous submission you've applied to mm-unstable.

Quoted 5 lines

Please, slow down. We have two months. Await reviewer feedback, spend time over those changelogs, value clarity and accuracy and completeness over hastiness. The only reason for rushing things is if a patch is disrupting ongoing testing of the linux-next tree.

There was a discussion in [2], which lead to this fixup series.

In terms of changelogs this series is indeed a bit "lax", since I have recognized that you queue up fixup patches for changes that did already land in mm-unstable to be squashed into the original commits later on.

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://lore.kernel.org/linux-mm/[email protected]/T/#m065a7f875b44dc945dd535c2b7168c3d77a98993

Vlastimil Babka

2024-07-31 14:43 UTC #9 lore raw

Re: [PATCH 3/4] mm: vrealloc: properly document __GFP_ZERO behavior

Quoted 21 lines · Danilo Krummrich

Quoted 9 lines · Andrew Morton
Quoted 5 lines · On Tue, 30 Jul 2024 20:49:43 +0200 Danilo Krummrich <[email protected]> wrote:
Properly document that if __GFP_ZERO logic is requested, callers must ensure that, starting with the initial memory allocation, every subsequent call to this API for the same memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that __GFP_ZERO is not fully honored by this API.
I appear to have just seen this, in a separate mailing.
What you have seen in a separate mail is a similar patch for krealloc() [1]. This one is a fixup for vrealloc() from a previous submission you've applied to mm-unstable.
Quoted 5 lines
Please, slow down. We have two months. Await reviewer feedback, spend time over those changelogs, value clarity and accuracy and completeness over hastiness. The only reason for rushing things is if a patch is disrupting ongoing testing of the linux-next tree.

Quoted 5 lines

There was a discussion in [2], which lead to this fixup series.
In terms of changelogs this series is indeed a bit "lax", since I have recognized that you queue up fixup patches for changes that did already land in mm-unstable to be squashed into the original commits later on.

Some of the changes in the fixups would however ideally result in udpdates to the original changelogs in addition to squashing code. Also with 4 fixups to 2 original patches it might be IMHO better to squash on your side and resend as a full replacement. Perhaps also together with the other 2 patches about __GFP_ZERO for krealloc in a single series. As Andrew mentioned we are early in the rc phase to afford this.

Quoted 2 lines

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://lore.kernel.org/linux-mm/[email protected]/T/#m065a7f875b44dc945dd535c2b7168c3d77a98993

Danilo Krummrich

author

2024-07-31 15:10 UTC #10 lore raw

Re: [PATCH 3/4] mm: vrealloc: properly document __GFP_ZERO behavior

Quoted 28 lines · Vlastimil Babka

Quoted 21 lines · Danilo Krummrich
Quoted 9 lines · Andrew Morton
Quoted 5 lines · On Tue, 30 Jul 2024 20:49:43 +0200 Danilo Krummrich <[email protected]> wrote:
Properly document that if __GFP_ZERO logic is requested, callers must ensure that, starting with the initial memory allocation, every subsequent call to this API for the same memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that __GFP_ZERO is not fully honored by this API.
I appear to have just seen this, in a separate mailing.
What you have seen in a separate mail is a similar patch for krealloc() [1]. This one is a fixup for vrealloc() from a previous submission you've applied to mm-unstable.
Quoted 5 lines
Please, slow down. We have two months. Await reviewer feedback, spend time over those changelogs, value clarity and accuracy and completeness over hastiness. The only reason for rushing things is if a patch is disrupting ongoing testing of the linux-next tree.
Quoted 5 lines
There was a discussion in [2], which lead to this fixup series.
In terms of changelogs this series is indeed a bit "lax", since I have recognized that you queue up fixup patches for changes that did already land in mm-unstable to be squashed into the original commits later on.

Quoted 6 lines

Some of the changes in the fixups would however ideally result in udpdates to the original changelogs in addition to squashing code. Also with 4 fixups to 2 original patches it might be IMHO better to squash on your side and resend as a full replacement. Perhaps also together with the other 2 patches about __GFP_ZERO for krealloc in a single series. As Andrew mentioned we are early in the rc phase to afford this.

(JFYI, Andrew applied the fixups meanwhile.)

I also don't think that they lead to updates of the commit messages.

But yes, we can proceed with:

(1) leave [1] as it is (with the fixups applied to mm-unstable already) and send

    v2 of [2]

(2) send a v3 for [1] that also includes [2]
(3) send a v3 of [1] and a separate v2 for [2]

Just let me know what you prefer. I'm fine with either of those. :)

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://lore.kernel.org/linux-mm/[email protected]/

Danilo Krummrich

author

2024-07-30 18:51 UTC #11 lore raw

PATCH 4/4 mm: kvrealloc: properly document __GFP_ZERO behavior

In reply to Danilo Krummrich

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

---
 mm/util.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

mm/util.c +8 -3

index bfb2d69b6434..f899b0f984a0 100644
@@ -726,12 +726,17 @@ EXPORT_SYMBOL(kvfree_sensitive);
  * @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, kvrealloc() behaves exactly like kvmalloc(). If @size is 0
  * and @p is not a %NULL pointer, the object pointed to is freed.
  *
+ * If __GFP_ZERO logic is requested, callers must ensure that, starting with the
+ * initial memory allocation, every subsequent call to this API for the same
+ * memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that
+ * __GFP_ZERO is not fully honored by this API.
+ *
+ * In any case, the contents of the object pointed to are preserved up to the
+ * lesser of the new and old sizes.
+ *
  * This function must not be called concurrently with itself or kvfree() for the
  * same memory allocation.
  *

Signature · 1 line

2.45.2