Skip to content

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: net/smc: Introduce smc_hs_ctrl
version: 3
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006989

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 4ef77dd
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006989
version: 3

@kernel-patches-review-bot
Copy link

AI review job summary: https://github.com/kernel-patches/bpf/actions/runs/18088199303

Inline review:

commit bfb0eca9a22887d8d0d1e29dbd4dc2fa3637fb32
Author: D. Wythe <[email protected]>

net/smc: bpf: Introduce generic hook for handshake flow

This patch introduces BPF struct_ops hooks to allow eBPF programs to
control SMC protocol usage during TCP handshake. Users can write custom
implementations to decide whether to use SMC based on runtime context
like IP addresses and ports.

> diff --git a/include/net/netns/smc.h b/include/net/netns/smc.h
> index fc752a50f..d66bfd392 100644
> --- a/include/net/netns/smc.h
> +++ b/include/net/netns/smc.h
> @@ -17,6 +17,9 @@ struct netns_smc {
>  #ifdef CONFIG_SYSCTL
>  	struct ctl_table_header		*smc_hdr;
>  #endif
> +#if IS_ENABLED(CONFIG_SMC_HS_CTRL_BPF)
> +	struct smc_hs_ctrl __rcu	*hs_ctrl;
> +#endif /* CONFIG_SMC_HS_CTRL_BPF */
>  	unsigned int			sysctl_autocorking_size;
>  	unsigned int			sysctl_smcr_buf_type;
>  	int				sysctl_smcr_testlink_time;

[ ... ]

> diff --git a/net/smc/smc_sysctl.c b/net/smc/smc_sysctl.c
> index 2fab6456f..918516734 100644
> --- a/net/smc/smc_sysctl.c
> +++ b/net/smc/smc_sysctl.c
> @@ -12,12 +12,14 @@
>
>  #include <linux/init.h>
>  #include <linux/sysctl.h>
> +#include <linux/bpf.h>
>  #include <net/net_namespace.h>
>
>  #include "smc.h"
>  #include "smc_core.h"
>  #include "smc_llc.h"
>  #include "smc_sysctl.h"
> +#include "smc_hs_bpf.h"
>
>  static int min_sndbuf = SMC_BUF_MIN_SIZE;
>  static int min_rcvbuf = SMC_BUF_MIN_SIZE;

[ ... ]

> @@ -109,6 +183,16 @@ int __net_init smc_sysctl_net_init(struct net *net)
>  	table = smc_table;
>  	if (!net_eq(net, &init_net)) {
>  		int i;
> +#if IS_ENABLED(CONFIG_SMC_HS_CTRL_BPF)
> +		struct smc_hs_ctrl *ctrl;
> +
> +		rcu_read_lock();
> +		ctrl = rcu_dereference(init_net.smc.hs_ctrl);
> +		if (ctrl && ctrl->flags & SMC_HS_CTRL_FLAG_INHERITABLE &&
> +		    bpf_try_module_get(ctrl, ctrl->owner))
> +			rcu_assign_pointer(net->smc.hs_ctrl, ctrl);
> +		rcu_read_unlock();
> +#endif /* CONFIG_SMC_HS_CTRL_BPF */

Can this leak the module reference if the function fails later? When
bpf_try_module_get() succeeds here and assigns ctrl to net->smc.hs_ctrl,
but then kmemdup() fails below, the code jumps to err_alloc which calls
smc_net_replace_smc_hs_ctrl(net, NULL). However, at that point
net->smc.hs_ctrl already holds the reference we just acquired.

>
>  		table = kmemdup(table, sizeof(smc_table), GFP_KERNEL);
>  		if (!table)
> @@ -139,6 +223,9 @@ int __net_init smc_sysctl_net_init(struct net *net)
>  	if (!net_eq(net, &init_net))
>  		kfree(table);
>  err_alloc:
> +#if IS_ENABLED(CONFIG_SMC_HS_CTRL_BPF)
> +	smc_net_replace_smc_hs_ctrl(net, NULL);
> +#endif /* CONFIG_SMC_HS_CTRL_BPF */
>  	return -ENOMEM;
>  }
>

The smc_net_replace_smc_hs_ctrl() function will set net->smc.hs_ctrl
to NULL, but the module reference acquired earlier won't be released
since the xchg() at smc_sysctl.c:58 will return NULL (the ctrl was
assigned but the xchg expects the RCU_INITIALIZER wrapper).

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 4ef77dd
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006989
version: 3

Exports three necessary symbols for implementing struct_ops with
tristate subsystem.

To hold or release refcnt of struct_ops refcnt by inline funcs
bpf_try_module_get and bpf_module_put which use bpf_struct_ops_get(put)
conditionally.

And to copy obj name from one to the other with effective checks by
bpf_obj_name_cpy.

Signed-off-by: D. Wythe <[email protected]>
The introduction of IPPROTO_SMC enables eBPF programs to determine
whether to use SMC based on the context of socket creation, such as
network namespaces, PID and comm name, etc.

As a subsequent enhancement, to introduce a new generic hook that
allows decisions on whether to use SMC or not at runtime, including
but not limited to local/remote IP address or ports.

User can write their own implememtion via bpf_struct_ops now to choose
whether to use SMC or not before TCP 3rd handshake to be comleted.

Signed-off-by: D. Wythe <[email protected]>
Reviewed-by: Dust Li <[email protected]>
This tests introduces a tiny smc_hs_ctrl for filtering SMC connections
based on IP pairs, and also adds a realistic topology model to verify it.

Also, we can only use SMC loopback under CI test, so an additional
configuration needs to be enabled.

Follow the steps below to run this test.

make -C tools/testing/selftests/bpf
cd tools/testing/selftests/bpf
sudo ./test_progs -t smc

Results shows:
Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: D. Wythe <[email protected]>
Tested-by: Saket Kumar Bhaskar <[email protected]>
Reviewed-by: Zhu Yanjun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant