Message-ID: <CAL+tcoCvqkY4=XPLkwKguhPCH0Dduc4Xk4GO2ymySo9Nv2sh3A@mail.gmail.com>
Date: Tue, 15 Oct 2024 11:02:21 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, willemb@...gle.com, ast@...nel.org, 
	daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev, 
	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, bpf@...r.kernel.org, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2 00/12] net-timestamp: bpf extension to equip
 applications transparently

On Tue, Oct 15, 2024 at 10:59 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > On Tue, Oct 15, 2024 at 9:28 AM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > On Sun, Oct 13, 2024 at 1:48 AM Willem de Bruijn
> > > > <willemdebruijn.kernel@...il.com> wrote:
> > > > >
> > > > > Jason Xing wrote:
> > > > > > From: Jason Xing <kernelxing@...cent.com>
> > > > > >
> > > > > > A few weeks ago, I planned to extend SO_TIMESTMAMPING feature by using
> > > > > > tracepoint to print information (say, tstamp) so that we can
> > > > > > transparently equip applications with this feature and require no
> > > > > > modification in user side.
> > > > > >
> > > > > > Later, we discussed at netconf and agreed that we can use bpf for better
> > > > > > extension, which is mainly suggested by John Fastabend and Willem de
> > > > > > Bruijn. Many thanks here! So I post this series to see if we have a
> > > > > > better solution to extend. My feeling is BPF is a good place to provide
> > > > > > a way to add timestamping by administrators, without having to rebuild
> > > > > > applications.
> > > > > >
> > > > > > This approach mostly relies on existing SO_TIMESTAMPING feature, users
> > > > > > only needs to pass certain flags through bpf_setsocktop() to a separate
> > > > > > tsflags. For TX timestamps, they will be printed during generation
> > > > > > phase. For RX timestamps, we will wait for the moment when recvmsg() is
> > > > > > called.
> > > > > >
> > > > > > After this series, we could step by step implement more advanced
> > > > > > functions/flags already in SO_TIMESTAMPING feature for bpf extension.
> > > > > >
> > > > > > In this series, I only support TCP protocol which is widely used in
> > > > > > SO_TIMESTAMPING feature.
> > > > > >
> > > > > > ---
> > > > > > V2
> > > > > > Link: https://lore.kernel.org/all/20241008095109.99918-1-kerneljasonxing@gmail.com/
> > > > > > 1. Introduce tsflag requestors so that we are able to extend more in the
> > > > > > future. Besides, it enables TX flags for bpf extension feature separately
> > > > > > without breaking users. It is suggested by Vadim Fedorenko.
> > > > > > 2. introduce a static key to control the whole feature. (Willem)
> > > > > > 3. Open the gate of bpf_setsockopt for the SO_TIMESTAMPING feature in
> > > > > > some TX/RX cases, not all the cases.
> > > > > >
> > > > > > Note:
> > > > > > The main concern we've discussion in V1 thread is how to deal with the
> > > > > > applications using SO_TIMESTAMPING feature? In this series, I allow both
> > > > > > cases to happen at the same time, which indicates that even one
> > > > > > applications setting SO_TIMESTAMPING can still be traced through BPF
> > > > > > program. Please see patch [04/12].
> > > > >
> > > > > This revision does not address the main concern.
> > > > >
> > > > > An administrator installed BPF program can affect results of a process
> > > > > using SO_TIMESTAMPING in ways that break it.
> > > >
> > > > Sorry, I didn't get it. How the following code snippet would break users?
> > >
> > > The state between user and bpf timestamping needs to be separate to
> > > avoid interference.
> >
> > Do you agree that we will use this method as following, only allow
> > either of them to work?
> >
> > void __skb_tstamp_tx(struct sk_buff *orig_skb,
> >                      const struct sk_buff *ack_skb,
> >                      struct skb_shared_hwtstamps *hwtstamps,
> >                      struct sock *sk, int tstype)
> > {
> >         if (!sk)
> >                 return;
> >
> >        ret = skb_tstamp_tx_output(orig_skb, ack_skb, hwtstamps, sk, tstype);
> >        if (ret)
> >                /* Apps does set the SO_TIMESTAMPING flag, return directly */
> >                return;
> >
> >        if (static_branch_unlikely(&bpf_tstamp_control))
> >                 bpf_skb_tstamp_tx_output(sk, orig_skb, tstype, hwtstamps);
> > }
> >
> > which means if the apps using non-bpf method, we will not see the
> > output even if we load bpf program.
>
> Could the bpf setsockopt fail hard in that case?

We can do this. I think I will add some if test statements to see if
sk_tsflags is initialized before.

>
> Your current patch tries to make them work at the same time. It mostly
> does work. There are just a few concerning edge cases that may result
> in hard to understand bugs.

Agree.

>
> Making only one method work per socket and fail hard if both try it is
> crude, but at least the failure will be clear: the setsockopt fails.
>
> I think that's safer. And in practice, the use cases for BPF
> timestamping probably are exactly when application timestamping is
> missing?

Fair enough. Let me try this way:)

Thanks,
Jason

Powered by blists - more mailing lists