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: <8d245f5a-0c75-4634-9513-3d420eb2c88f@linux.dev>
Date: Thu, 14 Mar 2024 14:48:54 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
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/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.

[ ... ]

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;
		/* ... */
};

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

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/

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