netdev - Re: [PATCH bpf-next v7 11/13] net-timestamp: add a new callback in tcp_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoDZXc56BsO9tYvb1EFDdMHhv3OcBsPwY3ctJ85rvb+OHA@mail.gmail.com>
Date: Tue, 4 Feb 2025 09:25:05 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, willemdebruijn.kernel@...il.com, 
	willemb@...gle.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, 
	eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev, 
	john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me, 
	haoluo@...gle.com, jolsa@...nel.org, horms@...nel.org, bpf@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v7 11/13] net-timestamp: add a new callback in tcp_tx_timestamp()

On Tue, Feb 4, 2025 at 9:16 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 1/28/25 12:46 AM, Jason Xing wrote:
> > Introduce the callback to correlate tcp_sendmsg timestamp with other
> > points, like SND/SW/ACK. We can let bpf trace the beginning of
> > tcp_sendmsg_locked() and fetch the socket addr, so that in
>
> Instead of "fetch the socket addr...", should be "store the sendmsg timestamp at
> the bpf_sk_storage ...".

I will revise it. Thanks.

>
> > tcp_tx_timestamp() we can correlate the tskey with the socket addr.
>
>
> > It is accurate since they are under the protect of socket lock.
> > More details can be found in the selftest.
>
> The selftest uses the bpf_sk_storage to store the sendmsg timestamp at
> fentry/tcp_sendmsg_locked and retrieves it back at tcp_tx_timestamp (i.e.
> BPF_SOCK_OPS_TS_SND_CB added in this patch).
>
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> > ---
> >   include/uapi/linux/bpf.h       | 7 +++++++
> >   net/ipv4/tcp.c                 | 1 +
> >   tools/include/uapi/linux/bpf.h | 7 +++++++
> >   3 files changed, 15 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 800122a8abe5..accb3b314fff 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -7052,6 +7052,13 @@ enum {
> >                                        * when SK_BPF_CB_TX_TIMESTAMPING
> >                                        * feature is on.
> >                                        */
> > +     BPF_SOCK_OPS_TS_SND_CB,         /* Called when every sendmsg syscall
> > +                                      * is triggered. For TCP, it stays
> > +                                      * in the last send process to
> > +                                      * correlate with tcp_sendmsg timestamp
> > +                                      * with other timestamping callbacks,
> > +                                      * like SND/SW/ACK.
>
> Do you have a chance to look at how this will work at UDP?

Sure, I feel like it could not be useful for UDP. Well, things get
strange because I did write a long paragraph about this thing which
apparently disappeared...

I manage to find what I wrote:
    For UDP type, BPF_SOCK_OPS_TS_SND_CB may be not suitable because
    there are two sending process, 1) lockless path, 2) lock path, which
    should be handled carefully later. For the former, even though it's
    unlikely multiple threads access the socket to call sendmsg at the
    same time, I think we'd better not correlate it like what we do to the
    TCP case because of the lack of sock lock protection. Considering SND_CB is
    uapi flag, I think we don't need to forcely add the 'TCP_' prefix in
    case we need to use it someday.

    And one more thing is I'd like to use the v5[1] method in the next round
    to introduce a new tskey_bpf which is good for UDP type. The new field
    will not conflict with the tskey in shared info which is generated
    by sk->sk_tskey in __ip_append_data(). It hardly works if both features
    (so_timestamping and its bpf extension) exists at the same time. Users
    could get confused because sometimes they fetch the tskey from skb,
    sometimes they don't, especially when we have cmsg feature to turn it on/
    off per sendmsg. A standalone tskey for bpf extension will be needed.
    With this tskey_bpf, we can easily correlate the timestamp in sendmsg
    syscall with other tx points(SND/SW/ACK...).

    [1]: https://lore.kernel.org/all/20250112113748.73504-14-kerneljasonxing@gmail.com/

    If possible, we can leave this question until the UDP support series
    shows up. I will figure out a better solution :)

In conclusion, it probably won't be used by the UDP type. It's uAPI
flag so I consider the compatibility reason.