netdev - Re: [PATCH bpf-next v4 2/3] net/smc: bpf: Introduce generic hook for handshake flow

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: <dfed97fb-4e0c-416e-b5d8-8de7b3edce69@linux.dev>
Date: Wed, 5 Nov 2025 14:58:48 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: "D. Wythe" <alibuda@...ux.alibaba.com>
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
 pabeni@...hat.com, song@...nel.org, sdf@...gle.com, haoluo@...gle.com,
 yhs@...com, edumazet@...gle.com, john.fastabend@...il.com,
 kpsingh@...nel.org, jolsa@...nel.org, mjambigi@...ux.ibm.com,
 wenjia@...ux.ibm.com, wintera@...ux.ibm.com, dust.li@...ux.alibaba.com,
 tonylu@...ux.alibaba.com, guwen@...ux.alibaba.com, bpf@...r.kernel.org,
 davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
 sidraya@...ux.ibm.com, jaka@...ux.ibm.com
Subject: Re: [PATCH bpf-next v4 2/3] net/smc: bpf: Introduce generic hook for
 handshake flow

On 11/4/25 11:01 PM, D. Wythe wrote:
> On Tue, Nov 04, 2025 at 04:03:46PM -0800, Martin KaFai Lau wrote:
>>
>>
>> On 11/2/25 11:31 PM, D. Wythe wrote:
>>> +#if IS_ENABLED(CONFIG_SMC_HS_CTRL_BPF)
>>> +#define smc_call_hsbpf(init_val, sk, func, ...) ({		\
>>> +	typeof(init_val) __ret = (init_val);			\
>>> +	struct smc_hs_ctrl *ctrl;				\
>>> +	rcu_read_lock();					\
>>> +	ctrl = rcu_dereference(sock_net(sk)->smc.hs_ctrl);	\
>>
>> The smc_hs_ctrl (and its ops) is called from the netns, so the
>> bpf_struct_ops is attached to a netns. Attaching bpf_struct_ops to a
>> netns has not been done before. More on this later.
>>
>>> +	if (ctrl && ctrl->func)					\
>>> +		__ret = ctrl->func(__VA_ARGS__);		\
>>> +
>>> +	if (static_branch_unlikely(&tcp_have_smc) && tp->syn_smc) {
>>> +		tp->syn_smc = !!smc_call_hsbpf(1, sk, syn_option, tp);
>>
>> ... so just pass tp instead of passing both sk and tp?
>>
>> [ ... ]
>>
> 
> You're right, it is a bit redundant. However, if we merge the parameters,
> every user of this macro will be forced to pass tp. In fact, we’re
> already considering adding some callback functions that don’t take tp as
> a parameter.

If the struct_ops callback does not take tp, then don't pass it to the
callback. I have a hard time to imagine why the bpf prog will not be
interested in the tp/sk pointer though.

or you meant the caller does not have tp? and where is the future caller?
> 
> I’ve been considering this: since smc_hs_ctrl is called from the netns,
> maybe we should replace the sk parameter with netns directly. After all,
> the only reason we pass sk here is to extract sock_net(sk). Doing so
> would remove the redundancy and also keep the interface more flexible
> for future extensions. What do you think?

The net can be obtained from the tp also.

Like in this patch, all the caller needs to type
"const struct sock *sk = &tp->inet_conn.icsk_inet.sk;". I can imagine all
the callers will have to type "sock_net((struct sock *)tp)" if passing net.
Why not just do that in the smc_hs_ctrl instead of asking all the callers
to type that?

I meant something like this (untested):

-#define smc_call_hsbpf(init_val, sk, func, ...) ({             \
+#define smc_call_hsbpf(init_val, func, tp, ...) ({             \
	typeof(init_val) __ret = (init_val);                    \
	struct smc_hs_ctrl *ctrl;                               \
	rcu_read_lock();                                        \
-	ctrl = rcu_dereference(sock_net(sk)->smc.hs_ctrl);      \
+	ctrl = rcu_dereference(sock_net((struct sock *)(tp))->smc.hs_ctrl);     \
	if (ctrl && ctrl->func)                                 \
-		__ret = ctrl->func(__VA_ARGS__);                \
+		__ret = ctrl->func(tp, ##__VA_ARGS__);          \
	rcu_read_unlock();                                      \
	__ret;                                                  \
  })