On 3/3/26 02:51, Martin KaFai Lau wrote: > On 2/24/26 7:28 AM, Michal Luczaj wrote: >> On 2/23/26 22:43, Martin KaFai Lau wrote: >>> >>> On 2/11/26 2:02 AM, Michal Luczaj wrote: >>>>> It should also be helpful to be consistent with tcp/udp iter and use >>>>> lock_sock() instead of lock_sock_fast() in bpf_iter_unix_seq_show(). >>>> OK, I'll tweak that in v3. >>> >>> Hi, Michal. Are you planning to send v3 soon? I don't think the >>> sock_owned_by_user for the non-tracing prog needs to be addressed in the >>> same set. >> >> Yes, I'm working on it. Sorry for the delay, I'm taking my sweet time to >> come up with a selftest. >> >> I think I can neatly fit the sock_owned_by_user()-related stuff in this >> series, but let me know if you'd rather have it separately. Whichever way, >> I don't mind. > > I think sock_owned_by_user() is not related to the AF_UNIX's sockmap > fix. If it is not, it's better to separate it. I think one thing at a > time is easier to review.
All right, sending out v3 to reduce the scope: https://lore.kernel.org/bpf/20260306-unix-proto-update-null-ptr-deref-v3-0-2f0c7410c...@rbox.co/ > If they are related somewhat or it's easier to review them together, see > if it makes sense to structure them in a way such that the AF_UNIX's > sockmap-related fixes and tests are at the beginning of the patch set so > that they can be applied first if others need more discussion. I'd say those changes are related, but things are getting unclear for me, I won't insist on keeping them together. So. Simply dropping BH locks and introducing sock_owned_by_user() in sock_map_update_elem() is not enough. For example, sock_map_seq_show() and sock_hash_seq_show() must be adapted before taking the sock lock (as they currently run under RCU). But there's a deeper problem: bpf iters let the bpf code call sockmap update when sk's file descriptor is halfway through sock_map_close(). This breaks the sockmap contract as the update can happen after sock_map_remove_links(), but before saved_close(). sockmap ends up with a closed sk that normally would be removed. For sockmap/sockhash iters this condition can be handled: before running the bpf tracer prog, check if sk is still linked to map. Since those are sockmap/sockhash iters, if there's no link, it means sk should be skipped. But I'm not sure about other iters. Plus the case you've mentioned of bpf tc progs that can call sockmap update with sk unowned. Hence I stop here worried this may be a doomed approach :) Long story short: feels like taking the spinlocks (without unconditionally checking sock_owned_by_user()) in sock_map_update_elem() is the best approach. Just as it is currently implemented (+the af_unix special casing). Some sks will come owned, some won't. What needs to be added after taking the spinlock(s) is a sort of is-sk-being-closed check. Or am I missing something? > Regarding the "Keeping sparse annotations in > sock_map_sk_{acquire,release}() required some > hackery...". Maybe just remove the annotations? > [ btw, from commit 5b63d0ae94cc, the sparse support is removed and it > depends solely on clang now (?). If it is the case, what does clang do > on the "sock_or_unix_lock"? ] Oh, I haven't seen that one, thanks. Will take a look. For now, I've just dropped the annotations.
