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.
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.
