Re: [PATCH bpf-next v8 1/3] net: Rename mono_delivery_time to tstamp_type for scalabilty

[Abhishek Chauhan (ABC)]

On 5/28/2024 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.
> 
I understand your thought process here, but i think doing this option means 
no application from userspace can use any other clocks except REALTIME, MONO and TAI.

That being said application which are using different sock options to set other clocks needs
to change and work with just REALTIME , MONO or TAI. (Meaning the above warning from google compute engine
would be gone because setsock option itself failed in the first place, I suspect here the clockid being used is 
CLOCK_BOOTTIME which is similar to CLOCK_MONOTONIC with system suspend time as well)

I feel that the options which are exposed by SO_TXTIME are limitless as of today the code 
lacks basic checks such as not checking if the userspace gave a correct input. Meaning if i set
value 100 as the clockid and write a small application in userspace to set SO_TXTIME. The funny part is the 
clockid is successfully set even though there is no clock id 100 in kernel 

example :- 

[root@auto-lvarm-004 ~]# ./a.out -4 -S 192.168.1.1 -D 192.168.1.10 a,10

value from getsockopt is 100 <== Which means the setsockopt was successful with clockid 100 (which is junk)


I also agree that even without my patch, the code in fragmentation case was defaulting it to CLOCK_REALTIME 
if the mono_delivery_time bool was not set. (So we tried to keep the logic as close to the one which was available in upstream today)


I can propose 2 solutions to this 
1. Have stricter checks in setsockopt functions to set only REALTIME, MONO and TAI 
OR
2. Allow all clock id but only set tstamp_type for TAI, MONO and REALTIME to be forwarded to userspace(logic) 

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;
	case CLOCK_TAI:
		tstamp_type = SKB_CLOCK_TAI;
		break;
	default:
		WARN_ON_ONCE(1); <== remove this 
		kt = 0; <== remove this
	}

	skb_set_delivery_time(skb, kt, tstamp_type); <== pass kt as ease (as it is done previously too) and tstamp_type internally remains REAL
}

Let me know what you think. !

> 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.