On 1/24/25 6:25 PM, Martin KaFai Lau wrote:
Sorry, I don't think it can work for all the cases because:
1) please see BPF_SOCK_OPS_WRITE_HDR_OPT_CB/BPF_SOCK_OPS_HDR_OPT_LEN_CB,
if req exists, there is no allow_tcp_access initialization. Then
calling some function like bpf_sock_ops_setsockopt will be rejected
because allow_tcp_access is zero.
2) tcp_call_bpf() only set allow_tcp_access only when the socket is
fullsock. As far as I know, all the callers have the full stock for
now, but in the future it might not.
Note that the existing helper bpf_sock_ops_cb_flags_set and
bpf_sock_ops_{set,get}sockopt itself have done the sk_fullsock() test and then
return -EINVAL. bpf_sock->sk is fullsock or not does not matter to these helpers.
You are right on the BPF_SOCK_OPS_WRITE_HDR_OPT_CB/BPF_SOCK_OPS_HDR_OPT_LEN_CB
but the only helper left that testing allow_tcp_access is not enough is
bpf_sock_ops_load_hdr_opt(). Potentially, it can test "if (!bpf_sock-
>allow_tcp_access && !bpf_sock->syn_skb) { return -EOPNOTSUPP; }".
Agree to stay with the current "bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB"
as in this patch. It is cleaner.
Also ignore my earlier comment on merging patch 3 and 4. Better keep patch 4 on
its own since it is not reusing the allow_tcp_access test. Instead, stay with
the "bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB" test.
