netdev - Re: [PATCH net-next v3 10/14] net-timestamp: add basic support with tskey offset

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: <CAL+tcoD8jYsFPfAHAppbKSQu3eJS0V2V60xvijaUnN2rJSQiCg@mail.gmail.com>
Date: Fri, 1 Nov 2024 14:33:42 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, willemdebruijn.kernel@...il.com, 
	willemb@...gle.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, 
	eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev, 
	john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me, 
	haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org, ykolal@...com, 
	bpf@...r.kernel.org, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 10/14] net-timestamp: add basic support with
 tskey offset

On Fri, Nov 1, 2024 at 7:50 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 10/30/24 7:41 PM, Jason Xing wrote:
> >> bpf prog cannot directly access the skops->skb now. It is because the sockops
> >> prog sees the uapi "struct bpf_sock_ops" instead of "struct
> >> bpf_sock_ops(_kern)". The conversion is done in sock_ops_convert_ctx_access. It
> >> is an old way before BTF. I don't want to extend the uapi "struct bpf_sock_ops".
> >
> > Oh, so it seems we cannot use this way, right?
>
> No. don't extend the uapi "struct bpf_sock_ops". Use bpf_cast_to_kern_ctx() instead.

Got it!

>
> >
> > I also noticed a use case that allow users to get the information from one skb:
> > "int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb)" in
> > tools/testing/selftests/bpf/progs/netif_receive_skb.c
> > But it requires us to add the tracepoint in __skb_tstamp_tx() first.
> > Two months ago, I was planning to use a tracepoint for some people who
> > find it difficult to deploy bpf.
>
>
> It is a tracing prog instead of sockops prog. The verifier allows accessing
> different things based on the program type. This patch set is using the sockops
> bpf prog type which is not a tracing prog. Tracing can do a lot of read-only
> things but here we need write (e.g. bpf_setsockopt), so tracing prog is not
> suitable here.

Thanks for the explaination.

>
> >
> >>
> >> Instead, use bpf_cast_to_kern_ctx((struct bpf_sock_ops *)skops_ctx) to get a
> >> trusted "struct bpf_sock_ops(_kern) *skops" pointer. Then it can access the
> >> skops->skb.
> >
> > Let me spend some time on it. Thanks.
>
> Take a look at the bpf_cast_to_kern_ctx() examples in selftests/bpf. I think
> this can be directly used to get to (struct bpf_sock_ops_kern *)skops->skb. Ping
> back if your selftest bpf prog cannot load.

No problem :)

>
> >
> >> afaik, the tcb->seq should be available already during sendmsg. it
> >> should be able to get it from TCP_SKB_CB(skb)->seq with the bpf_core_cast. Take
> >> a look at the existing examples of bpf_core_cast.
> >>
> >> The same goes for the skb->data. It can use the bpf_dynptr_from_skb(). It is not
> >> available to skops program now but should be easy to expose.
>  > I wonder what the use of skb->data is here.
>
> You are right, not needed. I was thinking it may need to parse the tcp header
> from the skb at the rx timestamping. It is not needed. The tcp stack should have
> already parsed it and TCP_SKB_CB can be directly used as long as the sockops
> prog can get to the skops->skb.

Agreed.

>
> >>
> >> In the bpf prog, when the SCHED/SND/ACK timestamp comes back, it has to find the
> >> earlier sendmsg timestamp. One option is to store the earlier sendmsg timestamp
> >> at the bpf map key-ed by seqno or the shinfo's tskey. Storing in a bpf map
> >> key-ed by seqno/tskey is probably what the selftest should do. In the future, we
> >> can consider allowing the rbtree in the bpf sk local storage for searching
> >> seqno. There is shinfo's hwtstamp that can be used also if there is a need.
> >
> > Thanks for the information! Let me investigate how the bpf map works...
> >
> > I wonder that for the selftests could it be much simpler if we just
> > record each timestamp stored in three variables and calculate them at
> > last since we only send the small packet once instead of using bpf
> > map. I mean, bpf map is really good as far as I know, but I'm a bit
> > worried that implementing such a function could cause more extra work
> > (implementation and review).
>
> Don't worry on the review side. imo, a closer to the real world selftest prog is
> actually helping the review process. It needs to test the tskey anyway and it
> needs to store somewhere. bpf map is pretty simple to use. I don't think it will
> have much different in term of complexity also.

Got it, will do it soon :)

Thanks,
Jason