netdev - Re: [PATCH net-next v2 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoAr4viE-1HfQN8EpmikeSFUefr9NKiXFx3ysoGRNUn=SQ@mail.gmail.com>
Date: Thu, 29 Aug 2024 23:34:04 +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 v2 1/2] tcp: make SOF_TIMESTAMPING_RX_SOFTWARE
 feature per socket

On Thu, Aug 29, 2024 at 10:16 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > Normally, if we want to record and print the rx timestamp after
> > tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
> > and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
> > through running rxtimestamp binary in selftests (see testcase 7).
> >
> > However, there is one particular case that fails the selftests with
> > "./rxtimestamp: Expected swtstamp to not be set." error printing in
> > testcase 6.
> >
> > How does it happen? When we keep running a thread starting a socket
> > and set SOF_TIMESTAMPING_RX_SOFTWARE option first, then running
> > ./rxtimestamp, it will fail. The reason is the former thread
> > switching on netstamp_needed_key that makes the feature global,
> > every skb going through netif_receive_skb_list_internal() function
> > will get a current timestamp in net_timestamp_check(). So the skb
> > will have timestamp regardless of whether its socket option has
> > SOF_TIMESTAMPING_RX_SOFTWARE or not.
> >
> > After this patch, we can pass the selftest and control each socket
> > as we want when using rx timestamp feature.
> >
> > Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> >  net/ipv4/tcp.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 8514257f4ecd..5e88c765b9a1 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -2235,6 +2235,7 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       struct scm_timestamping_internal *tss)
> >  {
> >       int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
> > +     u32 tsflags = READ_ONCE(sk->sk_tsflags);
> >       bool has_timestamping = false;
> >
> >       if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) {
> > @@ -2274,14 +2275,20 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       }
> >               }
> >
> > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > +             /* We have to use the generation flag here to test if we
> > +              * allow the corresponding application to receive the rx
> > +              * timestamp. Only using report flag does not hold for
> > +              * receive timestamping case.
> > +              */
>
> Nit: what does "does not hold" mean here? I don't think a casual reader
> will be able to parse this comment and understand it.

“hold for” can be a fixed collocation, which means "be suitable for"?
I'm not that sure. I was trying to say "only using the report flag
cannot meet our needs" something like this.

>
> Perhaps something along the lines of
>
> "Test both reporting and generation flag, to filter out false
> positives where the process asked only for tx software timestamps and
> another process enabled receive software timestamp generation."

Thanks, it's much better than mine. I will use it.

Thanks,
Jason

>
> > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> >                       has_timestamping = true;
> >               else
> >                       tss->ts[0] = (struct timespec64) {0};
> >       }
> >
> >       if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) {
> > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_RAW_HARDWARE)
> > +             if (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)
> >                       has_timestamping = true;
> >               else
> >                       tss->ts[2] = (struct timespec64) {0};
> > --
> > 2.37.3
> >
>
>