Re: [PATCH bpf-next v2 1/2] net/smc: Introduce BPF injection capability for SMC

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

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@linux.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.


Hi Martin,

Thank you very much for your comments and I'm very sorry for my mistakes.

>
>>
>> Signed-off-by: D. Wythe <alibuda@linux.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?

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
>> +
>