Re: [PATCH] padata: fix lockdep warning in padata_find_next()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] padata: redundant spin_lock_nested() in padata_find_next() splits lock class
--

commit d8de136037d32da1b6bd4096b78d163c014e8d23
Author: Tristan Madani <tristmd@gmail.com>

padata: fix lockdep warning in padata_find_next()

This commit replaces spin_lock() with spin_lock_nested() in
padata_find_next() to annotate acquiring reorder->lock while
padata_reorder() holds squeue->serial.lock.

> diff --git a/kernel/padata.c b/kernel/padata.c
> index 5a113c667d4cb..94d7880a001c4 100644
> --- 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, SINGLE_DEPTH_NESTING);

[Severity: Medium]
Is this spin_lock_nested() annotation still needed here, and does it split
reorder->lock across two lockdep subclasses?

In Linux, spin_lock_init() assigns a lock_class_key per macro expansion site
rather than per C struct type. Historically, squeue->serial.lock and
reorder->lock shared a lockdep class because both were initialized via a
common helper __padata_list_init() (and padata_find_next() was moved inside
squeue->serial.lock in commit 71203f68c774 ("padata: Fix list-related race
in padata_find_next"), rather than commit 16295bec6398).

However, commit af3852cda347 ("padata: remove __padata_list_init()") removed
that helper and initialized the two locks at separate spin_lock_init() call
sites in kernel/padata.c:

padata_init_squeues()
	INIT_LIST_HEAD(&squeue->serial.list);
	spin_lock_init(&squeue->serial.lock);

padata_init_reorder_list()
	INIT_LIST_HEAD(&list->list);
	spin_lock_init(&list->lock);

Because squeue->serial.lock and reorder->lock already have distinct lockdep
classes, acquiring reorder->lock inside squeue->serial.lock does not trigger
a recursive locking warning on current kernels.

Meanwhile, padata_do_serial() acquires the same reorder->lock at the default
subclass 0 before calling padata_reorder():

padata_do_serial()
	spin_lock(&reorder->lock);
	...
	spin_unlock(&reorder->lock);

	if (gotit)
		padata_reorder(padata);
			spin_lock(&squeue->serial.lock);
			padata_find_next()
				spin_lock_nested(&reorder->lock, SINGLE_DEPTH_NESTING);

Won't annotating reorder->lock with SINGLE_DEPTH_NESTING in
padata_find_next() while padata_do_serial() acquires it at subclass 0 cause
lockdep to track the same physical lock as two separate subclasses, hiding
lock ordering dependencies between padata_do_serial() and
padata_find_next()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102155353.1268322-1-tristan@talencesecurity.com?part=1