Received-SPF: pass (google.com: domain of linux-kernel+bounces-193008-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <c7d66e70-0998-4e2b-94e8-8b19e65ec959@quicinc.com>
Date: Tue, 28 May 2024 13:11:58 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH bpf-next v8 1/3] net: Rename mono_delivery_time to
 tstamp_type for scalabilty
Content-Language: en-US
To: Martin KaFai Lau <martin.lau@linux.dev>,
        Willem de Bruijn
	<willemdebruijn.kernel@gmail.com>
CC: <kernel@quicinc.com>, Willem de Bruijn <willemb@google.com>,
        "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>
References: <20240509211834.3235191-1-quic_abchauha@quicinc.com>
 <20240509211834.3235191-2-quic_abchauha@quicinc.com>
 <6bdba7b6-fd22-4ea5-a356-12268674def1@quicinc.com>
 <665613536e82e_2a1fb929437@willemb.c.googlers.com.notmuch>
 <d1c18889-ef48-4cb8-8b81-474b3b7ddd81@linux.dev>
From: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>
In-Reply-To: <d1c18889-ef48-4cb8-8b81-474b3b7ddd81@linux.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit


On 5/28/2024 12:30 PM, Martin KaFai Lau wrote:
> On 5/28/24 10:24 AM, Willem de Bruijn wrote:
>> Abhishek Chauhan (ABC) wrote:
>>
>>>> +static inline void skb_set_delivery_type_by_clockid(struct sk_buff *skb,
>>>> +                            ktime_t kt, clockid_t clockid)
>>>> +{
>>>> +    u8 tstamp_type = SKB_CLOCK_REALTIME;
>>>> +
>>>> +    switch (clockid) {
>>>> +    case CLOCK_REALTIME:
>>>> +        break;
>>>> +    case CLOCK_MONOTONIC:
>>>> +        tstamp_type = SKB_CLOCK_MONOTONIC;
>>>> +        break;
>>>> +    default:
>>>
>>> Willem and Martin, I was thinking we should remove this warn_on_once from below line. Some systems also use panic on warn.
>>> So i think this might result in unnecessary crashes.
>>>
>>> Let me know what you think.
>>>
>>> Logs which are complaining.
>>> https://syzkaller.appspot.com/x/log.txt?x=118c3ae8980000
>>
>> I received reports too. Agreed that we need to fix these reports.
>>
>> The alternative is to limit sk_clockid to supported ones, by failing
>> setsockopt SO_TXTIME on an unsupported clock.
>>
>> That changes established ABI behavior. But I don't see how another
>> clock can be used in any realistic way anyway.
>>
>> Putting it out there as an option. It's riskier, but in the end I
>> believe a better fix than just allowing this state to continue.
> 
> Failing early would be my preference also. The current ABI is arguably at least confusing (if not broken) considering other clockid is silently ignored by the kernel.
> 

Okay since we all agree to fix the setsockopt SO_TXTIME  to only limit sk_clockid be set to supported ones (MONO/TAI/REAL).
All other clocks needs to return -EINVAL for setsockopt. 

I will raise the patch and also add the fixes and reported-by tag accordingly. 

>>
>> A third option would be to not fail the system call, but silently
>> fall back to CLOCK_REALTIME. Essentially what happens in the datapath
>> in skb_set_delivery_type_by_clockid now. That is surprising behavior,
>> we should not do that.
> 
> Not sure if it makes sense to go back to this option only after there is breakage report with a legit usage?
>