netdev - Re: [PATCH net-next 1/2] tcp: make SOF_TIMESTAMPING_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] [day] [month] [year] [list]

Message-ID: <CAL+tcoCuPcpMf1Z3aT-TaEoo+Dp1tzg5YCuxcqLkf5tki0PqLg@mail.gmail.com>
Date: Wed, 28 Aug 2024 22:18: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, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 1/2] tcp: make SOF_TIMESTAMPING_RX_SOFTWARE
 feature per socket

On Wed, Aug 28, 2024 at 10:10 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > On Wed, Aug 28, 2024 at 9:44 PM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > > > > I can see what you mean here: you don't like combining the reporting
> > > > > > flag and generation flag, right? But If we don't check whether those
> > > > > > two flags (SOF_TIMESTAMPING_RX_SOFTWARE __and__
> > > > > > SOF_TIMESTAMPING_SOFTWARE) in sock_recv_timestamp(), some tests in the
> > > > > > protocols like udp will fail as we talked before.
> > > > > >
> > > > > > netstamp_needed_key cannot be implemented as per socket feature (at
> > > > > > that time when the driver just pass the skb to the rx stack, we don't
> > > > > > know which socket the skb belongs to). Since we cannot prevent this
> > > > > > from happening during its generation period, I suppose we can delay
> > > > > > the check and try to stop it when it has to report, I mean, in
> > > > > > sock_recv_timestamp().
> > > > > >
> > > > > > Or am I missing something? What would you suggest?
> > > > > >
> > > > > > >
> > > > > > >         /*
> > > > > > >          * generate control messages if
> > > > > > >          * - receive time stamping in software requested
> > > > > > >          * - software time stamp available and wanted
> > > > > > >          * - hardware time stamps available and wanted
> > > > > > >          */
> > > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > > > > > >             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > > > > > >             (hwtstamps->hwtstamp &&
> > > > > > >              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > > > > > >                 __sock_recv_timestamp(msg, sk, skb);
> > > > > > >
> > > > > > > I evidently already noticed this back in 2014, when I left a note in
> > > > > > > commit b9f40e21ef42 ("net-timestamp: move timestamp flags out of
> > > > > > > sk_flags"):
> > > > > > >
> > > > > > >     SOCK_TIMESTAMPING_RX_SOFTWARE is also used to toggle the receive
> > > > > > >     timestamp logic (netstamp_needed). That can be simplified and this
> > > > > > >     last key removed, but will leave that for a separate patch.
> > > > > > >
> > > > > > > But I do not see __sock_recv_timestamp toggling the feature either
> > > > > > > then or now, so I think this is vestigial and can be removed.
> > > >
> > > > After investigating more of it, as your previous commit said, the
> > > > legacy SOCK_TIMESTAMPING_RX_SOFTWARE flag can be replaced by
> > > > SOF_TIMESTAMPING_RX_SOFTWARE and we can completely remove that SOCK_xx
> > > > flag from enum sock_flags {}, right? Do you expect me to do that? If
> > > > so, I would love to do it :)
> > >
> > > I did not say that. I said that the specific line here appears
> > > vestigial.
> > >
> > > > > > >         if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > > > > > >             (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> >
> > Thanks for your patience.
> >
> > I think I will remove the above two lines as one patch with your
> > suggested-by tag in the series so that later we can easily review each
> > of them.
>
> Please don't. As I said right below this:
>
> One thing at a time. Let's focus on the change you proposed to me.
>
> The above is just a hunch on first read. I would have to spend more
> time to convince myself that it is indeed correct and safe. At which
> point I might as well send it myself too..
>
> More importantly, let's not expand what is intended to be a stand
> alone improvement with tangential changes. It just makes reviewing
> harder and slows it down.

I got it :)

I think I will not touch the __sock_recv_timestamp() function and then
move those test statements from sock_recv_timestamp() to
__sock_recv_timestamp() in patch [2/2].

Let me send a v2 soon to see if they are correct.

Thanks,
Jason