netdev - Re: [PATCH net-next] net: skb: free up one bit in tx

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] [day] [month] [year] [list]
Message-ID: <daab23b7-7841-407b-8c93-b801d6182cc1@engleder-embedded.com>
Date: Fri, 21 Feb 2025 22:11:57 +0100
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
 pabeni@...hat.com, kerneljasonxing@...il.com, pav@....fi,
 vinicius.gomes@...el.com, anthony.l.nguyen@...el.com,
 Willem de Bruijn <willemb@...gle.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: skb: free up one bit in tx_flags

On 21.02.25 04:58, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@...gle.com>
> 
> The linked series wants to add skb tx completion timestamps.
> That needs a bit in skb_shared_info.tx_flags, but all are in use.
> 
> A per-skb bit is only needed for features that are configured on a
> per packet basis. Per socket features can be read from sk->sk_tsflags.
> 
> Per packet tsflags can be set in sendmsg using cmsg, but only those in
> SOF_TIMESTAMPING_TX_RECORD_MASK.
> 
> Per packet tsflags can also be set without cmsg by sandwiching a
> send inbetween two setsockopts:
> 
>      val |= SOF_TIMESTAMPING_$FEATURE;
>      setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
>      write(fd, buf, sz);
>      val &= ~SOF_TIMESTAMPING_$FEATURE;
>      setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
> 
> Changing a datapath test from skb_shinfo(skb)->tx_flags to
> skb->sk->sk_tsflags can change behavior in that case, as the tx_flags
> is written before the second setsockopt updates sk_tsflags.
> 
> Therefore, only bits can be reclaimed that cannot be set by cmsg and
> are also highly unlikely to be used to target individual packets
> otherwise.
> 
> Free up the bit currently used for SKBTX_HW_TSTAMP_USE_CYCLES. This
> selects between clock and free running counter source for HW TX
> timestamps. It is probable that all packets of the same socket will
> always use the same source.

For separate ptp4l instances with separate clocks this should be true.

> Link: https://lore.kernel.org/netdev/cover.1739988644.git.pav@iki.fi/
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ---
>   drivers/net/ethernet/engleder/tsnep_main.c |  4 ++--
>   drivers/net/ethernet/intel/igc/igc_main.c  |  3 ++-
>   include/linux/skbuff.h                     |  5 ++---
>   net/socket.c                               | 11 +----------
>   4 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index 0d030cb0b21c..3de4cb06e266 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -852,8 +852,8 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>   			struct skb_shared_hwtstamps hwtstamps;
>   			u64 timestamp;
>   
> -			if (skb_shinfo(entry->skb)->tx_flags &
> -			    SKBTX_HW_TSTAMP_USE_CYCLES)
> +			if (entry->skb->sk &&
> +			    READ_ONCE(entry->skb->sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
>   				timestamp =
>   					__le64_to_cpu(entry->desc_wb->counter);
>   			else
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 84307bb7313e..0c4216a4552b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1650,7 +1650,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>   		if (igc_request_tx_tstamp(adapter, skb, &tstamp_flags)) {
>   			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>   			tx_flags |= IGC_TX_FLAGS_TSTAMP | tstamp_flags;
> -			if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES)
> +			if (skb->sk &&
> +			    READ_ONCE(skb->sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
>   				tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;
>   		} else {
>   			adapter->tx_hwtstamp_skipped++;
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index bb2b751d274a..a65b2b08f994 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -478,8 +478,8 @@ enum {
>   	/* device driver is going to provide hardware time stamp */
>   	SKBTX_IN_PROGRESS = 1 << 2,
>   
> -	/* generate hardware time stamp based on cycles if supported */
> -	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
> +	/* reserved */
> +	SKBTX_RESERVED = 1 << 3,
>   
>   	/* generate wifi status information (where possible) */
>   	SKBTX_WIFI_STATUS = 1 << 4,
> @@ -494,7 +494,6 @@ enum {
>   #define SKBTX_ANY_SW_TSTAMP	(SKBTX_SW_TSTAMP    | \
>   				 SKBTX_SCHED_TSTAMP)
>   #define SKBTX_ANY_TSTAMP	(SKBTX_HW_TSTAMP | \
> -				 SKBTX_HW_TSTAMP_USE_CYCLES | \
>   				 SKBTX_ANY_SW_TSTAMP)
>   
>   /* Definitions for flags in struct skb_shared_info */
> diff --git a/net/socket.c b/net/socket.c
> index 28bae5a94234..2e3e69710ea4 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -680,18 +680,9 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
>   {
>   	u8 flags = *tx_flags;
>   
> -	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
> +	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
>   		flags |= SKBTX_HW_TSTAMP;
>   
> -		/* PTP hardware clocks can provide a free running cycle counter
> -		 * as a time base for virtual clocks. Tell driver to use the
> -		 * free running cycle counter for timestamp if socket is bound
> -		 * to virtual clock.
> -		 */
> -		if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
> -			flags |= SKBTX_HW_TSTAMP_USE_CYCLES;
> -	}
> -
>   	if (tsflags & SOF_TIMESTAMPING_TX_SOFTWARE)
>   		flags |= SKBTX_SW_TSTAMP;
>   

Reviewed-by: Gerhard Engleder <gerhard@...leder-embedded.com>