On Sat, Jan 25, 2025 at 10:26 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 1/24/25 5:15 PM, Jason Xing wrote:
> >>> +static bool is_locked_tcp_sock_ops(struct bpf_sock_ops_kern *bpf_sock)
> >>> +{
> >>> + return bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB;
> >>
> >> More bike shedding...
> >>
> >> After sleeping on it again, I think it can just test the
> >> bpf_sock->allow_tcp_access instead.
> >
> > 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.
>
> >>> @@ -5673,7 +5678,12 @@ static const struct bpf_func_proto bpf_sock_addr_getsockopt_proto = {
> >>> BPF_CALL_5(bpf_sock_ops_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
> >>> int, level, int, optname, char *, optval, int, optlen)
> >>> {
> >>> - return _bpf_setsockopt(bpf_sock->sk, level, optname, optval, optlen);
> >>> + struct sock *sk = bpf_sock->sk;
> >>> +
> >>> + if (is_locked_tcp_sock_ops(bpf_sock) && sk_fullsock(sk))
> >>
> >> afaict, the new timestamping callbacks still can do setsockopt and it is
> >> incorrect. It should be:
> >>
> >> if (!bpf_sock->allow_tcp_access)
> >> return -EOPNOTSUPP;
> >>
> >> I recalled I have asked in v5 but it may be buried in the long thread, so asking
> >> here again. Please add test(s) to check that the new timestamping callbacks
> >> cannot call setsockopt and read/write to some of the tcp_sock fields through the
> >> bpf_sock_ops.
> >>
> >>> + sock_owned_by_me(sk);
> >>
> >> Not needed and instead...
> >
> > Sorry I don't get you here. What I was doing was letting non
> > timestamping callbacks be checked by the sock_owned_by_me() function.
> >
> > If the callback belongs to timestamping, we will skip the check.
>
> It will skip the sock_owned_by_me() test and
> continue to do the following __bpf_setsockopt() which the new timetamping
> callback should not do, no?
>
> It should be just this at the very beginning of bpf_sock_ops_setsockopt:
>
> if (!is_locked_tcp_sock_ops(bpf_sock))
> return -EOPNOTSUPP;
> >
> >>
> >>> +
> >>> + return __bpf_setsockopt(sk, level, optname, optval, optlen);
> >>
> >> keep the original _bpf_setsockopt().
> >
> > Oh, I remembered we've already assumed/agreed the timestamping socket
> > must be full sock. I will use it.
> >
> >>
> >>> }
> >>>
> >>> static const struct bpf_func_proto bpf_sock_ops_setsockopt_proto = {
> >>> @@ -5759,6 +5769,7 @@ BPF_CALL_5(bpf_sock_ops_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
> >>> int, level, int, optname, char *, optval, int, optlen)
> >>> {
> >>> if (IS_ENABLED(CONFIG_INET) && level == SOL_TCP &&
> >>> + bpf_sock->sk->sk_protocol == IPPROTO_TCP &&
> >>> optname >= TCP_BPF_SYN && optname <= TCP_BPF_SYN_MAC) {
> >>
> >> No need to allow getsockopt regardless what SOL_* it is asking. To keep it
> >> simple, I would just disable both getsockopt and setsockopt for all SOL_* for
> >
> > Really? I'm shocked because the selftests in this series call
> > bpf_sock_ops_getsockopt() and bpf_sock_ops_setsockopt() in patch
> > [13/13]:
>
> Yes, really. It may be late Friday for me here. Please double check your test if
> the bpf_set/getsockopt is called from the new timestamp callback or it is only
> called from the existing BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB callback.
Oh, after reading the above a few times, I finally realized you're
right. I'll do it. Thanks!!
>
> Note that I am only asking to disable the set/getsockopt,
> bpf_sock_ops_cb_flags_set, and the bpf_sock_ops_load_hdr_opt for the new
> timestamping callbacks.
Right, right!
Thanks,
Jason
