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: <66d8ce15415ec_163d93294a2@willemb.c.googlers.com.notmuch>
Date: Wed, 04 Sep 2024 17:16:05 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 shuah@...nel.org, 
 willemdebruijn.kernel@...il.com
Cc: 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

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.

> 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

> 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.

> ---
>  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).

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.

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).

>  }
>  
>  static int recv_errmsg(int fd)
> -- 
> 2.37.3
>