netdev - Re: [PATCH bpf-next v8 05/12] 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+tcoDvCrfE+Xs3ywTA35pvR_NyFyXLihyAuFFZBA4aHmiZBg@mail.gmail.com>
Date: Thu, 6 Feb 2025 16:43:27 +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, horms@...nel.org, bpf@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v8 05/12] net-timestamp: prepare for isolating
 two modes of SO_TIMESTAMPING

On Wed, Feb 5, 2025 at 11:34 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > No functional changes here, only add skb_enable_app_tstamp() to test
> > if the orig_skb matches the usage of application SO_TIMESTAMPING
> > or its bpf extension. And it's good to support two modes in
> > parallel later in this series.
> >
> > Also, this patch deliberately distinguish 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.
>
> Disagree. Don't add a crutch that has not shown to be necessary for
> all this time.
>
> Just infer hw from hwtstamps != NULL.

I can surely modify this part as you said, but may I ask why? I cannot
find a good reason to absolutely trust the hardware behaviour. If that
corner case happens, it would be very hard to trace the root cause...

>
> > 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
>
> Unfortunately this cannot be modified to skb_tstamp_tx_hw, as that
> would require updating way too many callers.

I didn't change the name, only the description and usage of
skb_tstamp_tx(). It always gets called in the hardware timestamp
situation except skb_tx_timestamp() that is modified.

Thanks,
Jason