Re: [RFC PATCH bpf-next v6 2/3] net: Add additional bit to support clockid_t timestamp type

["Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>]

On 5/7/2024 4:39 AM, Willem de Bruijn wrote:
> Martin KaFai Lau wrote:
>> On 5/3/24 8:13 PM, Abhishek Chauhan wrote:
>>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
>>> index fe86cadfa85b..c3d852eecb01 100644
>>> --- a/net/ipv4/ip_output.c
>>> +++ b/net/ipv4/ip_output.c
>>> @@ -1457,7 +1457,10 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
>>>   
>>>   	skb->priority = (cork->tos != -1) ? cork->priority: READ_ONCE(sk->sk_priority);
>>>   	skb->mark = cork->mark;
>>> -	skb->tstamp = cork->transmit_time;
>>> +	if (sk_is_tcp(sk))
>>
>> This seems not catching all IPPROTO_TCP case. In particular, the percpu 
>> "ipv4_tcp_sk" is SOCK_RAW. sk_is_tcp() is checking SOCK_STREAM:
>>
>> void __init tcp_v4_init(void)
>> {
>>
>> 	/* ... */
>> 	res = inet_ctl_sock_create(&sk, PF_INET, SOCK_RAW,
>> 				   IPPROTO_TCP, &init_net);
>>
>> 	/* ... */
>> }
>>
>> "while :; do ./test_progs -t tc_redirect/tc_redirect_dtime || break; done" 
>> failed pretty often exactly in this case.
>>
> 
> Interesting. The TCP stack opens non TCP sockets.
> 
> Initializing sk->sk_clockid for this socket should address that.
> 
Willem, Are you suggesting your point from the previous patch ? 

"I think we want to avoid special casing if we can. Note the if.

If TCP always uses monotonic, we could consider initializing
sk_clockid to CLOCK_MONONOTIC in tcp_init_sock.

I guess TCP logic currently entirely ignores sk_clockid. If we are to
start using this, then setsocktop SO_TXTIME must explicitly fail or
ignore for TCP sockets, or silently skip the write.

All of that is more complexity. Than is maybe warranted for this one
case. So no objections from me to special casing using sk_is_tcp(sk)
either." 

Few places we need to initialize the clock base for tcp to monotonic 
1. tcp_init_sock 
2. void __init tcp_v4_init(void) in tcp_ipv4.c
3. static int __net_init tcpv6_net_init(struct net *net)
4. Ignore setsockopts for SO_TXTIME if the sk->protocol is tcp.  

Is it safe to assume the TCP will never use any other close base ? 


OR 


For now we can do just protocol level check in ip_make_skb and ip6_make_skb 
like 
if (iph->protocol == IPPROTO_TCP)
    /* ... */
else
    /* ... */