| 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
| ||
|
Message-ID: <76e226e6-f3bf-f740-c86c-6ee214aff07d@linux.dev>
Date: Wed, 22 Feb 2023 13:40:43 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: "D. Wythe" <alibuda@...ux.alibaba.com>
Cc: kuba@...nel.org, davem@...emloft.net, netdev@...r.kernel.org,
linux-s390@...r.kernel.org, linux-rdma@...r.kernel.org,
bpf@...r.kernel.org, kgraul@...ux.ibm.com, wenjia@...ux.ibm.com,
jaka@...ux.ibm.com, ast@...nel.org, daniel@...earbox.net,
andrii@...nel.org
Subject: Re: [PATCH bpf-next v2 1/2] net/smc: Introduce BPF injection
capability for SMC
On 2/21/23 4:18 AM, D. Wythe wrote:
> From: "D. Wythe" <alibuda@...ux.alibaba.com>
>
> This PATCH attempts to introduce BPF injection capability for SMC.
> As we all know that the SMC protocol is not suitable for all scenarios,
> especially for short-lived. However, for most applications, they cannot
> guarantee that there are no such scenarios at all. Therefore, apps
> may need some specific strategies to decide shall we need to use SMC
> or not, for example, apps can limit the scope of the SMC to a specific
> IP address or port.
>
> Based on the consideration of transparent replacement, we hope that apps
> can remain transparent even if they need to formulate some specific
> strategies for SMC using. That is, do not need to recompile their code.
>
> On the other hand, we need to ensure the scalability of strategies
> implementation. Although it is simple to use socket options or sysctl,
> it will bring more complexity to subsequent expansion.
>
> Fortunately, BPF can solve these concerns very well, users can write
> thire own strategies in eBPF to choose whether to use SMC or not.
> And it's quite easy for them to modify their strategies in the future.
>
> This PATCH implement injection capability for SMC via struct_ops.
> In that way, we can add new injection scenarios in the future.
I have never used smc. I can only comment at its high level usage and details on
the bpf side.
>
> Signed-off-by: D. Wythe <alibuda@...ux.alibaba.com>
> ---
> include/linux/btf_ids.h | 15 +++
> include/net/smc.h | 254 ++++++++++++++++++++++++++++++++++++++
> kernel/bpf/bpf_struct_ops_types.h | 4 +
> net/Makefile | 5 +
> net/smc/af_smc.c | 10 +-
> net/smc/bpf_smc_struct_ops.c | 146 ++++++++++++++++++++++
> net/smc/smc.h | 220 ---------------------------------
> 7 files changed, 433 insertions(+), 221 deletions(-)
> create mode 100644 net/smc/bpf_smc_struct_ops.c
>
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index 3a4f7cd..25eab1e 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -264,6 +264,21 @@ enum {
> MAX_BTF_TRACING_TYPE,
> };
>
> +#if IS_ENABLED(CONFIG_SMC)
> +#define BTF_SMC_TYPE_xxx \
> + BTF_SMC_TYPE(BTF_SMC_TYPE_SOCK, smc_sock) \
> + BTF_SMC_TYPE(BTF_SMC_TYPE_CONNECTION, smc_connection) \
> + BTF_SMC_TYPE(BTF_SMC_TYPE_HOST_CURSOR, smc_host_cursor)
> +
> +enum {
> +#define BTF_SMC_TYPE(name, type) name,
> +BTF_SMC_TYPE_xxx
> +#undef BTF_SMC_TYPE
> +MAX_BTF_SMC_TYPE,
> +};
> +extern u32 btf_smc_ids[];
Do all these need to be in btf_ids.h?
> +#endif
> +
> extern u32 btf_tracing_ids[];
> extern u32 bpf_cgroup_btf_id[];
> extern u32 bpf_local_storage_map_btf_id[];
> diff --git a/include/net/smc.h b/include/net/smc.h
> index 597cb93..912c269 100644
> --- a/include/net/smc.h
> +++ b/include/net/smc.h
It is not obvious to me why the header moving is needed (from net/smc/smc.h to
include/net/smc.h ?). This can use some comment in the commit message and please
break it out to another patch.
[ ... ]
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -52,6 +52,11 @@ obj-$(CONFIG_TIPC) += tipc/
> obj-$(CONFIG_NETLABEL) += netlabel/
> obj-$(CONFIG_IUCV) += iucv/
> obj-$(CONFIG_SMC) += smc/
> +ifneq ($(CONFIG_SMC),)
> +ifeq ($(CONFIG_BPF_SYSCALL),y)
> +obj-y += smc/bpf_smc_struct_ops.o
This will ensure bpf_smc_struct_ops.c compiled as builtin even when smc is
compiled as module?
> diff --git a/net/smc/bpf_smc_struct_ops.c b/net/smc/bpf_smc_struct_ops.c
> new file mode 100644
> index 0000000..a5989b6
> --- /dev/null
> +++ b/net/smc/bpf_smc_struct_ops.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kernel.h>
> +#include <linux/bpf_verifier.h>
> +#include <linux/btf_ids.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> +#include <net/sock.h>
> +#include <net/smc.h>
> +
> +extern struct bpf_struct_ops smc_sock_negotiator_ops;
> +
> +DEFINE_RWLOCK(smc_sock_negotiator_ops_rwlock);
> +struct smc_sock_negotiator_ops *negotiator;
Is it sure one global negotiator (policy) will work for all smc_sock? or each sk
should have its own negotiator and the negotiator is selected by setsockopt.
> +
> +/* convert sk to smc_sock */
> +static inline struct smc_sock *smc_sk(const struct sock *sk)
> +{
> + return (struct smc_sock *)sk;
> +}
> +
> +/* register ops */
> +static inline void smc_reg_passive_sk_ops(struct smc_sock_negotiator_ops *ops)
> +{
> + write_lock_bh(&smc_sock_negotiator_ops_rwlock);
> + negotiator = ops;
What happens to the existing negotiator?
> + write_unlock_bh(&smc_sock_negotiator_ops_rwlock);
> +}
> +
> +/* unregister ops */
> +static inline void smc_unreg_passive_sk_ops(struct smc_sock_negotiator_ops *ops)
> +{
> + write_lock_bh(&smc_sock_negotiator_ops_rwlock);
> + if (negotiator == ops)
> + negotiator = NULL;
> + write_unlock_bh(&smc_sock_negotiator_ops_rwlock);
> +}
> +
> +int smc_sock_should_select_smc(const struct smc_sock *smc)
> +{
> + int ret = SK_PASS;
> +
> + read_lock_bh(&smc_sock_negotiator_ops_rwlock);
> + if (negotiator && negotiator->negotiate)
> + ret = negotiator->negotiate((struct smc_sock *)smc);
> + read_unlock_bh(&smc_sock_negotiator_ops_rwlock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(smc_sock_should_select_smc);
> +
> +void smc_sock_perform_collecting_info(const struct smc_sock *smc, int timing)
> +{
> + read_lock_bh(&smc_sock_negotiator_ops_rwlock);
> + if (negotiator && negotiator->collect_info)
> + negotiator->collect_info((struct smc_sock *)smc, timing);
> + read_unlock_bh(&smc_sock_negotiator_ops_rwlock);
> +}
> +EXPORT_SYMBOL_GPL(smc_sock_perform_collecting_info);
> +
> +/* define global smc ID for smc_struct_ops */
> +BTF_ID_LIST_GLOBAL(btf_smc_ids, MAX_BTF_SMC_TYPE)
How is btf_smc_ids used?
> +#define BTF_SMC_TYPE(name, type) BTF_ID(struct, type)
> +BTF_SMC_TYPE_xxx
> +#undef BTF_SMC_TYPE
> +
Powered by blists - more mailing lists