Received-SPF: pass (google.com: domain of linux-kernel+bounces-172071-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <69c55618-931a-48f4-a25c-0d5666bcb5cb@quicinc.com>
Date: Tue, 7 May 2024 12:38:51 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH bpf-next v6 2/3] net: Add additional bit to support
 clockid_t timestamp type
Content-Language: en-US
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
        Martin KaFai Lau
	<martin.lau@linux.dev>
CC: "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni
	<pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, Andrew Halaney <ahalaney@redhat.com>,
        "Martin
 KaFai Lau" <martin.lau@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>, bpf <bpf@vger.kernel.org>,
        <kernel@quicinc.com>
References: <20240504031331.2737365-1-quic_abchauha@quicinc.com>
 <20240504031331.2737365-3-quic_abchauha@quicinc.com>
 <cab0c7ba-90bf-49e2-908d-ecd879160667@linux.dev>
 <663a12f089b81_726ea29426@willemb.c.googlers.com.notmuch>
 <fc8334a6-6961-41f4-affc-28bdfc3dd697@quicinc.com>
 <663a7e7e1f5b5_cc75c294c0@willemb.c.googlers.com.notmuch>
From: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>
In-Reply-To: <663a7e7e1f5b5_cc75c294c0@willemb.c.googlers.com.notmuch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 5/7/2024 12:18 PM, Willem de Bruijn wrote:
> Abhishek Chauhan (ABC) wrote:
>>
>>
>> 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 ? 
>>
> 
> No, just for this custom socket to initialize the sk_clockid. It is
> not a TCP socket, but only used by TCP.
Thanks Willem, 
Noted! Which means there are only two places these custom RAW tcp socket 
are getting called 

1. tcp_ipv4.c 
2. tcp_ipv6.c 

I will take care of initializing sk_clockid to monotonic in the next patch 
in the above two files. 

Let me know if i missed out anything.