| 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: <9e7ec07f-bc03-4e62-a0f6-28f668a1ec42@gmail.com>
Date: Fri, 20 Oct 2023 08:12:23 -0700
From: Kui-Feng Lee <sinquersw@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>, thinker.li@...il.com
Cc: kuifeng@...a.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
ast@...nel.org, song@...nel.org, kernel-team@...a.com, andrii@...nel.org,
drosen@...gle.com
Subject: Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
On 10/18/23 18:49, Martin KaFai Lau wrote:
> On 10/17/23 9:23 AM, thinker.li@...il.com wrote:
>> From: Kui-Feng Lee <thinker.li@...il.com>
>>
>> Replace the static list of struct_ops types with pre-btf
>> struct_ops_tab to
>> enable dynamic registration.
>>
>> Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function
>> instead of being listed in bpf_struct_ops_types.h.
>>
>> Cc: netdev@...r.kernel.org
>> Signed-off-by: Kui-Feng Lee <thinker.li@...il.com>
>> ---
>> include/linux/bpf.h | 2 +
>> include/linux/btf.h | 29 +++++++
>> kernel/bpf/bpf_struct_ops.c | 124 +++++++++++++++---------------
>> kernel/bpf/bpf_struct_ops_types.h | 12 ---
>> kernel/bpf/btf.c | 2 +-
>> net/bpf/bpf_dummy_struct_ops.c | 14 +++-
>> net/ipv4/bpf_tcp_ca.c | 16 +++-
>> 7 files changed, 119 insertions(+), 80 deletions(-)
>> delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 1e1647c8b0ce..b0f33147aa93 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct
>> bpf_prog *prog)
>> return prog->aux->func_idx != 0;
>> }
>> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
>> +
>> #endif /* _LINUX_BPF_H */
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index aa2ba77648be..fdc83aa10462 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -12,6 +12,8 @@
>> #include <uapi/linux/bpf.h>
>> #define BTF_TYPE_EMIT(type) ((void)(type *)0)
>> +#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0); \
>> + ((void)(struct bpf_struct_ops_##type *)0); }
>> #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>> /* These need to be macros, as the expressions are used in assembler
>> input */
>> @@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
>> bool btf_is_kernel(const struct btf *btf);
>> bool btf_is_module(const struct btf *btf);
>> struct module *btf_try_get_module(const struct btf *btf);
>> +struct btf *btf_get_module_btf(const struct module *module);
>> u32 btf_nr_types(const struct btf *btf);
>> bool btf_member_is_reg_int(const struct btf *btf, const struct
>> btf_type *s,
>> const struct btf_member *m,
>> @@ -577,4 +580,30 @@ int btf_add_struct_ops(struct bpf_struct_ops
>> *st_ops);
>> const struct bpf_struct_ops **
>> btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
>> +enum bpf_struct_ops_state {
>> + BPF_STRUCT_OPS_STATE_INIT,
>> + BPF_STRUCT_OPS_STATE_INUSE,
>> + BPF_STRUCT_OPS_STATE_TOBEFREE,
>> + BPF_STRUCT_OPS_STATE_READY,
>> +};
>> +
>> +struct bpf_struct_ops_common_value {
>> + refcount_t refcnt;
>> + enum bpf_struct_ops_state state;
>> +};
>> +#define BPF_STRUCT_OPS_COMMON_VALUE struct
>> bpf_struct_ops_common_value common
>
> Since there is 'struct bpf_struct_ops_common_value' now, the
> BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets
> remove it.
Agree
>
>> +
>> +/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
>> + * the map's value exposed to the userspace and its btf-type-id is
>> + * stored at the map->btf_vmlinux_value_type_id.
>> + *
>> + */
>> +#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name) \
>> +extern struct bpf_struct_ops bpf_##_name; \
>> + \
>> +struct bpf_struct_ops_##_name { \
>> + BPF_STRUCT_OPS_COMMON_VALUE; \
>> + struct _name data ____cacheline_aligned_in_smp; \
>> +}
>
> I think the bpp_struct_ops_* should not be in btf.h. Probably move them
> to bpf.h instead. or there is some other considerations I am missing?
Yes, I think bpf.h is the right place.
>
>> +
>> #endif
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 60445ff32275..175068b083cb 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -13,19 +13,6 @@
>> #include <linux/btf_ids.h>
>> #include <linux/rcupdate_wait.h>
>> -enum bpf_struct_ops_state {
>> - BPF_STRUCT_OPS_STATE_INIT,
>> - BPF_STRUCT_OPS_STATE_INUSE,
>> - BPF_STRUCT_OPS_STATE_TOBEFREE,
>> - BPF_STRUCT_OPS_STATE_READY,
>> -};
>> -
>> -struct bpf_struct_ops_common_value {
>> - refcount_t refcnt;
>> - enum bpf_struct_ops_state state;
>> -};
>> -#define BPF_STRUCT_OPS_COMMON_VALUE struct
>> bpf_struct_ops_common_value common
>> -
>> struct bpf_struct_ops_value {
>> BPF_STRUCT_OPS_COMMON_VALUE;
>> char data[] ____cacheline_aligned_in_smp;
>> @@ -72,35 +59,6 @@ static DEFINE_MUTEX(update_mutex);
>> #define VALUE_PREFIX "bpf_struct_ops_"
>> #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
>> -/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
>> - * the map's value exposed to the userspace and its btf-type-id is
>> - * stored at the map->btf_vmlinux_value_type_id.
>> - *
>> - */
>> -#define BPF_STRUCT_OPS_TYPE(_name) \
>> -extern struct bpf_struct_ops bpf_##_name; \
>> - \
>> -struct bpf_struct_ops_##_name { \
>> - BPF_STRUCT_OPS_COMMON_VALUE; \
>> - struct _name data ____cacheline_aligned_in_smp; \
>> -};
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> -
>> -enum {
>> -#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> - __NR_BPF_STRUCT_OPS_TYPE,
>> -};
>> -
>> -static struct bpf_struct_ops * const bpf_struct_ops[] = {
>> -#define BPF_STRUCT_OPS_TYPE(_name) \
>> - [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> -};
>> -
>> const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
>> };
>> @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct
>> bpf_struct_ops *st_ops,
>> }
>> +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
>> + struct btf *btf)
>
> Please combine this function into register_bpf_struct_ops(). They are
> both very short.
>
Got it!
>> +{
>> + struct bpf_verifier_log *log;
>> + int err;
>> +
>> + if (st_ops == NULL)
>> + return -EINVAL;
>> +
>> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
>> + if (!log) {
>> + err = -ENOMEM;
>> + goto errout;
>> + }
>> +
>> + log->level = BPF_LOG_KERNEL;
>> +
>> + bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
>> +
>> + err = btf_add_struct_ops(st_ops);
>> +
>> +errout:
>> + kfree(log);
>> +
>> + return err;
>> +}
>> +
>> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
>
> Similar to the register kfunc counterpart, can this be moved to btf.c
> instead by extern-ing bpf_struct_ops_init_one()? or there are some other
> structs/functions need to extern?
It is wierd to move a function of bpf_struct_ops to btf.
But, kfunc already did that, I don't mind to follow it.
>
>> +{
>> + struct btf *btf;
>> + int err;
>> +
>> + btf = btf_get_module_btf(st_ops->owner);
>> + if (!btf)
>> + return -EINVAL;
>> + err = register_bpf_struct_ops_btf(st_ops, btf);
>> + btf_put(btf);
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
>> +
>> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>
> The bpf_struct_ops_init() is pretty much only finding the btf
> "module_id" and "common_value_id". Lets use the BTF_ID_LIST to do it
> instead. Then the newly added bpf_struct_ops_init_one() could use a
> proper name bpf_struct_ops_init() instead of having the special "_one"
> suffix.
Got it!
>
>> {
>> - struct bpf_struct_ops *st_ops;
>> s32 module_id, common_value_id;
>> - u32 i;
>> -
>> - /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
>> -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct
>> bpf_struct_ops_##_name);
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
>> if (module_id < 0) {
>> @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct
>> bpf_verifier_log *log)
>> return;
>> }
>> common_value_type = btf_type_by_id(btf, common_value_id);
>> -
>> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
>> - st_ops = bpf_struct_ops[i];
>> - bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>> - }
>> }
>> extern struct btf *btf_vmlinux;
>> @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;
>> static const struct bpf_struct_ops *
>> bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>> {
>> + const struct bpf_struct_ops *st_ops = NULL;
>> + const struct bpf_struct_ops **st_ops_list;
>> unsigned int i;
>> + u32 cnt = 0;
>> if (!value_id || !btf_vmlinux)
>
> The "!btf_vmlinux" should have been changed to "!btf" in the earlier
> patch (patch 2?),
This is not btf. It mean to check if btf_vmlinux is initialized.
It is not necessary anymore.
For checking btf, the following btf_get_struct_ops() will keep cnt zero
if btf is NULL, so it is unnecessary as well.
>
> and is this null check still needed now?
>
>> return NULL;
>> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
>> - if (bpf_struct_ops[i]->value_id == value_id)
>> - return bpf_struct_ops[i];
>> + st_ops_list = btf_get_struct_ops(btf, &cnt);
>> + for (i = 0; i < cnt; i++) {
>> + if (st_ops_list[i]->value_id == value_id) {
>> + st_ops = st_ops_list[i];
>
> nit. Like the change in the earlier patch that is being replaced here,
> directly "return st_ops_list[i];".
Got it!
>
>> + break;
>> + }
>> }
>> - return NULL;
>> + return st_ops;
>> }
>> const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf,
>> u32 type_id)
>> {
>> + const struct bpf_struct_ops *st_ops = NULL;
>> + const struct bpf_struct_ops **st_ops_list;
>> unsigned int i;
>> + u32 cnt;
>> if (!type_id || !btf_vmlinux)
>> return NULL;
>> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
>> - if (bpf_struct_ops[i]->type_id == type_id)
>> - return bpf_struct_ops[i];
>> + st_ops_list = btf_get_struct_ops(btf, &cnt);
>> + for (i = 0; i < cnt; i++) {
>> + if (st_ops_list[i]->type_id == type_id) {
>> + st_ops = st_ops_list[i];
>
> Same.
Ack!
>
>> + break;
>> + }
>> }
>> - return NULL;
>> + return st_ops;
>> }
>> static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void
>> *key,
>> diff --git a/kernel/bpf/bpf_struct_ops_types.h
>> b/kernel/bpf/bpf_struct_ops_types.h
>> deleted file mode 100644
>> index 5678a9ddf817..000000000000
>> --- a/kernel/bpf/bpf_struct_ops_types.h
>> +++ /dev/null
>> @@ -1,12 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0 */
>> -/* internal file - do not include directly */
>> -
>> -#ifdef CONFIG_BPF_JIT
>> -#ifdef CONFIG_NET
>> -BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
>> -#endif
>> -#ifdef CONFIG_INET
>> -#include <net/tcp.h>
>> -BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
>> -#endif
>> -#endif
>
> Seeing this gone is satisfying
>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index be5144dbb53d..990973d6057d 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct
>> btf *btf)
>> /* Returns struct btf corresponding to the struct module.
>> * This function can return NULL or ERR_PTR.
>> */
>> -static struct btf *btf_get_module_btf(const struct module *module)
>> +struct btf *btf_get_module_btf(const struct module *module)
>> {
>> #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>> struct btf_module *btf_mod, *tmp;
>> diff --git a/net/bpf/bpf_dummy_struct_ops.c
>> b/net/bpf/bpf_dummy_struct_ops.c
>> index 5918d1b32e19..724bb7224079 100644
>> --- a/net/bpf/bpf_dummy_struct_ops.c
>> +++ b/net/bpf/bpf_dummy_struct_ops.c
>> @@ -7,7 +7,7 @@
>> #include <linux/bpf.h>
>> #include <linux/btf.h>
>> -extern struct bpf_struct_ops bpf_bpf_dummy_ops;
>> +static struct bpf_struct_ops bpf_bpf_dummy_ops;
>
> Is it still needed ?
Yes, it will be used by bpf_struct_ops_test_run().
>
>
>
Powered by blists - more mailing lists