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: <20251106023302.GA44223@j66a10360.sqa.eu95>
Date: Thu, 6 Nov 2025 10:33:02 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com    >
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: "D. Wythe" <alibuda@...ux.alibaba.com>, 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 Wed, Nov 05, 2025 at 02:58:48PM -0800, Martin KaFai Lau wrote:
> 
> 
> 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?

My initial concern was that certain ctrl->func callbacks might
eventually need to operate on an smc_sock rather than a tcp_sock.
Crucially, we cannot always derive the owning smc_sock from a given
tcp_sock (at least not with the current design). Therefore, a macro
unconditionally passing tp (a tcp_sock pointer) would be unable to
handle future scenarios requiring operation on an smc_sock. This could
create an awkward situation with an unconditional tp argument.

However, considering the current situation, I believe the proposed
approach is workable. And for future cases where smc_sock-specific
callbacks become necessary, we can certainly introduce a new, dedicated
macro at that point to address it. Therefore, I'm happy to proceed with
your suggested change.

> >
> >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;                                                  \
>  })
> 
>
Take it. I’ll send the updated version with this change.