netdev - Re: [PATCH bpf-next v7 05/13] net-timestamp: prepare for isolating two modes of SO

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+tcoALoyfc7wL7odJ9uzsuD0D6BJ4qFpNJgxsAOHE21i_LAA@mail.gmail.com>
Date: Tue, 4 Feb 2025 08:18:36 +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 05/13] net-timestamp: prepare for isolating
 two modes of SO_TIMESTAMPING

On Tue, Feb 4, 2025 at 7:14 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 1/28/25 12:46 AM, Jason Xing wrote:
> > No functional changes here. I add skb_enable_app_tstamp() to test
> > if the orig_skb matches the usage of application SO_TIMESTAMPING
> > and skb_sw_tstamp_tx() to distinguish the software and hardware
>
> There is no skb_sw_tstamp_tx() in the code. An outdated commit message?

Thanks. I'll update it and double check before reposting.

>
> > timestamp when tsflag is SCM_TSTAMP_SND.
> >
> > Also, I deliberately distinguish the the software and hardware
> > SCM_TSTAMP_SND timestamp by passing 'sw' parameter in order to
> > avoid such a case where hardware may go wrong and pass a NULL
> > hwstamps, which is even though unlikely to happen. If it really
> > happens, bpf prog will finally consider it as a software timestamp.
> > It will be hardly recognized. Let's make the timestamping part
> > more robust.
> >
> > After this patch, I will soon add checks about bpf SO_TIMESTAMPING.
>
> This needs to be updated also. BPF does not use the SO_TIMESTAMPING socket option.
>
> > In this way, we can support two modes parallelly.
>
> s/parallely/in parallel/

Will fix it.

>
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> > ---
> >   include/linux/skbuff.h | 13 +++++++------
> >   net/core/dev.c         |  2 +-
> >   net/core/skbuff.c      | 32 ++++++++++++++++++++++++++++++--
> >   net/ipv4/tcp_input.c   |  3 ++-
> >   4 files changed, 40 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index bb2b751d274a..dfc419281cc9 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -39,6 +39,7 @@
> >   #include <net/net_debug.h>
> >   #include <net/dropreason-core.h>
> >   #include <net/netmem.h>
> > +#include <uapi/linux/errqueue.h>
> >
> >   /**
> >    * DOC: skb checksums
> > @@ -4533,18 +4534,18 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
> >
> >   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);
> > +                  struct sock *sk, bool sw, int tstype);
> >
> >   /**
> > - * skb_tstamp_tx - queue clone of skb with send time stamps
> > + * skb_tstamp_tx - queue clone of skb with send HARDWARE timestamps
> >    * @orig_skb:       the original outgoing packet
> >    * @hwtstamps:      hardware time stamps, may be NULL if not available
> >    *
> >    * If the skb has a socket associated, then this function clones the
> >    * skb (thus sharing the actual data and optional structures), stores
> > - * the optional hardware time stamping information (if non NULL) or
> > - * generates a software time stamp (otherwise), then queues the clone
>
> This line is removed. Does it mean no software timestamp now after this change?

Right, _software_ timestamp will enter skb_tx_timestamp() then call
__skb_tstamp_tx() instead of this skb_tx_timestamp().

>
> > - * to the error queue of the socket.  Errors are silently ignored.
> > + * the optional hardware time stamping information (if non NULL) then
> > + * queues the clone to the error queue of the socket.  Errors are
> > + * silently ignored.
> >    */
> >   void skb_tstamp_tx(struct sk_buff *orig_skb,
> >                  struct skb_shared_hwtstamps *hwtstamps);
> > @@ -4565,7 +4566,7 @@ static inline void skb_tx_timestamp(struct sk_buff *skb)
> >   {
> >       skb_clone_tx_timestamp(skb);
> >       if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
> > -             skb_tstamp_tx(skb, NULL);
> > +             __skb_tstamp_tx(skb, NULL, NULL, skb->sk, true, SCM_TSTAMP_SND);
> >   }
>
>