netdev - Re: [PATCH net-next v4] net: Re-use and set mono_delivery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ac03f67c-604a-4fb8-a0ca-c187e27855be@quicinc.com>
Date: Tue, 19 Mar 2024 23:22:41 -0700
From: "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
CC: Willem de Bruijn <willemdebruijn.kernel@...il.com>, <kernel@...cinc.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Andrew Halaney
	<ahalaney@...hat.com>,
        Martin KaFai Lau <martin.lau@...nel.org>, bpf
	<bpf@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Alexei
 Starovoitov" <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH net-next v4] net: Re-use and set mono_delivery_time bit
 for userspace tstamp packets



On 3/18/2024 12:02 PM, Abhishek Chauhan (ABC) wrote:
> 
> 
> On 3/14/2024 3:29 PM, Abhishek Chauhan (ABC) wrote:
>>
>>
>> On 3/14/2024 2:48 PM, Martin KaFai Lau wrote:
>>> On 3/14/24 1:53 PM, Abhishek Chauhan (ABC) wrote:
>>>>>> The bpf_convert_tstamp_{read,write} and the helper bpf_skb_set_tstamp need to be
>>>>>> changed to handle the new "user_delivery_time" bit anyway, e.g.
>>>>>> bpf_skb_set_tstamp(BPF_SKB_TSTAMP_DELIVERY_MONO) needs to clear the
>>>>>> "user_delivery_time" bit.
>>>>>>
>>>>>> I think the "struct inet_frag_queue" also needs a new "user_delivery_time"
>>>>>> field. "mono_delivery_time" is already in there.
>>>
>>> [ ... ]
>>>
> 
> Martin, Do we really need to add user_delivery_time as part of inet_frag_queue struct? I was wondering why is this required since we are using tstamp_type:2 to 
> distinguish between timestamp anyway .
> 
> Let me know what you think ? 
> 
>>> I would think the first step is to revert this patch. I don't think much of the current patch can be reused.
>>>
>>>> 1. I will raise one patch to introduce rename mono_delivery_time to
>>>> tstamp_type
>>>
>>> Right, I expect something like this:
>>>
>>> struct sk_buff {
>>>         /* ... */
>>> -            __u8                    mono_delivery_time:1;
>>> +        __u8            tstamp_type:1;
>>>         /* ... */
>>> };
>>>
>>
>> Okay ,This should be straight-forward. 
>>
>>>> 2. I will introduce setting of userspace timestamp type as the second bit
>>>> whem transmit_time is set.
>>>
>>> I expect the second patch should be introducing the enum first
>>>
>>> enum skb_tstamp_type {
>>>     SKB_TSTAMP_TYPE_RX_REAL = 0, /* A RX (receive) time in real */
>>>     SKB_TSTAMP_TYPE_TX_MONO = 1, /* A TX (delivery) time in mono */
>>> };
>>>
>>> and start doing "skb->tstamp_type = SKB_TSTAMP_TYPE_TX_MONO;" instead of
>>> "skb->tstamp_type = 1;"
>>>
>>> and the same for "skb->tstamp_type = SKB_TSTAMP_TYPE_RX_REAL;" instead of
>>> "skb->tstamp_type = 0;"
>>>
>>>
>>> This one I am not sure but probably need to change the skb_set_delivery_time() function signature also:
>>>
>>> static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
>>> -                                        bool mono)
>>> +                     enum skb_tstamp_type tstamp_type)
>>>
>> This should be straight-forward as well 
>>
>>> The third patch is to change tstamp_type from 1 bit to 2 bits and add SKB_TSTAMP_TYPE_TX_USER.
>>>
>>> struct sk_buff {
>>>         /* ... */
>>> -        __u8            tstamp_type:1;
>>> +        __u8            tstamp_type:2;
>>>         /* ... */
>>> };
>>>
>>> enum skb_tstamp_type {
>>>     SKB_TSTAMP_TYPE_RX_REAL = 0,    /* A RX (receive) time in real */
>>>     SKB_TSTAMP_TYPE_TX_MONO = 1,    /* A TX (delivery) time in mono */
>>> +    SKB_TSTAMP_TYPE_TX_USER = 2,    /* A TX (delivery) time and its clock
>>>                      * is in skb->sk->sk_clockid.
>>>                      */
>>>                
>>> };
>>>
>>> This will shift a bit out of the byte where tstamp_type lives. It should be the "inner_protocol_type" bit by my hand count. Please check if it is directly used in bpf instruction (filter.c). As far as I look, it is not, so should be fine. Some details about bpf instruction accessible skb bit field here: https://lore.kernel.org/all/20230321014115.997841-1-kuba@kernel.org/
>> This is where i would need thorough reviews from you and Willem as my area of expertise is limited to part of network stack and BPF is not one of them. 
>> But i have plan on this and i know how to do it. 
>>
>> Expect patches to be arriving to your inboxes next week, as we have a long weekend in Qualcomm 
>> Fingers crossed :) 
>>
>>>
>>>
>>>> 3. This will be a first step to make the design scalable.
>>>> 4. Tomorrow if we have more timestamp to support, upstream community has to do is
>>>> update the enum and increase the bitfield from 2=>3 and so on.
>>>>
>>>> I need help from Martin to test the patch which renames the mono_delivery_time
>>>> to tstamp_type (Which i feel should be straight forward as the value of the bit is 1)
>>>
>>> The bpf change is not a no-op rename of mono_delivery_time. It needs to take care of the new bit added to the tstamp_type. Please see the previous email (and I also left it in the beginning of this email).
>>>
>>> Thus, you need to compile the selftests/bpf/ and run it to verify the changes when handling the new bit. The Documentation/bpf/bpf_devel_QA.rst has the howto details. You probably only need the newer llvm (newer gcc should work also as bpf CI has been using it) and the newer pahole. I can definitely help if there is issue in running the test_progs in selftests/bpf or you have question on making the changes in filter.c. To run the test: "./test_progs -t tc_redirect/tc_redirect_dtime"
>>>

Martin,
I was able to compile test_progs and execute the above command mentioned by you . Does the output look okay for you ? 

[ 3076.040766] IPv6: ADDRCONF(NETDEV_CHANGE): veth_src_fwd: link becomes ready
[ 3076.040809] IPv6: ADDRCONF(NETDEV_CHANGE): veth_src: link becomes ready
[ 3076.072844] IPv6: ADDRCONF(NETDEV_CHANGE): veth_dst: link becomes ready
[ 3076.072880] IPv6: ADDRCONF(NETDEV_CHANGE): veth_dst_fwd: link becomes ready
#214/5   tc_redirect/tc_redirect_dtime:OK
#214     tc_redirect:OK
Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED