Received-SPF: pass (google.com: domain of linux-kernel+bounces-139511-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 10 Apr 2024 18:58:30 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.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>, 
 Martin KaFai Lau <martin.lau@linux.dev>, 
 Daniel Borkmann <daniel@iogearbox.net>, 
 bpf <bpf@vger.kernel.org>
Cc: kernel@quicinc.com
Message-ID: <66171996a10d2_2d123b29418@willemb.c.googlers.com.notmuch>
In-Reply-To: <f28de1e7-4a9b-4a97-b4f9-723425725b58@quicinc.com>
References: <20240409210547.3815806-1-quic_abchauha@quicinc.com>
 <20240409210547.3815806-4-quic_abchauha@quicinc.com>
 <6616b3587520_2a98a5294db@willemb.c.googlers.com.notmuch>
 <f28de1e7-4a9b-4a97-b4f9-723425725b58@quicinc.com>
Subject: Re: [RFC PATCH bpf-next v1 3/3] net: Add additional bit to support
 userspace timestamp type
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

Abhishek Chauhan (ABC) wrote:
> 
> 
> On 4/10/2024 8:42 AM, Willem de Bruijn wrote:
> > Abhishek Chauhan wrote:
> >> tstamp_type can be real, mono or userspace timestamp.
> >>
> >> This commit adds userspace timestamp and sets it if there is
> >> valid transmit_time available in socket coming from userspace.
> >>
> >> To make the design scalable for future needs this commit bring in
> >> the change to extend the tstamp_type:1 to tstamp_type:2 to support
> >> userspace timestamp.
> >>
> >> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/
> >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>

> > With pahole, does this have an effect on sk_buff layout?
> > 
> I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
> changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC. 
> That being said i am actually trying to understand/learn BPF instructions to know things better. 
> I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK
> 
> 
> #ifdef __BIG_ENDIAN_BITFIELD
> #define SKB_MONO_DELIVERY_TIME_MASK	(1 << 7) //Suspecting changes here too
> #define TC_AT_INGRESS_MASK		(1 << 6) // and here 
> #else
> #define SKB_MONO_DELIVERY_TIME_MASK	(1 << 0)
> #define TC_AT_INGRESS_MASK		(1 << 1) (this might have to change to 1<<2 )
> #endif
> #define SKB_BF_MONO_TC_OFFSET		offsetof(struct sk_buff, __mono_tc_offset)
> 
> Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c 
> I am trying to figure out what this part of the code is doing.
> https://lore.kernel.org/all/20230321014115.997841-4-kuba@kernel.org/
> 
> Please correct me if i am wrong here.

I broadly agree. We should convert all references to
SKB_MONO_DELIVERY_TIME_MASK to an skb_tstamp_type equivalent.

> 
> >> @@ -4274,7 +4280,16 @@ static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt,
> >>  					enum skb_tstamp_type tstamp_type)
> >>  {
> >>  	skb->tstamp = kt;
> >> -	skb->tstamp_type = kt && tstamp_type;
> >> +
> >> +	if (skb->tstamp_type)
> >> +		return;
> >> +
> > 
> I can put a warn on here incase if both MONO and TAI are set. 
> OR 
> Rather make it simple as you have mentioned below.

When might skb->tstamp_type already be non-zero when
skb_set_deliver_type is called?

In most cases, this is called for a fresh skb.

In br_ip6_fragment, it is called with a previous value. But this is
the value of skb->tstamp_type. It just clears it if kt is 0.

If skb->tstamp_type != tstamp_type is not a condition that can be
forced by an unprivileged user, then we can warn.