On 2/23/23 5:40 AM, Martin KaFai Lau wrote:
On 2/21/23 4:18 AM, D. Wythe wrote:
From: "D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>
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.
Hi Martin,
Thank you very much for your comments and I'm very sorry for my mistakes.
Signed-off-by: D. Wythe <alibuda@xxxxxxxxxxxxxxxxx>
---
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?
My original intention is to do some security checks via btf_smc_ids,
but since it is not implemented at present, so it is not necessary here.
+#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.
Got it, , I have finished the splitting.
[ ... ]
--- 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?
Yes, smc allow compiled as module.
We are also struggling here. If you have a better way, please let me
know. 😁
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.
This is really a good question, we can really consider adding an
independent negotiator for each sock.
But just like the TCP congestion control , the global negotiator can be
used for sock without
special requirements.
+
+/* 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?
What if we return a failure when the negotiator already exists ?
+ 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?
Yes, it is useless here for the time being. I will remove them in the
new version.
+#define BTF_SMC_TYPE(name, type) BTF_ID(struct, type)
+BTF_SMC_TYPE_xxx
+#undef BTF_SMC_TYPE
+
