Re: [RFC PATCH bpf-next v1 3/3] net: Add additional bit to support userspace timestamp type

[Willem de Bruijn]

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@xxxxxxxxx/
> >> Signed-off-by: Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx>

> > 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@xxxxxxxxxx/
> 
> 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.