Re: [PATCH v2] workqueue: Don't record workqueue stack holding raw_spin_lock
From: Shuah Khan
Date: Thu Sep 02 2021 - 19:46:22 EST
On 9/2/21 3:58 PM, Marco Elver wrote:
On Thu, 2 Sept 2021 at 22:01, Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> wrote:
When CONFIG_PROVE_RAW_LOCK_NESTING=y and CONFIG_KASAN are enabled,
kasan_record_aux_stack() runs into "BUG: Invalid wait context" when
it tries to allocate memory attempting to acquire spinlock in page
allocation code while holding workqueue pool raw_spinlock.
[snip]
Fix it by calling kasan_record_aux_stack() conditionally only when
CONFIG_PROVE_RAW_LOCK_NESTING is not enabled. After exploring other
options such as calling kasan_record_aux_stack() after releasing the
pool lock, opting for a least disruptive path of stubbing this record
function to avoid nesting raw spinlock and spinlock.
[snip]
Fixes: e89a85d63fb2 ("workqueue: kasan: record workqueue stack")
Signed-off-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
---
Changes since v1:
-- Instead of changing when record happens, disable record
when CONFIG_PROVE_RAW_LOCK_NESTING=y
kernel/workqueue.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f148eacda55a..435970ef81ae 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1328,8 +1328,16 @@ static void insert_work(struct pool_workqueue *pwq, struct work_struct *work,
{
struct worker_pool *pool = pwq->pool;
- /* record the work call stack in order to print it in KASAN reports */
+ /*
+ * record the work call stack in order to print it in KASAN reports
+ * Doing this when CONFIG_PROVE_RAW_LOCK_NESTING is enabled results
+ * in nesting raw spinlock with page allocation spinlock.
+ *
+ * Avoid recording when CONFIG_PROVE_RAW_LOCK_NESTING is enabled.
+ */
+#if !defined(CONFIG_PROVE_RAW_LOCK_NESTING)
Just "if (!IS_ENABLED(CONFIG_PROVE_RAW_LOCK_NESTING))" should work
here, however...
Yes. That would work.
... PROVE_RAW_LOCK_NESTING exists for PREEMPT_RT's benefit. I don't
think silencing the debugging tool is the solution, because the bug
still exists in a PREEMPT_RT kernel.
This silencing is limited in scope to just the insert_work() and when
PROVE_RAW_LOCK_NESTING is enabled. Please see below under your proposed
option 2
+Cc Sebastian for advice. I may have missed something obvious. :-)
Thanks for adding Sebastian
I have a suspicion that kasan_record_aux_stack() (via
stack_depot_save()) is generally unsound on PREEMPT_RT kernels,
because allocating memory cannot be done within raw-locked critical
sections because memory allocation is preemptible on RT. Even using
GWP_NOWAIT/ATOMIC doesn't help (which kasan_record_aux_stack() uses).
It follows that if we do not know what type of locks may be held when
calling kasan_record_aux_stack() we have a bug in RT.
I see 3 options:
1. Try to move kasan_record_aux_stack() where no raw lock is held.
(Seems complicated per v1 attempt?)
Yes. kasan_record_aux_stack() is better called from insert_work()
prior to insertion. This makes it difficult to do - we don't want
to release the pool lock.
But ideally we make kasan_record_aux_stack() more robust on RT:
2. Make kasan_record_aux_stack() a no-op on RT (and if
PROVE_RAW_LOCK_NESTING). Perhaps overkill?
I considered it and didn't go down that route because it is a big
hammer. I choose to just disable the debug code in insert_work()
path instead. Not ideal, but limits the disable to a narrower
scope. Limiting the scope in kasan_record_aux_stack() extends to
all other paths where kasan_record_aux_stack() is used.
3. Try to not allocate memory in stackdepot. Not sure this is feasible
without telling stackdepot to preallocate the max slabs on boot if RT.
We could. I have to ask though how much of the real world cases do we
need to impact for the debug code to work?
Anything else? Because I don't think any of the options are satisfying.
One option to consider is checking dry-run invalid nesting check and
bail out if it is true in kasan_record_aux_stack()
thanks,
-- Shuah