Re: [PATCH bpf v2] bpf, sockmap: disallow update and delete from tc, xdp and flow_dissector

[sashiko-bot@kernel.org]

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

New issues:
- [High] The commit silently breaks multiple existing BPF selftests by rejecting sockmap updates from TC, XDP, and flow_dissector programs without updating the tests, indicating an incomplete patch and a breaking UAPI change.

Pre-existing issues:
- [High] The patch incompletely fixes a circular locking dependency between `sk_callback_lock` and `stab->lock`. While it removes `BPF_PROG_TYPE_SCHED_CLS`, `BPF_PROG_TYPE_SCHED_ACT`, `BPF_PROG_TYPE_XDP`, and `BPF_PROG_TYPE_FLOW_DISSECTOR` from the allowed program types in `may_update_sockmap()`, it fails to remove `BPF_PROG_TYPE_SOCK_OPS` and `BPF_PROG_TYPE_SOCKET_FILTER`. Both of these can still be executed synchronously on the exact same egress path, triggering the identical deadlock.
--

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

bpf, sockmap: disallow update and delete from tc, xdp and flow_dissector

This patch removes the ability to update or delete sockmaps from TC, XDP, and
flow_dissector BPF programs. It aims to fix a circular locking dependency
between the sk_callback_lock and stab->lock that occurs during the egress
path.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2abc79dbf281c..612b264439c6c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8452,11 +8452,7 @@ static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
>  			return true;
>  		break;
>  	case BPF_PROG_TYPE_SOCKET_FILTER:

[Severity: High]
This is a pre-existing issue, but does leaving BPF_PROG_TYPE_SOCKET_FILTER
(and BPF_PROG_TYPE_SOCK_OPS just above this) allowed here still leave the door
open for the same deadlock?

Both of these can execute synchronously on the exact same egress path you
mentioned in the commit message:

sk_psock_strp_data_ready()
  tcp_bpf_strp_read_sock()
    __tcp_cleanup_rbuf()
      tcp_send_ack()
        __tcp_send_ack()
          __tcp_transmit_skb()

Inside __tcp_transmit_skb(), bpf_skops_write_hdr_opt() can execute a
BPF_PROG_TYPE_SOCK_OPS program. Alternatively, dev_queue_xmit() can call
dev_queue_xmit_nit() if an AF_PACKET socket is attached, executing a
BPF_PROG_TYPE_SOCKET_FILTER program.

If either program updates or deletes a sockmap element, wouldn't it acquire
stab->lock and recreate the sk_callback_lock -> stab->lock dependency?

> -	case BPF_PROG_TYPE_SCHED_CLS:
> -	case BPF_PROG_TYPE_SCHED_ACT:
> -	case BPF_PROG_TYPE_XDP:
>  	case BPF_PROG_TYPE_SK_REUSEPORT:
> -	case BPF_PROG_TYPE_FLOW_DISSECTOR:
>  	case BPF_PROG_TYPE_SK_LOOKUP:
>  		return true;

[Severity: High]
Since this drops support for XDP, TC, and flow_dissector, are there updates
needed for the selftests in tools/testing/selftests/bpf/progs/verifier_sockmap_mutate.c?

That test file explicitly expects success for xdp, flow_dissector,
classifier, and action programs performing sockmap mutations:

SEC("xdp")
__success
int test_xdp(struct xdp_md *ctx __always_unused)

Will this change cause the verifier to reject those programs and fail the
test suite?

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