Re: [PATCH 8/9] locking/lockdep: Shrink static list_entries array to early bootstrap buffer

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

New issues:
- [High] Dynamically allocated lock_list entries from lockdep_pool are permanently leaked when lock classes are zapped.
- [High] Allocating from folio_pool under graph_lock violates lock nesting rules and will deadlock on PREEMPT_RT.
- [High] `nf_tables_abort()` resets the scratchpad incorrectly, breaking subsystem toggles and hardcoding chunk sizes.
- [High] Transient operations are leaked on every map/unmap due to lack of scratchpad reset in DRM GPUVM.
- [High] BPF `generic_map_update_batch()` unconditionally fails with `-ENOMEM` for maps with large elements.

Pre-existing issues:
- [High] Synchronous clearing of list_entries_in_use allows reallocation of bootstrap entries before an RCU grace period elapses, causing UAF for lockless readers.
--

--- Patch [8]: [PATCH 8/9] locking/lockdep: Shrink static list_entries array to early bootstrap buffer ---
commit 50e28e082c0e7079e9acf36ed3d4b7ea1bf5fae3
Author: Jim Cromie <[email protected]>

locking/lockdep: Shrink static list_entries array to early bootstrap buffer

Shrink the static compile-time list_entries[] array and its tracking
bitmap from MAX_LOCKDEP_ENTRIES (32,768 entries, ~1.31 MB BSS) down to
a 4096-entry early boot bootstrap buffer (~160 KB BSS).

[ ... ]

> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index c8975c9282bb9..5968a976bf8ea 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c

[ ... ]

> @@ -6287,26 +6295,30 @@ static void zap_class(struct pending_free *pf, struct lock_class *class)
>  	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) {
> -				__clear_bit(other - list_entries, list_entries_in_use);
> +				if (is_bootstrap_entry(other))
> +					__clear_bit(other - list_entries, list_entries_in_use);

[Severity: High]
This is a pre-existing issue, but does synchronous clearing of
list_entries_in_use allow reallocation of bootstrap entries before an
RCU grace period elapses?

When a module is unloaded, zap_class() unlinks the class's lock_list entries
and clears their bootstrap_entries_in_use bit immediately. If
alloc_list_entry() instantly reallocates the bootstrap entry and mutates its
list pointers while lockless RCU readers are actively iterating over it,
can this cause a use-after-free or RCU violation for lockless readers?

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

[Severity: High]
Are dynamically allocated lock_list entries from lockdep_pool being
permanently leaked when lock classes are zapped?

Because the underlying folio_pool allocator does not support individual
element deallocation via a simple unlink, entries that are not bootstrap
entries are permanently forgotten after being unlinked.

Since unloading modules registers and unregisters lock classes, could this
eventually lead to a system out-of-memory condition?

>  				break;
>  			}

[ ... ]

[Severity: High]
Does allocating from folio_pool under graph_lock violate lock nesting rules
and deadlock on PREEMPT_RT?

As mentioned in the commit message, alloc_list_entry() spills subsequent
allocations to the lockdep_pool. When called with graph_lock() held (which
acquires an arch_spinlock_t and disables interrupts), it invokes the
folio_pool allocator:

kernel/locking/lockdep.c:alloc_list_entry() {
    ...
	p = folio_pool_alloc_type(&lockdep_pool, struct lock_list, GFP_ATOMIC);
    ...
}

lib/folio_pool.c:folio_scratchpad_alloc() {
    ...
	spin_lock_irqsave(&sp->lock, flags);
    ...
}

Because sp->lock is a spinlock_t, which maps to a sleepable rt_mutex on
PREEMPT_RT, attempting to acquire it while holding a raw spinlock causes a
sleep-in-atomic deadlock. Can this cause an invalid wait context deadlock and
lockdep splat?


[Severity: High]
Does nf_tables_abort() reset the scratchpad incorrectly, breaking subsystem
toggles and hardcoding chunk sizes?

When an nf_tables transaction is aborted (e.g., due to an error in a batch
ruleset load), nf_tables_abort() reinitializes the scratchpad:

net/netfilter/nf_tables_api.c:nf_tables_abort() {
    ...
	folio_scratchpad_free(&nft_net->trans_scratchpad);
	folio_scratchpad_init(&nft_net->trans_scratchpad, 4);
    ...
}

On architectures with 64KB pages, order 4 results in 1MB chunks instead of
the intended 64KB chunks, wasting large amounts of memory. Additionally,
passing a NULL key (implicitly through folio_scratchpad_init) permanently
disconnects the scratchpad from the nft_trans_scratchpad_key module parameter.


[Severity: High]
Are transient operations leaked on every map/unmap due to lack of scratchpad
reset in DRM GPUVM?

drm_gpuva_op structures are allocated dynamically via gpuva_op_alloc() using
folio_scratchpad_alloc_obj(). When the operation completes, it is freed:

drivers/gpu/drm/drm_gpuvm.c:gpuva_op_free() {
    ...
	else
		folio_scratchpad_free_elem(op);
}

The caller assumes this frees the operation state, but because
folio_scratchpad_free_elem() is a no-op for bump allocators, and drm_gpuvm
never explicitly calls folio_scratchpad_reset() between operations, the
embedded va_scratchpad will endlessly consume memory.

Could this lead to a rapid out-of-memory condition when continuously mapping
and unmapping GPU memory?


[Severity: High]
Does generic_map_update_batch() unconditionally fail with -ENOMEM for maps
with large elements?

When a user issues a BPF_MAP_UPDATE_BATCH syscall, the batch_sp scratchpad
is initialized with chunk_order = 0:

kernel/bpf/syscall.c:generic_map_update_batch() {
    ...
	folio_scratchpad_init_key(&batch_sp, 0, &bpf_batch_scratchpad_key);

	key = folio_scratchpad_alloc(&batch_sp, map->key_size,
				     sizeof(void *), GFP_USER | __GFP_NOWARN);
    ...
}

Because the stack-local bump allocator cannot satisfy requests larger than the
chunk size (page limit minus header), it will unconditionally return NULL for
keys or values exceeding a single page. Does this deterministically break
backward compatibility for large map elements?

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