Re: [PATCH net-next v2] net: Modify mono_delivery_time with clockid_delivery_time

[Abhishek Chauhan (ABC)]

On 2/29/2024 6:44 AM, Willem de Bruijn wrote:
> Martin KaFai Lau wrote:
>> On 2/28/24 7:53 AM, Willem de Bruijn wrote:
>>> Sidenote: with sk_clockid, FQ could detect when skb->tstamp is not
>>> set in monotonic (i.e., set by SO_TXTIME) and drop the packet or
>>> ignore the embedded timestamp, warn, etc.
>>
>> Thanks for cc-ing me. Sorry for the late reply. I just catch up to this thread 
>> and the v1.
>>
>> I think it is needed to detect if skb->tstamp is monotonic or not in fq. The 
>> container (with the veth setup) may use sch_etf while the host usually uses fq 
>> at the physical NIC and expects monotonic skb->tstamp.
>>
>> During forward (e.g. by bpf_redirect / ip[6]_forward from a veth to a physical 
>> NIC), skb_clear_tstamp() only forwards the monotonic skb->tstamp now. While 
>> sch_etf does check sk_clockid first before using skb->tstamp, fq does not check 
>> that now.
>> or fq_packet_beyond_horizon() is enough to catch this clock discrepancy?
> 
> Before your patch, I believe FQ had no such guard rails. An skb with
> any clockid from SO_TXTIME can arrive at FQ.
> 
> With the new clockid field, we could add guard rails in fq_enqueue.
> If the bit is set, look up sk_clockid.
> 

I agree with Willem here. Just how Sch_etf is doing in the validate_input_params 
function , it checks for qopt->clockid != CLOCK_TAI  and also in validate packet
its checking if the q->clockid and the skb->clockid are the same. Only then it 
proceeds to XMIT the packet. 
Such Guards can be added in FQ as well. 

I believe this is some optimization which can be done in future which is not in 
scope of this current problem (correct me if i am wrong). 

The main issue with bridge forwarding path which anyone using latest kernel would 
face is all ETF packets will be dropped since only monotonic tstamps are forwarded
and ETF uses TAI clocks . Hence my v3 patch would be helpful to solve the problem. 
Later we can take up the optimization part to add Guard checks in the FQ as well as
a seperate patch. 

> 
>