netdev - Re: [PATCH net-next] net: thunder: make tx software timestamp independent

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: <681b5ebc80a81_1e440629460@willemb.c.googlers.com.notmuch>
Date: Wed, 07 May 2025 09:23:08 -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, 
 horms@...nel.org, 
 sgoutham@...vell.com, 
 andrew+netdev@...n.ch, 
 willemb@...gle.com
Cc: linux-arm-kernel@...ts.infradead.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] net: thunder: make tx software timestamp
 independent

Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> skb_tx_timestamp() is used for tx software timestamp enabled by
> SOF_TIMESTAMPING_TX_SOFTWARE while SKBTX_HW_TSTAMP is controlled by
> SOF_TIMESTAMPING_TX_HARDWARE. As it clearly shows they are different
> timestamps in two dimensions, this patch makes the software one
> standalone.
> 
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
>  drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> index 06397cc8bb36..d368f381b6de 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> @@ -1389,11 +1389,11 @@ nicvf_sq_add_hdr_subdesc(struct nicvf *nic, struct snd_queue *sq, int qentry,
>  		this_cpu_inc(nic->pnicvf->drv_stats->tx_tso);
>  	}
>  
> +	skb_tx_timestamp(skb);
> +
>  	/* Check if timestamp is requested */

Nit: check if hw timestamp is requested.

> -	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
> -		skb_tx_timestamp(skb);
> +	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
>  		return;
> -	}

The SO_TIMESTAMPING behavior around both software and hardware
timestamps is a bit odd.

Unless SOF_TIMESTAMPING_OPT_TX_SWHW is set, by default a driver will
only return software if no hardware timestamp is also requested.

Through the following in __skb_tstamp_tx

        if (!hwtstamps && !(tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
            skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
                return;

There really is no good reason to have this dependency. But it is
historical and all drivers should implement the same behavior.

This automatically happens if the software timestamp request
skb_tx_timestamp is called after the hardware timestamp request
is configured, i.e., after SKBTX_IN_PROGRESS is set. That usually
happens because the software timestamp is requests as close to kicking
the doorbell as possible.

In this driver, that would be not in nicvf_sq_add_hdr_subdesc, but
just before calling nicvf_sq_doorbell. Unfortunately, there are two
callers, TSO and non-TSO.

>  
>  	/* Tx timestamping not supported along with TSO, so ignore request */
>  	if (skb_shinfo(skb)->gso_size)
> -- 
> 2.43.5
>