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

["D. Wythe" <alibuda@linux.alibaba.com >]

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.

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?

> >+static int smc_bpf_hs_ctrl_init(struct btf *btf) { return 0; }
> >+
> >+static int smc_bpf_hs_ctrl_reg(void *kdata, struct bpf_link *link)
> 
> More on attaching to netns. There is discussion on how to attach a
> bpf_struct_ops to a particular cgroup in a link. I think the link
> should be able to attach a bpf_struct_ops to a particular netns
> also.
> 
> I would suggest to reject link now. Later, link support can be added
> to attach to a particular netns. This will be the last non-link-only
> bpf_struct_ops addition, considering the blast radius is limited on
> smc_hs_ctrl and the smc effort was started a while ago. I could have
> missed things here. Other experts could chime in.
> 
> 	if (link)
> 		return -EOPNOTSUPP;

Got it. This approach looks good to me. I’ll send out the next version
with this change.