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

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]

Date: Wed, 10 Apr 2024 18:58:30 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.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>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 bpf <bpf@...r.kernel.org>
Cc: kernel@...cinc.com
Subject: Re: [RFC PATCH bpf-next v1 3/3] net: Add additional bit to support
 userspace timestamp type

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@...cinc.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.