Background:
==========
Currently lockdep does not print out the held locks of non-current
tasks that are running on some other CPU, due to the fact that
the held locks array is in flux and may be unreliable to print.
Syzkaller on the other hand found it that the analysis of locking
bugs is easier if we print this information too, because the
more locking information the merrier. In particular races are
bound to have multiple tasks running on different CPUs, and
the exclusion of their held locks information is unnecessarily
limiting.
So while it's still true that printing out their held locks
array is racy, it's not as bad as it seems.
There's 16 internal callers to lockdep_print_held_locks():
- 14 callers call it with the current task, which should be
safe out of box.
- 1 caller, debug_show_all_locks(), calls it with RCU held,
which should guarantee that 'p' cannot go away under us.
- 1 caller, debug_show_held_locks(), exposes the internal API
with the constraint that it should only be called by drivers
or platform code if the task isn't actively running - we can
assume that if it nevertheless does, it will be Their Problem™.
As for held locks being changed from under debug_show_held_locks(),
while the task cannot go away, so the held-locks array itself is
safe (although potentially non-stable), AFAICS the worst-case race
can be garbage printed out by print_lock(), not any actual crashes.
In particular:
unsigned int class_idx = hlock->class_idx;
may be stale (belong to a lock that already got released on another
CPU), but it should still be a valid class index bound by
MAX_LOCKDEP_KEYS, and thus the lock_classes_in_use bitmap use
should be safe.
The other two accesses are ::acquire_ip and ::instance:
printk(KERN_CONT "%px", hlock->instance);
print_lock_name(hlock, lock);
printk(KERN_CONT ", at: %pS\n", (void *)hlock->acquire_ip);
But both are printed out as pointers, so no risk of dereference
of a dangling pointer. We may print a garbage pointer.
Also note that the check itself doesn't protect debug_show_held_locks()
from printing garbage, as there's nothing that keeps a task from
becoming runnable a nanosecond after we've run the task_is_running()
check. In fact I'd argue that it's better to make this function
*more* racy, for the simple robustness reason that we absolutely
do not want it to crash even in the racy case.
TL;DR: it should be fine to print the held locks of running
tasks too, as long as we print out the information as well
that a task is running, so that users are aware of any
racy output.
Implementation:
==============
Implement that change.
Also re-flow the function and streamline the printout into
a single statement for all cases, which changes
the 'no locks held by' / '%d lock[s] held by' phrasing that had a
dependency on English spelling of plurals, to a uniform:
locks held by bash/1234: %d
Which spells correctly for 0, 1 and higher values, and should also
be easier to parse both for humans and for scripts.
Finally, print out the last CPU a task has ran on. This is very
useful information for races and for locking bugs in particular.
This basically extends the 'on CPU#%d' message we print for
running tasks to all tasks we print.
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Boqun Feng <boqun@kernel.org>
Cc: Gary Guo <gary@garyguo.net>
Cc: Mark Brown <broonie@kernel.org>
Cc: Theodore Tso <tytso@mit.edu>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Waiman Long <longman@redhat.com>
Link: https://patch.msgid.link/akoeSIQGwqd9cZwd@gmail.com
051f5d223d