public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] kmsan: apply clang-format to files mm/kmsan/
@ 2025-05-07 16:00 Alexander Potapenko
  2025-05-07 16:00 ` [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush() Alexander Potapenko
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Alexander Potapenko @ 2025-05-07 16:00 UTC (permalink / raw)
  To: glider
  Cc: elver, dvyukov, bvanassche, kent.overstreet, iii, akpm,
	linux-kernel, kasan-dev

KMSAN source files are expected to be formatted with clang-format, fix
some nits that slipped in. No functional change.

Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/kmsan/core.c   | 4 ++--
 mm/kmsan/hooks.c  | 4 +---
 mm/kmsan/init.c   | 3 +--
 mm/kmsan/shadow.c | 3 +--
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/mm/kmsan/core.c b/mm/kmsan/core.c
index a495debf14363..a97dc90fa6a93 100644
--- a/mm/kmsan/core.c
+++ b/mm/kmsan/core.c
@@ -159,8 +159,8 @@ depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id)
 	 * Make sure we have enough spare bits in @id to hold the UAF bit and
 	 * the chain depth.
 	 */
-	BUILD_BUG_ON(
-		(1 << STACK_DEPOT_EXTRA_BITS) <= (KMSAN_MAX_ORIGIN_DEPTH << 1));
+	BUILD_BUG_ON((1 << STACK_DEPOT_EXTRA_BITS) <=
+		     (KMSAN_MAX_ORIGIN_DEPTH << 1));
 
 	extra_bits = stack_depot_get_extra_bits(id);
 	depth = kmsan_depth_from_eb(extra_bits);
diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
index 3df45c25c1f62..05f2faa540545 100644
--- a/mm/kmsan/hooks.c
+++ b/mm/kmsan/hooks.c
@@ -114,9 +114,7 @@ void kmsan_kfree_large(const void *ptr)
 	kmsan_enter_runtime();
 	page = virt_to_head_page((void *)ptr);
 	KMSAN_WARN_ON(ptr != page_address(page));
-	kmsan_internal_poison_memory((void *)ptr,
-				     page_size(page),
-				     GFP_KERNEL,
+	kmsan_internal_poison_memory((void *)ptr, page_size(page), GFP_KERNEL,
 				     KMSAN_POISON_CHECK | KMSAN_POISON_FREE);
 	kmsan_leave_runtime();
 }
diff --git a/mm/kmsan/init.c b/mm/kmsan/init.c
index 10f52c085e6cd..b14ce3417e65e 100644
--- a/mm/kmsan/init.c
+++ b/mm/kmsan/init.c
@@ -35,8 +35,7 @@ static void __init kmsan_record_future_shadow_range(void *start, void *end)
 	KMSAN_WARN_ON(future_index == NUM_FUTURE_RANGES);
 	KMSAN_WARN_ON((nstart >= nend) ||
 		      /* Virtual address 0 is valid on s390. */
-		      (!IS_ENABLED(CONFIG_S390) && !nstart) ||
-		      !nend);
+		      (!IS_ENABLED(CONFIG_S390) && !nstart) || !nend);
 	nstart = ALIGN_DOWN(nstart, PAGE_SIZE);
 	nend = ALIGN(nend, PAGE_SIZE);
 
diff --git a/mm/kmsan/shadow.c b/mm/kmsan/shadow.c
index 1bb505a08415d..6d32bfc18d6a2 100644
--- a/mm/kmsan/shadow.c
+++ b/mm/kmsan/shadow.c
@@ -207,8 +207,7 @@ void kmsan_free_page(struct page *page, unsigned int order)
 	if (!kmsan_enabled || kmsan_in_runtime())
 		return;
 	kmsan_enter_runtime();
-	kmsan_internal_poison_memory(page_address(page),
-				     page_size(page),
+	kmsan_internal_poison_memory(page_address(page), page_size(page),
 				     GFP_KERNEL,
 				     KMSAN_POISON_CHECK | KMSAN_POISON_FREE);
 	kmsan_leave_runtime();
-- 
2.49.0.967.g6a0df3ecc3-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush()
  2025-05-07 16:00 [PATCH 1/5] kmsan: apply clang-format to files mm/kmsan/ Alexander Potapenko
@ 2025-05-07 16:00 ` Alexander Potapenko
  2025-05-07 16:09   ` Marco Elver
  2025-05-07 16:00 ` [PATCH 3/5] kmsan: drop the declaration of kmsan_save_stack() Alexander Potapenko
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Alexander Potapenko @ 2025-05-07 16:00 UTC (permalink / raw)
  To: glider
  Cc: elver, dvyukov, bvanassche, kent.overstreet, iii, akpm,
	linux-kernel, kasan-dev

Only enter the runtime to call __vmap_pages_range_noflush(), so that error
handling does not skip kmsan_leave_runtime().

This bug was spotted by CONFIG_WARN_CAPABILITY_ANALYSIS=y

Cc: Marco Elver <elver@google.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/kmsan/shadow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/kmsan/shadow.c b/mm/kmsan/shadow.c
index 6d32bfc18d6a2..54f3c3c962f07 100644
--- a/mm/kmsan/shadow.c
+++ b/mm/kmsan/shadow.c
@@ -247,17 +247,19 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
 	kmsan_enter_runtime();
 	mapped = __vmap_pages_range_noflush(shadow_start, shadow_end, prot,
 					    s_pages, page_shift);
+	kmsan_leave_runtime();
 	if (mapped) {
 		err = mapped;
 		goto ret;
 	}
+	kmsan_enter_runtime();
 	mapped = __vmap_pages_range_noflush(origin_start, origin_end, prot,
 					    o_pages, page_shift);
+	kmsan_leave_runtime();
 	if (mapped) {
 		err = mapped;
 		goto ret;
 	}
-	kmsan_leave_runtime();
 	flush_tlb_kernel_range(shadow_start, shadow_end);
 	flush_tlb_kernel_range(origin_start, origin_end);
 	flush_cache_vmap(shadow_start, shadow_end);
-- 
2.49.0.967.g6a0df3ecc3-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/5] kmsan: drop the declaration of kmsan_save_stack()
  2025-05-07 16:00 [PATCH 1/5] kmsan: apply clang-format to files mm/kmsan/ Alexander Potapenko
  2025-05-07 16:00 ` [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush() Alexander Potapenko
@ 2025-05-07 16:00 ` Alexander Potapenko
  2025-05-07 16:09   ` Marco Elver
  2025-05-07 16:00 ` [PATCH 4/5] kmsan: enter the runtime around kmsan_internal_memmove_metadata() call Alexander Potapenko
  2025-05-07 16:00 ` [PATCH 5/5] kmsan: rework kmsan_in_runtime() handling in kmsan_report() Alexander Potapenko
  3 siblings, 1 reply; 11+ messages in thread
From: Alexander Potapenko @ 2025-05-07 16:00 UTC (permalink / raw)
  To: glider
  Cc: elver, dvyukov, bvanassche, kent.overstreet, iii, akpm,
	linux-kernel, kasan-dev

This function is not defined anywhere.

Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/kmsan/kmsan.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/kmsan/kmsan.h b/mm/kmsan/kmsan.h
index 29555a8bc3153..bc3d1810f352c 100644
--- a/mm/kmsan/kmsan.h
+++ b/mm/kmsan/kmsan.h
@@ -121,7 +121,6 @@ static __always_inline void kmsan_leave_runtime(void)
 	KMSAN_WARN_ON(--ctx->kmsan_in_runtime);
 }
 
-depot_stack_handle_t kmsan_save_stack(void);
 depot_stack_handle_t kmsan_save_stack_with_flags(gfp_t flags,
 						 unsigned int extra_bits);
 
-- 
2.49.0.967.g6a0df3ecc3-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/5] kmsan: enter the runtime around kmsan_internal_memmove_metadata() call
  2025-05-07 16:00 [PATCH 1/5] kmsan: apply clang-format to files mm/kmsan/ Alexander Potapenko
  2025-05-07 16:00 ` [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush() Alexander Potapenko
  2025-05-07 16:00 ` [PATCH 3/5] kmsan: drop the declaration of kmsan_save_stack() Alexander Potapenko
@ 2025-05-07 16:00 ` Alexander Potapenko
  2025-05-07 16:09   ` Marco Elver
  2025-05-07 16:00 ` [PATCH 5/5] kmsan: rework kmsan_in_runtime() handling in kmsan_report() Alexander Potapenko
  3 siblings, 1 reply; 11+ messages in thread
From: Alexander Potapenko @ 2025-05-07 16:00 UTC (permalink / raw)
  To: glider
  Cc: elver, dvyukov, bvanassche, kent.overstreet, iii, akpm,
	linux-kernel, kasan-dev

kmsan_internal_memmove_metadata() transitively calls stack_depot_save()
(via kmsan_internal_chain_origin() and kmsan_save_stack_with_flags()),
which may allocate memory. Guard it with kmsan_enter_runtime() and
kmsan_leave_runtime() to avoid recursion.

This bug was spotted by CONFIG_WARN_CAPABILITY_ANALYSIS=y

Cc: Marco Elver <elver@google.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/kmsan/hooks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
index 05f2faa540545..97de3d6194f07 100644
--- a/mm/kmsan/hooks.c
+++ b/mm/kmsan/hooks.c
@@ -275,8 +275,10 @@ void kmsan_copy_to_user(void __user *to, const void *from, size_t to_copy,
 		 * Don't check anything, just copy the shadow of the copied
 		 * bytes.
 		 */
+		kmsan_enter_runtime();
 		kmsan_internal_memmove_metadata((void *)to, (void *)from,
 						to_copy - left);
+		kmsan_leave_runtime();
 	}
 	user_access_restore(ua_flags);
 }
-- 
2.49.0.967.g6a0df3ecc3-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/5] kmsan: rework kmsan_in_runtime() handling in kmsan_report()
  2025-05-07 16:00 [PATCH 1/5] kmsan: apply clang-format to files mm/kmsan/ Alexander Potapenko
                   ` (2 preceding siblings ...)
  2025-05-07 16:00 ` [PATCH 4/5] kmsan: enter the runtime around kmsan_internal_memmove_metadata() call Alexander Potapenko
@ 2025-05-07 16:00 ` Alexander Potapenko
  2025-05-07 16:10   ` Marco Elver
  3 siblings, 1 reply; 11+ messages in thread
From: Alexander Potapenko @ 2025-05-07 16:00 UTC (permalink / raw)
  To: glider
  Cc: elver, dvyukov, bvanassche, kent.overstreet, iii, akpm,
	linux-kernel, kasan-dev

kmsan_report() calls used to require entering/leaving the runtime around
them. To simplify the things, drop this requirement and move calls to
kmsan_enter_runtime()/kmsan_leave_runtime() into kmsan_report().

Cc: Marco Elver <elver@google.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/kmsan/core.c            | 8 --------
 mm/kmsan/instrumentation.c | 4 ----
 mm/kmsan/report.c          | 6 +++---
 3 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/mm/kmsan/core.c b/mm/kmsan/core.c
index a97dc90fa6a93..1ea711786c522 100644
--- a/mm/kmsan/core.c
+++ b/mm/kmsan/core.c
@@ -274,11 +274,9 @@ void kmsan_internal_check_memory(void *addr, size_t size,
 			 * bytes before, report them.
 			 */
 			if (cur_origin) {
-				kmsan_enter_runtime();
 				kmsan_report(cur_origin, addr, size,
 					     cur_off_start, pos - 1, user_addr,
 					     reason);
-				kmsan_leave_runtime();
 			}
 			cur_origin = 0;
 			cur_off_start = -1;
@@ -292,11 +290,9 @@ void kmsan_internal_check_memory(void *addr, size_t size,
 				 * poisoned bytes before, report them.
 				 */
 				if (cur_origin) {
-					kmsan_enter_runtime();
 					kmsan_report(cur_origin, addr, size,
 						     cur_off_start, pos + i - 1,
 						     user_addr, reason);
-					kmsan_leave_runtime();
 				}
 				cur_origin = 0;
 				cur_off_start = -1;
@@ -312,11 +308,9 @@ void kmsan_internal_check_memory(void *addr, size_t size,
 			 */
 			if (cur_origin != new_origin) {
 				if (cur_origin) {
-					kmsan_enter_runtime();
 					kmsan_report(cur_origin, addr, size,
 						     cur_off_start, pos + i - 1,
 						     user_addr, reason);
-					kmsan_leave_runtime();
 				}
 				cur_origin = new_origin;
 				cur_off_start = pos + i;
@@ -326,10 +320,8 @@ void kmsan_internal_check_memory(void *addr, size_t size,
 	}
 	KMSAN_WARN_ON(pos != size);
 	if (cur_origin) {
-		kmsan_enter_runtime();
 		kmsan_report(cur_origin, addr, size, cur_off_start, pos - 1,
 			     user_addr, reason);
-		kmsan_leave_runtime();
 	}
 }
 
diff --git a/mm/kmsan/instrumentation.c b/mm/kmsan/instrumentation.c
index 02a405e55d6ca..69f0a57a401c4 100644
--- a/mm/kmsan/instrumentation.c
+++ b/mm/kmsan/instrumentation.c
@@ -312,13 +312,9 @@ EXPORT_SYMBOL(__msan_unpoison_alloca);
 void __msan_warning(u32 origin);
 void __msan_warning(u32 origin)
 {
-	if (!kmsan_enabled || kmsan_in_runtime())
-		return;
-	kmsan_enter_runtime();
 	kmsan_report(origin, /*address*/ NULL, /*size*/ 0,
 		     /*off_first*/ 0, /*off_last*/ 0, /*user_addr*/ NULL,
 		     REASON_ANY);
-	kmsan_leave_runtime();
 }
 EXPORT_SYMBOL(__msan_warning);
 
diff --git a/mm/kmsan/report.c b/mm/kmsan/report.c
index 94a3303fb65e0..d6853ce089541 100644
--- a/mm/kmsan/report.c
+++ b/mm/kmsan/report.c
@@ -157,14 +157,14 @@ void kmsan_report(depot_stack_handle_t origin, void *address, int size,
 	unsigned long ua_flags;
 	bool is_uaf;
 
-	if (!kmsan_enabled)
+	if (!kmsan_enabled || kmsan_in_runtime())
 		return;
 	if (current->kmsan_ctx.depth)
 		return;
 	if (!origin)
 		return;
 
-	kmsan_disable_current();
+	kmsan_enter_runtime();
 	ua_flags = user_access_save();
 	raw_spin_lock(&kmsan_report_lock);
 	pr_err("=====================================================\n");
@@ -217,5 +217,5 @@ void kmsan_report(depot_stack_handle_t origin, void *address, int size,
 	if (panic_on_kmsan)
 		panic("kmsan.panic set ...\n");
 	user_access_restore(ua_flags);
-	kmsan_enable_current();
+	kmsan_leave_runtime();
 }
-- 
2.49.0.967.g6a0df3ecc3-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush()
  2025-05-07 16:00 ` [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush() Alexander Potapenko
@ 2025-05-07 16:09   ` Marco Elver
  2025-05-08  8:19     ` Alexander Potapenko
  0 siblings, 1 reply; 11+ messages in thread
From: Marco Elver @ 2025-05-07 16:09 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: dvyukov, bvanassche, kent.overstreet, iii, akpm, linux-kernel,
	kasan-dev

On Wed, 7 May 2025 at 18:00, Alexander Potapenko <glider@google.com> wrote:
>
> Only enter the runtime to call __vmap_pages_range_noflush(), so that error
> handling does not skip kmsan_leave_runtime().
>
> This bug was spotted by CONFIG_WARN_CAPABILITY_ANALYSIS=y

Might be worth pointing out this is not yet upstream:
https://lore.kernel.org/all/20250304092417.2873893-1-elver@google.com/

Also, for future reference, feel free to dump the diff here that added
the annotations that helped you find the missing kmsan*runtime()
calls. I'm sure it'd be of interest to others. At one point we may
upstream those annotations, too, but we'll need Capability Analysis
upstream first (which is blocked by some Clang improvements that were
requested).

> Cc: Marco Elver <elver@google.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Acked-by: Marco Elver <elver@google.com>

> ---
>  mm/kmsan/shadow.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/kmsan/shadow.c b/mm/kmsan/shadow.c
> index 6d32bfc18d6a2..54f3c3c962f07 100644
> --- a/mm/kmsan/shadow.c
> +++ b/mm/kmsan/shadow.c
> @@ -247,17 +247,19 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
>         kmsan_enter_runtime();
>         mapped = __vmap_pages_range_noflush(shadow_start, shadow_end, prot,
>                                             s_pages, page_shift);
> +       kmsan_leave_runtime();
>         if (mapped) {
>                 err = mapped;
>                 goto ret;
>         }
> +       kmsan_enter_runtime();
>         mapped = __vmap_pages_range_noflush(origin_start, origin_end, prot,
>                                             o_pages, page_shift);
> +       kmsan_leave_runtime();
>         if (mapped) {
>                 err = mapped;
>                 goto ret;
>         }
> -       kmsan_leave_runtime();
>         flush_tlb_kernel_range(shadow_start, shadow_end);
>         flush_tlb_kernel_range(origin_start, origin_end);
>         flush_cache_vmap(shadow_start, shadow_end);
> --
> 2.49.0.967.g6a0df3ecc3-goog
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/5] kmsan: drop the declaration of kmsan_save_stack()
  2025-05-07 16:00 ` [PATCH 3/5] kmsan: drop the declaration of kmsan_save_stack() Alexander Potapenko
@ 2025-05-07 16:09   ` Marco Elver
  0 siblings, 0 replies; 11+ messages in thread
From: Marco Elver @ 2025-05-07 16:09 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: dvyukov, bvanassche, kent.overstreet, iii, akpm, linux-kernel,
	kasan-dev

On Wed, 7 May 2025 at 18:00, Alexander Potapenko <glider@google.com> wrote:
>
> This function is not defined anywhere.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Acked-by: Marco Elver <elver@google.com>

> ---
>  mm/kmsan/kmsan.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/kmsan/kmsan.h b/mm/kmsan/kmsan.h
> index 29555a8bc3153..bc3d1810f352c 100644
> --- a/mm/kmsan/kmsan.h
> +++ b/mm/kmsan/kmsan.h
> @@ -121,7 +121,6 @@ static __always_inline void kmsan_leave_runtime(void)
>         KMSAN_WARN_ON(--ctx->kmsan_in_runtime);
>  }
>
> -depot_stack_handle_t kmsan_save_stack(void);
>  depot_stack_handle_t kmsan_save_stack_with_flags(gfp_t flags,
>                                                  unsigned int extra_bits);
>
> --
> 2.49.0.967.g6a0df3ecc3-goog
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/5] kmsan: enter the runtime around kmsan_internal_memmove_metadata() call
  2025-05-07 16:00 ` [PATCH 4/5] kmsan: enter the runtime around kmsan_internal_memmove_metadata() call Alexander Potapenko
@ 2025-05-07 16:09   ` Marco Elver
  0 siblings, 0 replies; 11+ messages in thread
From: Marco Elver @ 2025-05-07 16:09 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: dvyukov, bvanassche, kent.overstreet, iii, akpm, linux-kernel,
	kasan-dev

On Wed, 7 May 2025 at 18:00, Alexander Potapenko <glider@google.com> wrote:
>
> kmsan_internal_memmove_metadata() transitively calls stack_depot_save()
> (via kmsan_internal_chain_origin() and kmsan_save_stack_with_flags()),
> which may allocate memory. Guard it with kmsan_enter_runtime() and
> kmsan_leave_runtime() to avoid recursion.
>
> This bug was spotted by CONFIG_WARN_CAPABILITY_ANALYSIS=y
>
> Cc: Marco Elver <elver@google.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Acked-by: Marco Elver <elver@google.com>

> ---
>  mm/kmsan/hooks.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
> index 05f2faa540545..97de3d6194f07 100644
> --- a/mm/kmsan/hooks.c
> +++ b/mm/kmsan/hooks.c
> @@ -275,8 +275,10 @@ void kmsan_copy_to_user(void __user *to, const void *from, size_t to_copy,
>                  * Don't check anything, just copy the shadow of the copied
>                  * bytes.
>                  */
> +               kmsan_enter_runtime();
>                 kmsan_internal_memmove_metadata((void *)to, (void *)from,
>                                                 to_copy - left);
> +               kmsan_leave_runtime();
>         }
>         user_access_restore(ua_flags);
>  }
> --
> 2.49.0.967.g6a0df3ecc3-goog
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/5] kmsan: rework kmsan_in_runtime() handling in kmsan_report()
  2025-05-07 16:00 ` [PATCH 5/5] kmsan: rework kmsan_in_runtime() handling in kmsan_report() Alexander Potapenko
@ 2025-05-07 16:10   ` Marco Elver
  0 siblings, 0 replies; 11+ messages in thread
From: Marco Elver @ 2025-05-07 16:10 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: dvyukov, bvanassche, kent.overstreet, iii, akpm, linux-kernel,
	kasan-dev

On Wed, 7 May 2025 at 18:00, Alexander Potapenko <glider@google.com> wrote:
>
> kmsan_report() calls used to require entering/leaving the runtime around
> them. To simplify the things, drop this requirement and move calls to
> kmsan_enter_runtime()/kmsan_leave_runtime() into kmsan_report().
>
> Cc: Marco Elver <elver@google.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Acked-by: Marco Elver <elver@google.com>

> ---
>  mm/kmsan/core.c            | 8 --------
>  mm/kmsan/instrumentation.c | 4 ----
>  mm/kmsan/report.c          | 6 +++---
>  3 files changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/mm/kmsan/core.c b/mm/kmsan/core.c
> index a97dc90fa6a93..1ea711786c522 100644
> --- a/mm/kmsan/core.c
> +++ b/mm/kmsan/core.c
> @@ -274,11 +274,9 @@ void kmsan_internal_check_memory(void *addr, size_t size,
>                          * bytes before, report them.
>                          */
>                         if (cur_origin) {
> -                               kmsan_enter_runtime();
>                                 kmsan_report(cur_origin, addr, size,
>                                              cur_off_start, pos - 1, user_addr,
>                                              reason);
> -                               kmsan_leave_runtime();
>                         }
>                         cur_origin = 0;
>                         cur_off_start = -1;
> @@ -292,11 +290,9 @@ void kmsan_internal_check_memory(void *addr, size_t size,
>                                  * poisoned bytes before, report them.
>                                  */
>                                 if (cur_origin) {
> -                                       kmsan_enter_runtime();
>                                         kmsan_report(cur_origin, addr, size,
>                                                      cur_off_start, pos + i - 1,
>                                                      user_addr, reason);
> -                                       kmsan_leave_runtime();
>                                 }
>                                 cur_origin = 0;
>                                 cur_off_start = -1;
> @@ -312,11 +308,9 @@ void kmsan_internal_check_memory(void *addr, size_t size,
>                          */
>                         if (cur_origin != new_origin) {
>                                 if (cur_origin) {
> -                                       kmsan_enter_runtime();
>                                         kmsan_report(cur_origin, addr, size,
>                                                      cur_off_start, pos + i - 1,
>                                                      user_addr, reason);
> -                                       kmsan_leave_runtime();
>                                 }
>                                 cur_origin = new_origin;
>                                 cur_off_start = pos + i;
> @@ -326,10 +320,8 @@ void kmsan_internal_check_memory(void *addr, size_t size,
>         }
>         KMSAN_WARN_ON(pos != size);
>         if (cur_origin) {
> -               kmsan_enter_runtime();
>                 kmsan_report(cur_origin, addr, size, cur_off_start, pos - 1,
>                              user_addr, reason);
> -               kmsan_leave_runtime();
>         }
>  }
>
> diff --git a/mm/kmsan/instrumentation.c b/mm/kmsan/instrumentation.c
> index 02a405e55d6ca..69f0a57a401c4 100644
> --- a/mm/kmsan/instrumentation.c
> +++ b/mm/kmsan/instrumentation.c
> @@ -312,13 +312,9 @@ EXPORT_SYMBOL(__msan_unpoison_alloca);
>  void __msan_warning(u32 origin);
>  void __msan_warning(u32 origin)
>  {
> -       if (!kmsan_enabled || kmsan_in_runtime())
> -               return;
> -       kmsan_enter_runtime();
>         kmsan_report(origin, /*address*/ NULL, /*size*/ 0,
>                      /*off_first*/ 0, /*off_last*/ 0, /*user_addr*/ NULL,
>                      REASON_ANY);
> -       kmsan_leave_runtime();
>  }
>  EXPORT_SYMBOL(__msan_warning);
>
> diff --git a/mm/kmsan/report.c b/mm/kmsan/report.c
> index 94a3303fb65e0..d6853ce089541 100644
> --- a/mm/kmsan/report.c
> +++ b/mm/kmsan/report.c
> @@ -157,14 +157,14 @@ void kmsan_report(depot_stack_handle_t origin, void *address, int size,
>         unsigned long ua_flags;
>         bool is_uaf;
>
> -       if (!kmsan_enabled)
> +       if (!kmsan_enabled || kmsan_in_runtime())
>                 return;
>         if (current->kmsan_ctx.depth)
>                 return;
>         if (!origin)
>                 return;
>
> -       kmsan_disable_current();
> +       kmsan_enter_runtime();
>         ua_flags = user_access_save();
>         raw_spin_lock(&kmsan_report_lock);
>         pr_err("=====================================================\n");
> @@ -217,5 +217,5 @@ void kmsan_report(depot_stack_handle_t origin, void *address, int size,
>         if (panic_on_kmsan)
>                 panic("kmsan.panic set ...\n");
>         user_access_restore(ua_flags);
> -       kmsan_enable_current();
> +       kmsan_leave_runtime();
>  }
> --
> 2.49.0.967.g6a0df3ecc3-goog
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush()
  2025-05-07 16:09   ` Marco Elver
@ 2025-05-08  8:19     ` Alexander Potapenko
  2025-05-08  8:31       ` Alexander Potapenko
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Potapenko @ 2025-05-08  8:19 UTC (permalink / raw)
  To: Marco Elver
  Cc: dvyukov, bvanassche, kent.overstreet, iii, akpm, linux-kernel,
	kasan-dev

On Wed, May 7, 2025 at 6:09 PM Marco Elver <elver@google.com> wrote:
>
> On Wed, 7 May 2025 at 18:00, Alexander Potapenko <glider@google.com> wrote:
> >
> > Only enter the runtime to call __vmap_pages_range_noflush(), so that error
> > handling does not skip kmsan_leave_runtime().
> >
> > This bug was spotted by CONFIG_WARN_CAPABILITY_ANALYSIS=y
>
> Might be worth pointing out this is not yet upstream:
> https://lore.kernel.org/all/20250304092417.2873893-1-elver@google.com/

Thanks! I'll update the description (here and in the other patch) and
post v2 later today.

> Also, for future reference, feel free to dump the diff here that added
> the annotations that helped you find the missing kmsan*runtime()
> calls. I'm sure it'd be of interest to others. At one point we may
> upstream those annotations, too, but we'll need Capability Analysis
> upstream first (which is blocked by some Clang improvements that were
> requested).

The diff is below. I added a __no_matter() macro which isn't strictly
necessary (maybe we can remove it altogether), but I thought it'll be
more descriptive.

Author: Alexander Potapenko <glider@google.com>
Date:   Thu Apr 3 15:44:38 2025 +0200

    DO-NOT-SUBMIT: kmsan: enable capability analysis

    Add support for the new capability analysis framework to KMSAN.
    Use the KMSAN_RUNTIME capability token to ensure correctness of
    kmsan_enter_runtime()/kmsan_leave_runtime() usage.

    Cc: Marco Elver <elver@google.com>
    Cc: Bart Van Assche <bvanassche@acm.org>
    Cc: Kent Overstreet <kent.overstreet@linux.dev>
    Signed-off-by: Alexander Potapenko <glider@google.com>

diff --git a/mm/kmsan/Makefile b/mm/kmsan/Makefile
index 91cfdde642d16..94591d612384c 100644
--- a/mm/kmsan/Makefile
+++ b/mm/kmsan/Makefile
@@ -8,6 +8,7 @@ obj-y := core.o instrumentation.o init.o hooks.o
report.o shadow.o
 KMSAN_SANITIZE := n
 KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
+CAPABILITY_ANALYSIS := y

 # Disable instrumentation of KMSAN runtime with other tools.
 CC_FLAGS_KMSAN_RUNTIME := -fno-stack-protector
diff --git a/mm/kmsan/kmsan.h b/mm/kmsan/kmsan.h
index bc3d1810f352c..441c9dd39fe2a 100644
--- a/mm/kmsan/kmsan.h
+++ b/mm/kmsan/kmsan.h
@@ -35,6 +35,9 @@
 #define KMSAN_META_SHADOW (false)
 #define KMSAN_META_ORIGIN (true)

+token_capability(KMSAN_RUNTIME);
+#define __no_matter(X)
+
 /*
  * A pair of metadata pointers to be returned by the instrumentation functions.
  */
@@ -74,7 +77,7 @@ void kmsan_print_origin(depot_stack_handle_t origin);
  */
 void kmsan_report(depot_stack_handle_t origin, void *address, int size,
                  int off_first, int off_last, const void __user *user_addr,
-                 enum kmsan_bug_reason reason);
+                 enum kmsan_bug_reason reason) __must_not_hold(KMSAN_RUNTIME);

 DECLARE_PER_CPU(struct kmsan_ctx, kmsan_percpu_ctx);

@@ -107,6 +110,7 @@ static __always_inline bool kmsan_in_runtime(void)
 }

 static __always_inline void kmsan_enter_runtime(void)
+       __acquires(KMSAN_RUNTIME) __no_capability_analysis
 {
        struct kmsan_ctx *ctx;

@@ -115,6 +119,7 @@ static __always_inline void kmsan_enter_runtime(void)
 }

 static __always_inline void kmsan_leave_runtime(void)
+       __releases(KMSAN_RUNTIME) __no_capability_analysis
 {
        struct kmsan_ctx *ctx = kmsan_get_context();

@@ -122,7 +127,8 @@ static __always_inline void kmsan_leave_runtime(void)
 }

 depot_stack_handle_t kmsan_save_stack_with_flags(gfp_t flags,
-                                                unsigned int extra_bits);
+                                                unsigned int extra_bits)
+       __must_hold(KMSAN_RUNTIME);

 /*
  * Pack and unpack the origin chain depth and UAF flag to/from the extra bits
@@ -151,19 +157,26 @@ static __always_inline unsigned int
kmsan_depth_from_eb(unsigned int extra_bits)
  * kmsan_internal_ functions are supposed to be very simple and not require the
  * kmsan_in_runtime() checks.
  */
-void kmsan_internal_memmove_metadata(void *dst, void *src, size_t n);
+void kmsan_internal_memmove_metadata(void *dst, void *src, size_t n)
+       __must_hold(KMSAN_RUNTIME);
 void kmsan_internal_poison_memory(void *address, size_t size, gfp_t flags,
-                                 unsigned int poison_flags);
-void kmsan_internal_unpoison_memory(void *address, size_t size, bool checked);
+                                 unsigned int poison_flags)
+       __must_hold(KMSAN_RUNTIME);
+void kmsan_internal_unpoison_memory(void *address, size_t size, bool checked)
+       __no_matter(KMSAN_RUNTIME);
+
 void kmsan_internal_set_shadow_origin(void *address, size_t size, int b,
-                                     u32 origin, bool checked);
-depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id);
+                                     u32 origin, bool checked)
+       __no_matter(KMSAN_RUNTIME);
+depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id)
+       __must_hold(KMSAN_RUNTIME);

 void kmsan_internal_task_create(struct task_struct *task);

 bool kmsan_metadata_is_contiguous(void *addr, size_t size);
 void kmsan_internal_check_memory(void *addr, size_t size,
-                                const void __user *user_addr, int reason);
+                                const void __user *user_addr, int reason)
+       __must_not_hold(KMSAN_RUNTIME);

 struct page *kmsan_vmalloc_to_page_or_null(void *vaddr);
 void kmsan_setup_meta(struct page *page, struct page *shadow,

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush()
  2025-05-08  8:19     ` Alexander Potapenko
@ 2025-05-08  8:31       ` Alexander Potapenko
  0 siblings, 0 replies; 11+ messages in thread
From: Alexander Potapenko @ 2025-05-08  8:31 UTC (permalink / raw)
  To: Marco Elver
  Cc: dvyukov, bvanassche, kent.overstreet, iii, akpm, linux-kernel,
	kasan-dev

On Thu, May 8, 2025 at 10:19 AM Alexander Potapenko <glider@google.com> wrote:
>
> On Wed, May 7, 2025 at 6:09 PM Marco Elver <elver@google.com> wrote:
> >
> > On Wed, 7 May 2025 at 18:00, Alexander Potapenko <glider@google.com> wrote:
> > >
> > > Only enter the runtime to call __vmap_pages_range_noflush(), so that error
> > > handling does not skip kmsan_leave_runtime().
> > >
> > > This bug was spotted by CONFIG_WARN_CAPABILITY_ANALYSIS=y
> >
> > Might be worth pointing out this is not yet upstream:
> > https://lore.kernel.org/all/20250304092417.2873893-1-elver@google.com/
>
> Thanks! I'll update the description (here and in the other patch) and
> post v2 later today.

Since Andrew picked the changes up already, we've decided there's no
need for a respin :)

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-05-08  8:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-07 16:00 [PATCH 1/5] kmsan: apply clang-format to files mm/kmsan/ Alexander Potapenko
2025-05-07 16:00 ` [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush() Alexander Potapenko
2025-05-07 16:09   ` Marco Elver
2025-05-08  8:19     ` Alexander Potapenko
2025-05-08  8:31       ` Alexander Potapenko
2025-05-07 16:00 ` [PATCH 3/5] kmsan: drop the declaration of kmsan_save_stack() Alexander Potapenko
2025-05-07 16:09   ` Marco Elver
2025-05-07 16:00 ` [PATCH 4/5] kmsan: enter the runtime around kmsan_internal_memmove_metadata() call Alexander Potapenko
2025-05-07 16:09   ` Marco Elver
2025-05-07 16:00 ` [PATCH 5/5] kmsan: rework kmsan_in_runtime() handling in kmsan_report() Alexander Potapenko
2025-05-07 16:10   ` Marco Elver

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox