[PATCH] stackdepot: fix stack_depot_save_flags() in NMI context

[PATCH] stackdepot: fix stack_depot_save_flags() in NMI context

[Marco Elver]

Per documentation, stack_depot_save_flags() was meant to be usable from
NMI context if STACK_DEPOT_FLAG_CAN_ALLOC is unset. However, it still
would try to take the pool_lock in an attempt to save a stack trace in
the current pool (if space is available).

This could result in deadlock if an NMI is handled while pool_lock is
already held. To avoid deadlock, only try to take the lock in NMI
context and give up if unsuccessful.

The documentation is fixed to clearly convey this.

Link: https://lkml.kernel.org/r/Z0CcyfbPqmxJ9uJH@elver.google.com
Fixes: 4434a56ec209 ("stackdepot: make fast paths lock-less again")
Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Marco Elver <elver@google.com>
---
 include/linux/stackdepot.h |  6 +++---
 lib/stackdepot.c           | 10 +++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index e9ec32fb97d4..2cc21ffcdaf9 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -147,7 +147,7 @@ static inline int stack_depot_early_init(void)	{ return 0; }
  * If the provided stack trace comes from the interrupt context, only the part
  * up to the interrupt entry is saved.
  *
- * Context: Any context, but setting STACK_DEPOT_FLAG_CAN_ALLOC is required if
+ * Context: Any context, but unsetting STACK_DEPOT_FLAG_CAN_ALLOC is required if
  *          alloc_pages() cannot be used from the current context. Currently
  *          this is the case for contexts where neither %GFP_ATOMIC nor
  *          %GFP_NOWAIT can be used (NMI, raw_spin_lock).
@@ -156,7 +156,7 @@ static inline int stack_depot_early_init(void)	{ return 0; }
  */
 depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
 					    unsigned int nr_entries,
-					    gfp_t gfp_flags,
+					    gfp_t alloc_flags,
 					    depot_flags_t depot_flags);
 
 /**
@@ -175,7 +175,7 @@ depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
  * Return: Handle of the stack trace stored in depot, 0 on failure
  */
 depot_stack_handle_t stack_depot_save(unsigned long *entries,
-				      unsigned int nr_entries, gfp_t gfp_flags);
+				      unsigned int nr_entries, gfp_t alloc_flags);
 
 /**
  * __stack_depot_get_stack_record - Get a pointer to a stack_record struct
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 5ed34cc963fc..245d5b416699 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -630,7 +630,15 @@ depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
 			prealloc = page_address(page);
 	}
 
-	raw_spin_lock_irqsave(&pool_lock, flags);
+	if (in_nmi()) {
+		/* We can never allocate in NMI context. */
+		WARN_ON_ONCE(can_alloc);
+		/* Best effort; bail if we fail to take the lock. */
+		if (!raw_spin_trylock_irqsave(&pool_lock, flags))
+			goto exit;
+	} else {
+		raw_spin_lock_irqsave(&pool_lock, flags);
+	}
 	printk_deferred_enter();
 
 	/* Try to find again, to avoid concurrently inserting duplicates. */
-- 
2.47.0.371.ga323438b13-goog

Re: [PATCH] stackdepot: fix stack_depot_save_flags() in NMI context

[Sebastian Andrzej Siewior]

On 2024-11-22 16:39:47 [+0100], Marco Elver wrote:
> Per documentation, stack_depot_save_flags() was meant to be usable from
> NMI context if STACK_DEPOT_FLAG_CAN_ALLOC is unset. However, it still
> would try to take the pool_lock in an attempt to save a stack trace in
> the current pool (if space is available).
> 
> This could result in deadlock if an NMI is handled while pool_lock is
> already held. To avoid deadlock, only try to take the lock in NMI
> context and give up if unsuccessful.
> 
> The documentation is fixed to clearly convey this.
> 
> Link: https://lkml.kernel.org/r/Z0CcyfbPqmxJ9uJH@elver.google.com
> Fixes: 4434a56ec209 ("stackdepot: make fast paths lock-less again")
> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Marco Elver <elver@google.com>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

Re: [PATCH] stackdepot: fix stack_depot_save_flags() in NMI context

[Andrew Morton]

On Fri, 22 Nov 2024 16:39:47 +0100 Marco Elver <elver@google.com> wrote:

> Per documentation, stack_depot_save_flags() was meant to be usable from
> NMI context if STACK_DEPOT_FLAG_CAN_ALLOC is unset. However, it still
> would try to take the pool_lock in an attempt to save a stack trace in
> the current pool (if space is available).
> 
> This could result in deadlock if an NMI is handled while pool_lock is
> already held. To avoid deadlock, only try to take the lock in NMI
> context and give up if unsuccessful.

Is it possible to trigger this deadlock in current kernels, or is this
a might-happen-in-the-future thing?

> The documentation is fixed to clearly convey this.
> 

Re: [PATCH] stackdepot: fix stack_depot_save_flags() in NMI context

[Marco Elver]

On Thu, 28 Nov 2024 at 02:25, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 22 Nov 2024 16:39:47 +0100 Marco Elver <elver@google.com> wrote:
>
> > Per documentation, stack_depot_save_flags() was meant to be usable from
> > NMI context if STACK_DEPOT_FLAG_CAN_ALLOC is unset. However, it still
> > would try to take the pool_lock in an attempt to save a stack trace in
> > the current pool (if space is available).
> >
> > This could result in deadlock if an NMI is handled while pool_lock is
> > already held. To avoid deadlock, only try to take the lock in NMI
> > context and give up if unsuccessful.
>
> Is it possible to trigger this deadlock in current kernels, or is this
> a might-happen-in-the-future thing?

I can't find evidence this happens right now (at least with the quick
test I just ran), so it's more of a might happen if use of this API
broadens.

Thanks,
-- Marco