On Sat, Jan 25, 2025 at 11:12 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> 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.
Got it!
Thanks,
Jason
