Re: [PATCH net-next v3 1/4] net_tstamp: add SCM_TS_OPT_ID to provide OPT_ID in control message

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Vadim Fedorenko wrote:
> SOF_TIMESTAMPING_OPT_ID socket option flag gives a way to correlate TX
> timestamps and packets sent via socket. Unfortunately, there is no way
> to reliably predict socket timestamp ID value in case of error returned
> by sendmsg. For UDP sockets it's impossible because of lockless
> nature of UDP transmit, several threads may send packets in parallel. In
> case of RAW sockets MSG_MORE option makes things complicated. More
> details are in the conversation [1].
> This patch adds new control message type to give user-space
> software an opportunity to control the mapping between packets and
> values by providing ID with each sendmsg for UDP sockets.
> The documentation is also added in this patch.
> 
> [1] https://lore.kernel.org/netdev/CALCETrU0jB+kg0mhV6A8mrHfTE1D1pr1SD_B9Eaa9aDPfgHdtA@mail.gmail.com/
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>  Documentation/networking/timestamping.rst | 13 +++++++++++++
>  arch/alpha/include/uapi/asm/socket.h      |  2 ++
>  arch/mips/include/uapi/asm/socket.h       |  2 ++
>  arch/parisc/include/uapi/asm/socket.h     |  2 ++
>  arch/sparc/include/uapi/asm/socket.h      |  2 ++
>  include/net/inet_sock.h                   |  4 +++-
>  include/net/sock.h                        |  2 ++
>  include/uapi/asm-generic/socket.h         |  2 ++
>  include/uapi/linux/net_tstamp.h           |  7 +++++++
>  net/core/sock.c                           |  9 +++++++++
>  net/ipv4/ip_output.c                      | 18 +++++++++++++-----
>  net/ipv6/ip6_output.c                     | 18 +++++++++++++-----
>  12 files changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index a2c66b3d7f0f..1c38536350e7 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -38,6 +38,13 @@ enum {
>  				 SOF_TIMESTAMPING_LAST
>  };
>  
> +/*
> + * The highest bit of sk_tsflags is reserved for kernel-internal
> + * SOCKCM_FLAG_TS_OPT_ID. This check is to control that SOF_TIMESTAMPING*
> + * values do not reach this reserved area
> + */
> +static_assert(SOF_TIMESTAMPING_LAST != (1 << 31));

Let's not leak any if this implementation detail to include/uapi.

A BUILD_BUG_ON wherever SOCKCM_FLAG_TS_OPT_ID is used, such as in case
SCM_TS_OPT_ID, should work.

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index eea443b7f65e..bd2f6a699470 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -973,7 +973,7 @@ static int __ip_append_data(struct sock *sk,
>  	unsigned int maxfraglen, fragheaderlen, maxnonfragsize;
>  	int csummode = CHECKSUM_NONE;
>  	struct rtable *rt = dst_rtable(cork->dst);
> -	bool paged, hold_tskey, extra_uref = false;
> +	bool paged, hold_tskey = false, extra_uref = false;
>  	unsigned int wmem_alloc_delta = 0;
>  	u32 tskey = 0;
>  
> @@ -1049,10 +1049,15 @@ static int __ip_append_data(struct sock *sk,
>  
>  	cork->length += length;
>  
> -	hold_tskey = cork->tx_flags & SKBTX_ANY_TSTAMP &&
> -		     READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_OPT_ID;
> -	if (hold_tskey)
> -		tskey = atomic_inc_return(&sk->sk_tskey) - 1;
> +	if (cork->tx_flags & SKBTX_ANY_TSTAMP &&
> +	    READ_ONCE(sk->sk_tsflags) & SOCKCM_FLAG_TS_OPT_ID) {

s/SOCKCM_FLAG_TS_OPT_ID/SOF_TIMESTAMPING_OPT_ID/

> +		if (cork->flags & IPCORK_TS_OPT_ID) {
> +			tskey = cork->ts_opt_id;
> +		} else {
> +			tskey = atomic_inc_return(&sk->sk_tskey) - 1;
> +			hold_tskey = true;
> +		}
> +	}
>  
>  	/* So, what's going on in the loop below?
>  	 *
> @@ -1325,8 +1330,11 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
>  	cork->mark = ipc->sockc.mark;
>  	cork->priority = ipc->priority;
>  	cork->transmit_time = ipc->sockc.transmit_time;
> +	cork->ts_opt_id = ipc->sockc.ts_opt_id;
>  	cork->tx_flags = 0;
>  	sock_tx_timestamp(sk, ipc->sockc.tsflags, &cork->tx_flags);
> +	if (ipc->sockc.tsflags & SOCKCM_FLAG_TS_OPT_ID)
> +		cork->flags |= IPCORK_TS_OPT_ID;

We can move initialization of ts_opt_id into the branch.

Or conversely avoid the branch with some convoluted shift operations
to have the rval be either 1 << 1 or 0 << 1. But let's do the simpler
thing.