netdev - Re: [PATCH net-next v3 2/4] net_tstamp: add SCM_TS_OPT

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: <a3ccb46c-7493-4a6d-8792-bb5ccd19bce3@linux.dev>
Date: Fri, 6 Sep 2024 18:27:51 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Willem de Bruijn <willemb@...gle.com>
Cc: netdev@...r.kernel.org, David Ahern <dsahern@...nel.org>,
 Jason Xing <kerneljasonxing@...il.com>, Jakub Kicinski <kuba@...nel.org>,
 Simon Horman <horms@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v3 2/4] net_tstamp: add SCM_TS_OPT_ID for TCP
 sockets

On 06/09/2024 17:33, Willem de Bruijn wrote:
> Willem de Bruijn wrote:
>> Vadim Fedorenko wrote:
>>> On 05/09/2024 17:39, Willem de Bruijn wrote:
>>>> Vadim Fedorenko wrote:
>>>>> On 04/09/2024 12:31, Vadim Fedorenko wrote:
>>>>>> TCP sockets have different flow for providing timestamp OPT_ID value.
>>>>>> Adjust the code to support SCM_TS_OPT_ID option for TCP sockets.
>>>>>>
>>>>>> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
>>>>>> ---
>>>>>>     net/ipv4/tcp.c | 13 +++++++++----
>>>>>>     1 file changed, 9 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>>>>> index 8a5680b4e786..5553a8aeee80 100644
>>>>>> --- a/net/ipv4/tcp.c
>>>>>> +++ b/net/ipv4/tcp.c
>>>>>> @@ -474,9 +474,10 @@ void tcp_init_sock(struct sock *sk)
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(tcp_init_sock);
>>>>>>     
>>>>>> -static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
>>>>>> +static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
>>>>>>     {
>>>>>>     	struct sk_buff *skb = tcp_write_queue_tail(sk);
>>>>>> +	u32 tsflags = sockc->tsflags;
>>>>>>     
>>>>>>     	if (tsflags && skb) {
>>>>>>     		struct skb_shared_info *shinfo = skb_shinfo(skb);
>>>>>> @@ -485,8 +486,12 @@ static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
>>>>>>     		sock_tx_timestamp(sk, tsflags, &shinfo->tx_flags);
>>>>>>     		if (tsflags & SOF_TIMESTAMPING_TX_ACK)
>>>>>>     			tcb->txstamp_ack = 1;
>>>>>> -		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
>>>>>> -			shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
>>>>>> +		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) {
>>>>>> +			if (tsflags & SOCKCM_FLAG_TS_OPT_ID)
>>>>>> +				shinfo->tskey = sockc->ts_opt_id;
>>>>>> +			else
>>>>>> +				shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
>>>>>> +		}
>>>>>>     	}
>>>>>>     }
>>>>>>     
>>>>>> @@ -1318,7 +1323,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>>>>>>     
>>>>>>     out:
>>>>>>     	if (copied) {
>>>>>> -		tcp_tx_timestamp(sk, sockc.tsflags);
>>>>>> +		tcp_tx_timestamp(sk, &sockc);
>>>>>>     		tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
>>>>>>     	}
>>>>>>     out_nopush:
>>>>>
>>>>> Hi Willem,
>>>>>
>>>>> Unfortunately, these changes are not enough to enable custom OPT_ID for
>>>>> TCP sockets. There are some functions which rewrite shinfo->tskey in TCP
>>>>> flow:
>>>>>
>>>>> tcp_skb_collapse_tstamp()
>>>>> tcp_fragment_tstamp()
>>>>> tcp_gso_tstamp()
>>>>>
>>>>> I believe the last one breaks tests, but the problem is that there is no
>>>>> easy way to provide the flag of constant tskey to it. Only
>>>>> shinfo::tx_flags are available at the caller side and we have already
>>>>> discussed that we shouldn't use the last bit of this field.
>>>>>
>>>>> So, how should we deal with the problem? Or is it better to postpone
>>>>> support for TCP sockets in this case?
>>>>
>>>> Are you sure that this is a problem. These functions pass on the
>>>> skb_shinfo(skb)->ts_key from one skb to another.
>>>
>>> Yes, you are right, the problem is in a different place.
>>>
>>> __skb_complete_tx_timestamp receives skb with shinfo->tskey equal to
>>> provided by cmsg. But for TCP sockets it unconditionally adjusts ee_data
>>> value:
>>>
>>> 	if (sk_is_tcp(sk))
>>> 		serr->ee.ee_data -= atomic_read(&sk->sk_tskey);
>>>
>>> It happens because of assumption that for TCP sockets shinfo::tskey will
>>> have sequence number and the logic has to recalculate it back to the
>>> bytes sent. The same logic exists in all TCP TX timestamping functions
>>> (mentioned in the previous mail) and may trigger some unexpected
>>> behavior. To fix the issue we have to provide some kind of signal that
>>> tskey value is provided from user-space and shouldn't be changed. And we
>>> have to have it somewhere in skb info. Again, tx_flags looks like the
>>> best candidate, but it's impossible to use. I'm thinking of using
>>> special flag in tcp_skb_cb - gonna test more, but open for other
>>> suggestions.
>>
>> Ai, that is tricky. tx_flags is full/scarce indeed.
>>
>> CB does not persist as the skb transitions between layers.
> 
> Though specifically for TCP, it is possible to look up the fast
> clone on the rtx queue, whose tcp_skb_cb will be unperturbed. But
> the tcb currently does not have this data either.

It will work fine for software timestamps, but we cannot do the same
trick in case of HW timestamps, right?