Re: [PATCH 7/9] locking/lockdep: Traverse adjacency lists directly in zap_class()

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] zap_class() incorrectly searches the zapped class's own dependency lists instead of the connected classes' lists, failing to remove remote back-edges and causing Use-After-Free.
- [High] Out-of-bounds memory write in zap_class() due to unsafe pointer arithmetic on dynamically allocated lock_list entries.

Pre-existing issues:
- [High] lock_list entries are synchronously returned to the allocator before an RCU grace period expires, exposing lockless RCU readers to torn data (RCU-001 violation).
--

--- Patch [7]: [PATCH 7/9] locking/lockdep: Traverse adjacency lists directly in zap_class() ---
commit 71cf1d3ff4ec0c990dff428fe396cff0df9ba08d
Author: Jim Cromie <[email protected]>

locking/lockdep: Traverse adjacency lists directly in zap_class()

Rework zap_class() to traverse class->locks_after and class->locks_before
directly, unlinking each edge and its matching counterpart on the
connected lock class via RCU while clearing the respective allocation
slot bits.

> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index f0f58db090fff..c8975c9282bb9 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -6285,11 +6284,29 @@ static void zap_class(struct pending_free *pf, struct lock_class *class)
>  	 * Remove all dependencies this lock is
>  	 * involved in:
>  	 */
> -	for_each_set_bit(i, list_entries_in_use, ARRAY_SIZE(list_entries)) {
> -		entry = list_entries + i;
> -		if (entry->class != class && entry->links_to != class)
> -			continue;
> -		__clear_bit(i, list_entries_in_use);
> +	list_for_each_entry_safe(entry, tmp, &class->locks_after, entry) {
> +		list_for_each_entry_safe(other, other_tmp, &entry->links_to->locks_before, entry) {
> +			if (other->links_to == class) {

[Severity: High]
Does this code iterate the correct list to find the remote edge?

If entry->links_to points to the zapped class itself, iterating over
&entry->links_to->locks_before traverses the zapped class's own incoming
edges rather than the remote class's edges. Furthermore, the condition
(other->links_to == class) trivially matches elements in its own list.

Does this regression cause the actual reciprocal edges in the remote
classes to be skipped, leaving dangling pointers to the freed class that
could trigger a use-after-free during subsequent lockdep graph traversals?

> +				__clear_bit(other - list_entries, list_entries_in_use);

[Severity: High]
Could this pointer arithmetic regression cause out-of-bounds memory writes?

Lockdep supports allocating lock_list entries dynamically via a folio pool
when the static list_entries array is exhausted. By calculating bit indices
with (other - list_entries), this produces an out-of-bounds offset for
dynamically allocated entries, causing __clear_bit() to overwrite arbitrary
kernel memory.

Also, does this regression leak the dynamically allocated lock_list entries
since they are never freed back to their pool?

> +				nr_list_entries--;
> +				list_del_rcu(&other->entry);

[Severity: High]
This is a pre-existing issue, but does synchronously clearing the allocation
bit before an RCU grace period expose lockless RCU readers to torn data?

zap_class() unlinks dependency edges using list_del_rcu() but immediately
marks them as available by clearing their bit in the list_entries_in_use
bitmap above. This allows concurrent calls to alloc_list_entry() to
reallocate the struct lock_list block and insert it into a different list,
modifying its next pointer.

Could lockless readers traversing the original list observe the modified
next pointer and be steered into unrelated lists or infinite loops?

> +				break;
> +			}
> +		}
> +		__clear_bit(entry - list_entries, list_entries_in_use);
> +		nr_list_entries--;
> +		list_del_rcu(&entry->entry);
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7