padata: Is padata_find_next() thread-safe?

Posted by Tetsuo Handa 1 month, 2 weeks ago

syzbot is reporting possibility of recursive locking at
https://syzkaller.appspot.com/bug?extid=bd936ccd4339cea66e6b .
If this is a false positive report, the fix will be as simple as

--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -253,7 +253,7 @@ static struct padata_priv *padata_find_next(struct parallel_data *pd, int cpu,
 
 	reorder = per_cpu_ptr(pd->reorder_list, cpu);
 
-	spin_lock(&reorder->lock);
+	spin_lock_nested(&reorder->lock, 1);
 	if (list_empty(&reorder->list))
 		goto notfound;

. But I don't know if there is a possibility of AB-BA deadlock.

Can a sequence shown below possible? If it is possible, how is calling
padata_find_next() with a spinlock already held by caller introduced by
commit 71203f68c774 ("padata: Fix pd UAF once and for all") thread-safe?

  struct padata_list list[2];

  CPU 0:                         CPU 1:

  spin_lock(&list[0].lock);
                                 spin_lock(&list[1].lock);
  spin_lock_nested(&list[1].lock, 1);
                                 spin_lock_nested(&list[0].lock, 1);
  do_something();
                                 do_something();
  spin_unlock(&list[1].lock);
                                 spin_unlock(&list[0].lock);
  spin_unlock(&list[0].lock);
                                 spin_unlock(&list[1].lock);

Re: padata: Is padata_find_next() thread-safe?

Posted by Herbert Xu 1 month, 1 week ago

On Tue, Nov 04, 2025 at 08:44:53PM +0900, Tetsuo Handa wrote:
> syzbot is reporting possibility of recursive locking at
> https://syzkaller.appspot.com/bug?extid=bd936ccd4339cea66e6b .
> If this is a false positive report, the fix will be as simple as

Yes it's a false positive as reorder->lock is never the same as
squeue->serial.lock.

However, they both have the same data type which is why lockdep
is confused.

Please provide a patch that sets the class for one of them to
something different.  For example, change the lockdep class for
reorder->lock using lockdep_set_class and the problem should go
away.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: padata: Is padata_find_next() thread-safe?

Posted by Tetsuo Handa 1 month, 1 week ago

On 2025/11/06 18:28, Herbert Xu wrote:
> On Tue, Nov 04, 2025 at 08:44:53PM +0900, Tetsuo Handa wrote:
>> syzbot is reporting possibility of recursive locking at
>> https://syzkaller.appspot.com/bug?extid=bd936ccd4339cea66e6b .
>> If this is a false positive report, the fix will be as simple as
> 
> Yes it's a false positive as reorder->lock is never the same as
> squeue->serial.lock.

OK. But what is about the "Can a sequence shown below possible?" part?

> 
> However, they both have the same data type which is why lockdep
> is confused.
> 
> Please provide a patch that sets the class for one of them to
> something different.  For example, change the lockdep class for
> reorder->lock using lockdep_set_class and the problem should go
> away.

Is using _nested(lock, 1) wrong?

Re: padata: Is padata_find_next() thread-safe?

Posted by Herbert Xu 1 month, 1 week ago

On Thu, Nov 06, 2025 at 06:41:19PM +0900, Tetsuo Handa wrote:
>
> OK. But what is about the "Can a sequence shown below possible?" part?

It's a false positive, i.e., it cannot dead-lock as shown.

> Is using _nested(lock, 1) wrong?

That should kill this particular warning but there could still be
similar warnings if you treat these two locks as the same class.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] padata: use different lock_class_key for padata_list lock

Posted by Tetsuo Handa 1 month, 1 week ago

syzbot is reporting possibility of deadlock due to calling spin_lock_init()
in __padata_list_init() from both padata_init_reorder_list() and
padata_init_squeues(). This is a false positive, for reorder->lock is never
the same as squeue->serial.lock.

Reported-by: syzbot+bd936ccd4339cea66e6b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=bd936ccd4339cea66e6b
Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/padata.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/padata.c b/kernel/padata.c
index f4def028c48c..465d25020e1e 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -529,12 +529,14 @@ static void padata_init_squeues(struct parallel_data *pd)
 /* Initialize per-CPU reorder lists */
 static void padata_init_reorder_list(struct parallel_data *pd)
 {
+	static struct lock_class_key padata_list_key;
 	int cpu;
 	struct padata_list *list;
 
 	for_each_cpu(cpu, pd->cpumask.pcpu) {
 		list = per_cpu_ptr(pd->reorder_list, cpu);
 		__padata_list_init(list);
+		lockdep_set_class(&list->lock, &padata_list_key);
 	}
 }
 
-- 
2.47.3

[PATCH v2] padata: remove __padata_list_init()

Posted by Tetsuo Handa 1 month, 1 week ago

syzbot is reporting possibility of deadlock due to sharing lock_class_key
between padata_init_squeues() and padata_init_reorder_list(). This is a
false positive, for these callers initialize different object. Unshare
lock_class_key by embedding __padata_list_init() into these callers.

Reported-by: syzbot+bd936ccd4339cea66e6b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=bd936ccd4339cea66e6b
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
This version might be easier to understand, for __padata_list_init() is
cheap (which is likely inlined by compiler) and has only two callers.

 kernel/padata.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index f4def028c48c..aa66d91e20f9 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -506,12 +506,6 @@ void __init padata_do_multithreaded(struct padata_mt_job *job)
 	padata_works_free(&works);
 }
 
-static void __padata_list_init(struct padata_list *pd_list)
-{
-	INIT_LIST_HEAD(&pd_list->list);
-	spin_lock_init(&pd_list->lock);
-}
-
 /* Initialize all percpu queues used by serial workers */
 static void padata_init_squeues(struct parallel_data *pd)
 {
@@ -521,7 +515,8 @@ static void padata_init_squeues(struct parallel_data *pd)
 	for_each_cpu(cpu, pd->cpumask.cbcpu) {
 		squeue = per_cpu_ptr(pd->squeue, cpu);
 		squeue->pd = pd;
-		__padata_list_init(&squeue->serial);
+		INIT_LIST_HEAD(&squeue->serial.list);
+		spin_lock_init(&squeue->serial.lock);
 		INIT_WORK(&squeue->work, padata_serial_worker);
 	}
 }
@@ -534,7 +529,8 @@ static void padata_init_reorder_list(struct parallel_data *pd)
 
 	for_each_cpu(cpu, pd->cpumask.pcpu) {
 		list = per_cpu_ptr(pd->reorder_list, cpu);
-		__padata_list_init(list);
+		INIT_LIST_HEAD(&list->list);
+		spin_lock_init(&list->lock);
 	}
 }
 
-- 
2.47.3

Re: [PATCH v2] padata: remove __padata_list_init()

Posted by Herbert Xu 1 month ago

On Fri, Nov 07, 2025 at 11:49:37PM +0900, Tetsuo Handa wrote:
> syzbot is reporting possibility of deadlock due to sharing lock_class_key
> between padata_init_squeues() and padata_init_reorder_list(). This is a
> false positive, for these callers initialize different object. Unshare
> lock_class_key by embedding __padata_list_init() into these callers.
> 
> Reported-by: syzbot+bd936ccd4339cea66e6b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=bd936ccd4339cea66e6b
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> This version might be easier to understand, for __padata_list_init() is
> cheap (which is likely inlined by compiler) and has only two callers.
> 
>  kernel/padata.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

This looks much nicer.

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt