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

Message-ID: <CAL+tcoASfb-EPtdpmunbo2zxpQx19Kv+b8Bzs91diVFYYqQz7Q@mail.gmail.com>
Date: Wed, 4 Sep 2024 17:14:07 +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 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.

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?

Thanks,
Jason