netdev - Re: [PATCH net-next v4 3/4] net-timestamp: extend SOF_TIMESTAMPING_OPT_RX

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+tcoBPJCP1yUTCe3CLDjBR6rd2BphxTV0bPMdXhh7_CAfOtA@mail.gmail.com>
Date: Thu, 5 Sep 2024 21:57:13 +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, shuah@...nel.org, willemb@...gle.com, 
	linux-kselftest@...r.kernel.org, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v4 3/4] net-timestamp: extend SOF_TIMESTAMPING_OPT_RX_FILTER
 for hardware use

On Thu, Sep 5, 2024 at 9:45 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > In the previous patch, we found things could happen in the rx software
> > timestamp. Here, we also noticed that, for rx hardware timestamp case,
> > it could happen when one process enables the rx hardware timestamp
> > generating flag first, then another process only setting
> > SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware
> > timestamp.
> >
> > In this patch, we extend the OPT_RX_FILTER flag to filter out the
> > above case for hardware use.
> >
> > Suggested-by: Jakub Kicinski <kuba@...nel.org>
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> > Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
> > ---
> >  Documentation/networking/timestamping.rst | 15 +++++++++------
> >  net/core/sock.c                           |  5 +++--
> >  net/ipv4/tcp.c                            |  3 ++-
> >  net/socket.c                              |  3 ++-
> >  4 files changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > index ac57d9de2f11..55e79ea71f3e 100644
> > --- a/Documentation/networking/timestamping.rst
> > +++ b/Documentation/networking/timestamping.rst
> > @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW:
> >    each containing just one timestamp.
> >
> >  SOF_TIMESTAMPING_OPT_RX_FILTER:
> > -  Used in the receive software timestamp. Enabling the flag along with
> > -  SOF_TIMESTAMPING_SOFTWARE will not report the rx timestamp to the
> > -  userspace so that it can filter out the case where one process starts
> > -  first which turns on netstamp_needed_key through setting generation
> > -  flags like SOF_TIMESTAMPING_RX_SOFTWARE, then another one only passing
> > -  SOF_TIMESTAMPING_SOFTWARE report flag could also get the rx timestamp.
> > +  Used in the receive software/hardware timestamp. Enabling the flag
> > +  along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
> > +  will not report the rx timestamp to the userspace so that it can
> > +  filter out the cases where 1) one process starts first which turns
> > +  on netstamp_needed_key through setting generation flags like
> > +  SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
> > +  generating the hardware timestamp already, then another one only
> > +  passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
> > +  rx timestamp.
>
> I think this patch should be squashed into patch 1.
>
> Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors
> across its lifetime. Even if it is only two SHA1s apart.

I thought about last night as well. Like the patch [2/4] and this
patch, the reason why I wanted to split is because I have to explain a
lot for both hw and sw in one patch. One patch mixes different things.

No strong preference. If you still think so, I definitely can squash
them as you said :)

>
> It also avoids such duplicate changes to the same code/text blocks.
>
> More importantly, it matters for the behavior, see below.
>
> >
> >    SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being
> >    influenced by others and let the application choose whether to report
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 6a93344e21cf..dc4a43cfff59 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -908,8 +908,9 @@ int sock_set_timestamping(struct sock *sk, int optname,
> >           !(val & SOF_TIMESTAMPING_OPT_ID))
> >               return -EINVAL;
> >
> > -     if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > -         val & SOF_TIMESTAMPING_OPT_RX_FILTER)
> > +     if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&
> > +         (val & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > +          val & SOF_TIMESTAMPING_RX_HARDWARE))
> >               return -EINVAL;
>
> There may be legitimate use cases of wanting to receive hardware
> receive timestamps, plus software transmit timestamp, but
> suppress spurious software timestamps (or vice versa):
>
>     SOF_TIMESTAMPING_RAW_HARDWARE | \
>     SOF_TIMESTAMPING_RX_HARDWARE | \
>     SOF_TIMESTAMPING_SOFTWARE | \
>     SOF_TIMESTAMPING_TX_SOFTWARE | \
>     SOF_TIMESTAMPING_OPT_RX_FILTER

Oh, right, it can happen! RAW_HARDWARE is a little bit different,
covering both ingress and egress path.

>
> Admittedly this seems a bit contrived. But it's little hassle to
> support it?
>
> We just can no longer use the branch simplification that Jakub
> pointed out.
>

I see. I'm going to do two things as you said:
1) restore the simplification branch
2) only take care of software case in sock_set_timestamping()

Thanks for pointing this out!