On Thu, Jan 16, 2025 at 8:51 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 1/14/25 6:54 PM, Jason Xing wrote:
> > I construct my thoughts here according to our previous discussion:
> > 1. not limiting the use of is_fullsock, so in patch 2, I will use the
> > follow codes:
> > +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> > +{
> > + struct bpf_sock_ops_kern sock_ops;
> > +
> > + memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
> > + sock_ops.op = op;
> > + sock_ops.is_fullsock = 1;
> > + sock_ops.sk = sk;
>
> lgtm.
>
> > + BPF_CGROUP_RUN_PROG_SOCK_OPS(sk, &sock_ops, CGROUP_SOCK_OPS);
>
> After looking through the set and looking again at how sk is used in
> __skb_tstamp_tx(), I think the sk must be fullsock here, so using
> __cgroup_bpf_run_filter_sock_ops() as in patch 2 is good. It will be useful to
> have a comment here to explain it must be a fullsock.
Got it, will add more comments on it.
>
> > +}
> >
> > 2. introduce the allow_direct_access flag which is used to test if the
> > socket is allowed to access tcp socket or not.
>
> yeah, right now is only tcp_sock, but future will have UDP TS support.
>
> May be the "allow_direct_access" naming is not obvious to mean the existing
> tcp_sock support. May be "allow_tcp_access"?
I like this name :)
>
> I was thinking to set the allow_direct_access for the "existing" sockops
> callback which must be tcp_sock and must have the sk locked.
>
> > On the basis of the above bpf_skops_tx_timestamping() function, I
> > would add one check there:
> > + if (sk_is_tcp(sk))
> > + sock_ops. allow_direct_access = 1;
>
> so don't set this in the new TS callback from bpf_skops_tx_timestamping
> regardless it is tcp or not.
>
> >
> > Also, I need to set allow_direct_access to one as long as there is
> > "sock_ops.is_fullsock = 1;" in the existing callbacks.
>
> Only set allow_direct_access when the sk is fullsock in the "existing" sockops
> callback.
Only "existing"? Then how can the bpf program access those members of
the tcp socket structure in the current/new timestamping callbacks?
>
> After thinking a bit more today, I think this should work. Please give it a try
> and check if some cases may be missed in sock_ops_convert_ctx_access().
I will give it a shot this week.
>
> >
> > 3. I will replace is_fullsock with allow_direct_access in
> > SOCK_OPS_GET/SET_FIELD() instead of SOCK_OPS_GET_SK().
>
> Yep.
>
> >
> > Then the udp socket can freely access the socket with the helper
> > SOCK_OPS_GET_SK() because it is a fullsock. And udp socket cannot
> > access struct tcp_sock because in the timestamping callback, there is
> > no place where setting allow_direct_access for udp use.
>
> __sk_buff->sk? yes.
