Re: [PATCH bpf v4 5/5] bpf, sockmap: Take state lock for af_unix iter

On 4/14/26 23:30, [email protected] wrote:
>> @@ -3737,6 +3737,7 @@ static int bpf_iter_unix_seq_show(struct seq_file *seq, void *v)
>>  		return 0;
>>  
>>  	lock_sock(sk);
>> +	unix_state_lock(sk);
> 
> [ ... ]
> 
>> @@ -3748,6 +3749,7 @@ static int bpf_iter_unix_seq_show(struct seq_file *seq, void *v)
>>  	prog = bpf_iter_get_info(&meta, false);
>>  	ret = unix_prog_seq_show(prog, &meta, v, uid);
> 
> Does holding a spinlock during bpf program execution introduce
> sleep-in-atomic or lock inversion issues?
> 
> If the bpf iterator program is loaded with BPF_F_SLEEPABLE,
> bpf_iter_run_prog() explicitly calls might_fault(). Calling might_fault()
> while holding a spinlock (which disables preemption) triggers a sleep in
> atomic context bug.

See bpf_iter_link_attach(): BPF_F_SLEEPABLE is only allowed for
resched-able iterators (BPF_ITER_RESCHED). This excludes unix iter.

> Also, could this introduce a lock inversion risk? The bpf programs can
> interact with maps or other sockets, acquiring locks like map->lock or
> bh_lock_sock. Taking unix_state_lock before these locks might create an
> unnatural lock hierarchy, potentially leading to deadlocks if other kernel
> paths acquire these locks in the standard order.

Nothing I can think of. Any specific lock inversion scenario to consider?

> Additionally, does this fully resolve the use-after-free in
> unix_stream_bpf_update_proto()?
> 
> Since unix_stream_bpf_update_proto() is the underlying callback used
> whenever an af_unix socket is inserted into a sockmap, can it be reached
> independently of this iterator?
> 
> For example, could it be reached via the bpf(BPF_MAP_UPDATE_ELEM) syscall,
> or when a bpf program updates the map using a different af_unix socket than
> the one being iterated? In those cases, unix_state_lock(sk) would not be
> held, leaving the use-after-free unresolved.

No, I don't think it can race the bpf() syscall. Both
bpf_iter_unix_seq_show() (see patch 2/5) and sock_map_update_elem_sys()
take the lock_sock. The latter via sock_map_sk_acquire().

Yes, bpf updating sockmap using a different unix sock than the one being
iterated is possible:
a) iter prog accessing another sock (e.g. peer) by pointer chasing. To my
understanding that's something the bpf validator is expected to reject[1].
b) iter prog looking up another sock in a sockmap. That sounds like a
(safe) scenario 2.2.1 described in patch 4/5 commit message.

And yes, as pointed out by Martin[2], there's still the issue of
sock_map_update_elem() missing sock_owned_by_user() check.

[1]:
https://lore.kernel.org/bpf/CAADnVQK_93g_KkNFYXSr8ZvA1fYh4hoFRJCJFPS-zs4ox0HhAA@mail.gmail.com/
[2]:
https://lore.kernel.org/bpf/[email protected]/

> Should the lock be acquired directly inside unix_stream_bpf_update_proto()
> instead?

unix_stream_bpf_update_proto() is called in both process (e.g. syscall) and
softirq (e.g. TC prog) contexts. Taking spinlock there would lead to
context violation.