Re: [PATCH bpf] bpf, sockmap: fix lock inversion between stab->lock and sk_callback_lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch resolves a circular locking dependency between `stab->lock` and `sk_callback_lock` for `sockmap`, but completely misses the identical circular locking dependency between `bucket->lock` and `sk_callback_lock` for `sockhash`.
--

commit ca126c8c2f5577ef6bb462f6d0bbf16156b6795a
Author: Sechang Lim <rhkrqnwk98@gmail.com>

bpf, sockmap: fix lock inversion between stab->lock and sk_callback_lock

This patch resolves a circular locking dependency between stab->lock and
sk_callback_lock in the sockmap code. It prevents a deadlock by releasing
stab->lock prior to calling sock_map_unref() during element updates and
deletions.

> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 99e3789492a09..390bd5ee46d44 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
[ ... ]
> @@ -505,9 +505,10 @@ static int sock_map_update_common(struct bpf_map *map, u32 idx,
>  
>  	sock_map_add_link(psock, link, map, &stab->sks[idx]);
>  	stab->sks[idx] = sk;
> +	spin_unlock_bh(&stab->lock);
> +
>  	if (osk)
>  		sock_map_unref(osk, &stab->sks[idx]);
> -	spin_unlock_bh(&stab->lock);
>  	return 0;
>  out_unlock:
>  	spin_unlock_bh(&stab->lock);

[Severity: High]
This is a pre-existing issue, but while fixing the lock inversion for sockmap,
does the identical circular locking dependency still exist for sockhash between
bucket->lock and sk_callback_lock?

Looking at sock_hash_delete_from_link() in net/core/sock_map.c,
sock_map_unref() is called while the bucket lock is still held:

		hlist_del_rcu(&elem->node);
		sock_map_unref(elem->sk, elem);
		sock_hash_free_elem(htab, elem);
	}
	spin_unlock_bh(&bucket->lock);

This pattern is also present in sock_hash_delete_elem() and
sock_hash_update_common() in the same file.

Could this lead to the same ABBA deadlock if a thread updates or deletes an
element in a SOCKHASH map (acquiring bucket->lock and then sk_callback_lock via
sock_map_unref), while concurrently a stream parser holds sk_callback_lock for
read and a TC program triggers bpf_map_delete_elem() on the same bucket?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616091153.2966617-1-rhkrqnwk98@gmail.com?part=1