netdev - Re: [PATCH net-next v3 1/2] net-timestamp: filter out report when setting SOF_TIMESTAMPING

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] [day] [month] [year] [list]

Message-ID: <CAL+tcoD1eq-Gj1Du5U1X=pw97yT0fNvkwFb5iQeeTis4ekGbOQ@mail.gmail.com>
Date: Thu, 5 Sep 2024 05:38:36 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, willemb@...gle.com, davem@...emloft.net, 
	edumazet@...gle.com, pabeni@...hat.com, dsahern@...nel.org, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 1/2] net-timestamp: filter out report when
 setting SOF_TIMESTAMPING_SOFTWARE

On Thu, Sep 5, 2024 at 4:25 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > On Wed, Sep 4, 2024 at 6:13 AM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > Jakub Kicinski wrote:
> > > > On Fri, 30 Aug 2024 23:37:50 +0800 Jason Xing wrote:
> > > > > +   if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> > > > > +       val & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > > > +           return -EINVAL;
> > > >
> > > >
> > > > > -           if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > > > > +           if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > > +               (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > > > +                !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)))
> > > > >                     has_timestamping = true;
> > > > >             else
> > > > >                     tss->ts[0] = (struct timespec64) {0};
> > > > >     }
> > > >
> > > > >     memset(&tss, 0, sizeof(tss));
> > > > >     tsflags = READ_ONCE(sk->sk_tsflags);
> > > > > -   if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> > > > > +   if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > > > > +        (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > > > +        skb_is_err_queue(skb) ||
> > > > > +        !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER))) &&
> > > >
> > > > Willem, do you prefer to keep the:
> > > >
> > > >       tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> > > >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > >
> > > > conditions?IIUC we prevent both from being set at once. So
> > > >
> > > >       !(tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
> > > >
> > > > is sufficient (and, subjectively, more intuitive).
> > >
> > > Good point. Yes, let's definitely simplify.
> > >
> > > > Question #2 -- why are we only doing this for SW stamps?
> > > > HW stamps for TCP are also all or nothing.
> > >
> > > Fair. Else we'll inevitably add a
> > > SOF_TIMESTAMPING_OPT_RX_HARDWARE_FILTER at some point.
> > >
> > > There probably is no real use to filter one, but not the other.
> > >
> > > So SOF_TIMESTAMPING_OPT_RX_FILTER then, and also apply
> > > to the branch below:
> > >
> > >         if (shhwtstamps &&
> > >             (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > >             !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > >
> > > and same for tcp_recv_timestamp.
> >
> > When I'm looking at this part, I noticed that RAW_HARDWARE is actually
> > a tx report flag instead of rx, please also see the kdoc you wrote a
> > long time ago:
> >
> > SOF_TIMESTAMPING_RAW_HARDWARE:
> >   Report hardware timestamps as generated by
> >   SOF_TIMESTAMPING_TX_HARDWARE when available.
>
> Right, this is analogous to the software part that you modify:
>
>         if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
>             ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
>                 empty = 0;
>
> The idea is to also add for hardware timestamps your suggested
> condition that the socket also sets the timestamp generation flag
> SOF_TIMESTAMPING_RX_HARDWARE or that the new OPT_RX_FILTER flag
> is not set.
>
>
> > If so, OPT_RX_FILTER doesn't fit for the name of tx timestamp.
> >
> > I wonder if I can only revise the series with the code simplified as
> > Jakub suggested and then repost it? I think we need to choose a new
> > name for this tx hardware report case, like
> > SOF_TIMESTAMPING_OPT_TX_HARDWARE_FILTER?
> >
> > Since it belongs to the tx path, can I put it into another series or a
> > new patch in the current series where I will explicitly explain why we
> > also need to introduce this new flag?
>
> I think the confusion here comes from that comment that
> SOF_TIMESTAMPING_RAW_HARDWARE only reports
> SOF_TIMESTAMPING_TX_HARDWARE generated timestamps. This statement is
> incorrect and should be revised. It also reports
> SOF_TIMESTAMPING_RX_HARDWARE.
>
> Unless I'm missing something. But I think the author of that statement
> is the one who made the mistake. Who is.. also me.

That's all right :)

Then, the OPT_RX_FILTER is fine under this circumstance.

Let me also revise the documentation first in the series and put it
into a separate patch because the rule of writing the patch is to keep
one patch doing only one thing at one time.

Similar action will be done to the "rx hardware" part because I think
mixing the long description for both of them (software & hardware) can
be a little bit messy to readers.

Let me try a better organization of the series: keep each patch clear,
small, atomic and easy to review.

Thanks for your help!