netdev - Re: [PATCH net-next] selftests: return failure when timestamps can't be parsed

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+tcoBhPiqSv1kZmyQRBi5KyNh5qyC7x2KENg+mHpY2D_wYYw@mail.gmail.com>
Date: Thu, 5 Sep 2024 05:49:32 +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, shuah@...nel.org, linux-kselftest@...r.kernel.org, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] selftests: return failure when timestamps can't
 be parsed

On Thu, Sep 5, 2024 at 5:16 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > When I was trying to modify the tx timestamping feature, I found that
> > running "./txtimestamp -4 -C -L 127.0.0.1" didn't reflect the fact
> > properly.
>
> Did not reflect what fact? Sorry, I don't entirely follow the issue
> you raise.

I was trying to say if someone writes a bug in the timestamping
feature, the selftest probably returns pass value instead of failure.

I will explicitly report the case I met.

>
> > In this selftest file, we respectively test three tx generation flags.
> > With the generation and report flag enabled, we expect that the timestamp
> > must be returned to the userspace unless 1) generating the timestamp
> > fails, 2) reporting the timestamp fails. So we should test if the
> > timestamps can be read and parsed succuessfuly in txtimestamp.c, or
>
> typo: successfully

Will update it.

>
> > else there is a bug in the kernel.
> >
> > After adding the check so that running ./txtimestamp will reflect the
> > result correctly like this if there is an error in kernel:
> > protocol:     TCP
> > payload:      10
> > server port:  9000
> >
> > family:       INET
> > test SND
> >     USR: 1725458477 s 667997 us (seq=0, len=0)
> > Failed to parse timestamps
> >     USR: 1725458477 s 718128 us (seq=0, len=0)
> > Failed to parse timestamps
> >     USR: 1725458477 s 768273 us (seq=0, len=0)
> > Failed to parse timestamps
> >     USR: 1725458477 s 818416 us (seq=0, len=0)
> > Failed to parse timestamps
> > ...
> >
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
> > I'm not sure if I should also check if the cur->tv_sec or cur->tv_nsec
> > is zero in __print_timestamp(). Could it be valid when either of
> > them is zero?
>
> tv_nsec can be zero. tv_sec cannot.

Thanks. Now I am learning :)

>
> > ---
> >  tools/testing/selftests/net/txtimestamp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/tools/testing/selftests/net/txtimestamp.c b/tools/testing/selftests/net/txtimestamp.c
> > index ec60a16c9307..b69aae840a67 100644
> > --- a/tools/testing/selftests/net/txtimestamp.c
> > +++ b/tools/testing/selftests/net/txtimestamp.c
> > @@ -358,6 +358,10 @@ static void __recv_errmsg_cmsg(struct msghdr *msg, int payload_len)
> >
> >       if (batch > 1)
> >               fprintf(stderr, "batched %d timestamps\n", batch);
> > +     else if (!batch) {
> > +             fprintf(stderr, "Failed to parse timestamps\n");
> > +             test_failed = true;
> > +     }
>
> nit: if adding braces around one side of a branch, then add to both (all).

I see.

>
> This is not so much a parsing failure as that no timestamps arrived.
>
> More importantly, this function gets called also if
> recvmsg(fd, .., MSG_ERRQUEUE) returned 0:
>
>         if (ret >= 0) {
>                 __recv_errmsg_cmsg(&msg, ret);
>
> That seems counterintuitive, as there is no data. But this was
> introduced with cfg_loop_nodata (SOF_TIMESTAMPING_OPT_TSONLY). When
> there may be packets looped, just 0B packets. In those cases we also
> expect timestamps.

Right, It does make sense.

>
> But, can __recv_errmsg_cmsg now also be called when there truly is
> nothing on the error queue? It is a non-blocking read, after all.
>
> Judging from
>
>                 while (!recv_errmsg(fd)) {}
>
> The caller can. But if there is nothing waiting it returns -1 with
> EAGAIN:
>
>         ret = recvmsg(fd, &msg, MSG_ERRQUEUE);
>         if (ret == -1 && errno != EAGAIN)
>                 error(1, errno, "recvmsg");
>
> So long story short, subject to a few nits your patch sounds okay to
> me (but it's not entirely trivial that that is so: sharing so that you
> also double check, thanks).

Thanks for pointing out this one. I will rewrite this patch/patch
series tomorrow with your questions resolved.

Thanks,
Jason