Re: [PATCH net-next v4 10/11] net-timestamp: export the tskey for TCP bpf extension

[Martin KaFai Lau <martin.lau@linux.dev>]

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@google.com>
> 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.