netdev - Re: [PATCH bpf-next v9 02/12] bpf: prepare for timestamping callbacks use

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: <fb1111fc-6a4e-4388-860c-0077910e814f@linux.dev>
Date: Mon, 10 Feb 2025 17:31:50 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Jason Xing <kerneljasonxing@...il.com>
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, horms@...nel.org, bpf@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v9 02/12] bpf: prepare for timestamping callbacks
 use

On 2/8/25 2:32 AM, Jason Xing wrote:
> Later, four callback points to report information to user space
> based on this patch will be introduced.
> 
> As to skb initialization here, users can follow these three steps
> as below to fetch the shared info from the exported skb in the bpf
> prog:
> 1. skops_kern = bpf_cast_to_kern_ctx(skops);
> 2. skb = skops_kern->skb;
> 3. shinfo = bpf_core_cast(skb->head + skb->end, struct skb_shared_info);
> 
> More details can be seen in the last selftest patch of the series.

This BPF program example is not useful in this commit message. It is not how 
this change will be used in the kernel. People will naturally be required to 
look at the selftest to see how the bpf prog can get to the skb and tskey, etc.

The commit message should focus on explaining "what" has changed and "why" it is 
necessary. The "why" part ("four callback points to report...") is mostly 
present but could be clearer.

Subject: bpf: Prepare the sock_ops ctx and call bpf prog for TX timestamping

(What)
This patch introduces a new bpf_skops_tx_timestamping() function that prepares 
the "struct bpf_sock_ops" ctx and then executes the sockops BPF program.

(Why)
The subsequent patch will utilize bpf_skops_tx_timestamping() at the existing TX 
timestamping kernel callbacks (__sk_tstamp_tx specifically) to call the sockops 
BPF program.


> 
> Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> ---
>   include/net/sock.h |  7 +++++++
>   net/core/sock.c    | 15 +++++++++++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7916982343c6..6f4d54faba92 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2923,6 +2923,13 @@ int sock_set_timestamping(struct sock *sk, int optname,
>   			  struct so_timestamping timestamping);
>   
>   void sock_enable_timestamps(struct sock *sk);
> +#if defined(CONFIG_CGROUP_BPF)
> +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op);
> +#else
> +static inline void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> +{
> +}
> +#endif
>   void sock_no_linger(struct sock *sk);
>   void sock_set_keepalive(struct sock *sk);
>   void sock_set_priority(struct sock *sk, u32 priority);
> diff --git a/net/core/sock.c b/net/core/sock.c
> index eae2ae70a2e0..41db6407e360 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -948,6 +948,21 @@ int sock_set_timestamping(struct sock *sk, int optname,
>   	return 0;
>   }
>   
> +#if defined(CONFIG_CGROUP_BPF)
> +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> +{
> +	struct bpf_sock_ops_kern sock_ops;
> +
> +	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
> +	sock_ops.op = op;
> +	sock_ops.is_fullsock = 1;
> +	sock_ops.sk = sk;
> +	bpf_skops_init_skb(&sock_ops, skb, 0);
> +	/* Timestamping bpf extension supports only TCP and UDP full socket */

nit: After our earlier discussions, it's clear that the above is_fullsock = 1; 
is always true for all sk here. This comment has become redundant. Let's remove it.

> +	__cgroup_bpf_run_filter_sock_ops(sk, &sock_ops, CGROUP_SOCK_OPS);
> +}
> +#endif
> +
>   void sock_set_keepalive(struct sock *sk)
>   {
>   	lock_sock(sk);