On 12/13/24 7:44 AM, Jason Xing wrote:
@@ -5569,7 +5569,10 @@ static void __skb_tstamp_tx_bpf(struct sock *sk, struct sk_buff *skb,
return;
}
- bpf_skops_tx_timestamping(sk, skb, op, 2, args);
+ if (sk_is_tcp(sk))
+ args[2] = skb_shinfo(skb)->tskey;
Instead of only passing one info "skb_shinfo(skb)->tskey" of a skb, pass the
whole skb ptr to the bpf prog. Take a look at bpf_skops_init_skb. Lets start
with end_offset = 0 for now so that the bpf prog won't use it to read the
skb->data. It can be revisited later.
bpf_skops_init_skb(&sock_ops, skb, 0);
The bpf prog can use bpf_cast_to_kern_ctx() and bpf_core_cast() to get to the
skb_shinfo(skb). Take a look at the md_skb example in type_cast.c.
Sorry, I didn't give it much thought on getting to the shinfo. That's
why I quickly gave up using bpf_skops_init_skb() after I noticed the
seq of skb is always zero 🙁
I will test it tomorrow. Thanks.
Then it needs to add a bpf_sock->op check to the existing
bpf_sock_ops_{load,store}_hdr_opt() helpers to ensure these helpers can only be
used by the BPF_SOCK_OPS_PARSE_HDR_OPT_CB, BPF_SOCK_OPS_HDR_OPT_LEN_CB, and
BPF_SOCK_OPS_WRITE_HDR_OPT_CB callback.
Forgive me. I cannot see how the bpf_sock_ops_load_hdr_opt helper has
something to do with the current thread? Could you enlighten me?
Sure. This is the same discussion as in patch 2, so may be worth to highlight
something that I guess may be missing:
a bpf prog does not need to use a helper does not mean:
a bpf prog is not allowed to call a helper because it is not safe.
The sockops prog running at the new timestamp hook does not need to call
bpf_setsockopt() but it does not mean the bpf prog is not allowed to call
bpf_setsockopt() without holding the sk_lock which is then broken.
The sockops timestamp prog does not need to use the
bpf_sock_ops_{load,store}_hdr_opt but it does not mean the bpf prog is not
allowed to call bpf_sock_ops_{load,store}_hdr_opt to change the skb which is
then also broken.
Now, skops->skb is not NULL only when the sockops prog is allowed to read/write
the skb.
With bpf_skops_init_skb(), skops->skb will not be NULL in the new timestamp
callback hook. bpf_sock_ops_{load,store}_hdr_opt() will be able to use the
skops->skb and it will be broken.
btw, how is the ack_skb used for the SCM_TSTAMP_ACK by the user space now?
To be honest, I hardly use the ack_skb[1] under this circumstance... I
think if someone offers a suggestion to use it, then we can support
it?
Thanks for the pointer.
Yep, supporting it later is fine. I am curious because the ack_skb is used in
the user space time stamping now but not in your patch. I was asking to ensure
that we should be able to support it in the future if there is a need. We
should be able to reuse the skops->syn_skb to support that in the future.
[1]
commit e7ed11ee945438b737e2ae2370e35591e16ec371
Author: Yousuk Seung<ysseung@xxxxxxxxxx>
Date: Wed Jan 20 12:41:55 2021 -0800
tcp: add TTL to SCM_TIMESTAMPING_OPT_STATS
This patch adds TCP_NLA_TTL to SCM_TIMESTAMPING_OPT_STATS that exports
the time-to-live or hop limit of the latest incoming packet with
SCM_TSTAMP_ACK. The value exported may not be from the packet that acks
the sequence when incoming packets are aggregated. Exporting the
time-to-live or hop limit value of incoming packets helps to estimate
the hop count of the path of the flow that may change over time.
