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.
+}
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 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.
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().
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.
