netdev - Re: [PATCH net-next v4 3/4] net-timestamp: extend SOF_TIMESTAMPING_OPT_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: <66d9b5db807aa_18ac2129468@willemb.c.googlers.com.notmuch>
Date: Thu, 05 Sep 2024 09:44:59 -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, 
 dsahern@...nel.org, 
 willemdebruijn.kernel@...il.com, 
 shuah@...nel.org, 
 willemb@...gle.com
Cc: linux-kselftest@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v4 3/4] net-timestamp: extend
 SOF_TIMESTAMPING_OPT_RX_FILTER for hardware use

Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> In the previous patch, we found things could happen in the rx software
> timestamp. Here, we also noticed that, for rx hardware timestamp case,
> it could happen when one process enables the rx hardware timestamp
> generating flag first, then another process only setting
> SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware
> timestamp.
> 
> In this patch, we extend the OPT_RX_FILTER flag to filter out the
> above case for hardware use.
> 
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
> Link: https://lore.kernel.org/all/20240903121940.6390b958@kernel.org/
> ---
>  Documentation/networking/timestamping.rst | 15 +++++++++------
>  net/core/sock.c                           |  5 +++--
>  net/ipv4/tcp.c                            |  3 ++-
>  net/socket.c                              |  3 ++-
>  4 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index ac57d9de2f11..55e79ea71f3e 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW:
>    each containing just one timestamp.
>  
>  SOF_TIMESTAMPING_OPT_RX_FILTER:
> -  Used in the receive software timestamp. Enabling the flag along with
> -  SOF_TIMESTAMPING_SOFTWARE will not report the rx timestamp to the
> -  userspace so that it can filter out the case where one process starts
> -  first which turns on netstamp_needed_key through setting generation
> -  flags like SOF_TIMESTAMPING_RX_SOFTWARE, then another one only passing
> -  SOF_TIMESTAMPING_SOFTWARE report flag could also get the rx timestamp.
> +  Used in the receive software/hardware timestamp. Enabling the flag
> +  along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
> +  will not report the rx timestamp to the userspace so that it can
> +  filter out the cases where 1) one process starts first which turns
> +  on netstamp_needed_key through setting generation flags like
> +  SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
> +  generating the hardware timestamp already, then another one only
> +  passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
> +  rx timestamp.

I think this patch should be squashed into patch 1.

Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors
across its lifetime. Even if it is only two SHA1s apart.

It also avoids such duplicate changes to the same code/text blocks.

More importantly, it matters for the behavior, see below.

>  
>    SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being
>    influenced by others and let the application choose whether to report
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 6a93344e21cf..dc4a43cfff59 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -908,8 +908,9 @@ int sock_set_timestamping(struct sock *sk, int optname,
>  	    !(val & SOF_TIMESTAMPING_OPT_ID))
>  		return -EINVAL;
>  
> -	if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> -	    val & SOF_TIMESTAMPING_OPT_RX_FILTER)
> +	if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&
> +	    (val & SOF_TIMESTAMPING_RX_SOFTWARE ||
> +	     val & SOF_TIMESTAMPING_RX_HARDWARE))
>  		return -EINVAL;

There may be legitimate use cases of wanting to receive hardware
receive timestamps, plus software transmit timestamp, but
suppress spurious software timestamps (or vice versa):

    SOF_TIMESTAMPING_RAW_HARDWARE | \
    SOF_TIMESTAMPING_RX_HARDWARE | \
    SOF_TIMESTAMPING_SOFTWARE | \
    SOF_TIMESTAMPING_TX_SOFTWARE | \
    SOF_TIMESTAMPING_OPT_RX_FILTER

Admittedly this seems a bit contrived. But it's little hassle to
support it?

We just can no longer use the branch simplification that Jakub
pointed out.