From: Kui-Feng Lee <sinquersw@gmail.com>
To: Martin KaFai Lau <martin.lau@linux.dev>, thinker.li@gmail.com
Cc: kuifeng@meta.com, bpf@vger.kernel.org, ast@kernel.org,
song@kernel.org, kernel-team@meta.com, andrii@kernel.org
Subject: Re: [RFC bpf-next v3 04/11] bpf: attach a module BTF to a bpf_struct_ops
Date: Mon, 25 Sep 2023 16:25:19 -0700 [thread overview]
Message-ID: <5826f846-b3f3-cf24-5a7e-9ee12d24ec4e@gmail.com> (raw)
In-Reply-To: <dc241e84-bc0a-b529-f032-9bd27abc3d41@linux.dev>
On 9/25/23 15:57, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Every struct_ops type should has an associated module BTF to provide type
>> information since we are going to allow modules to define and register
>> new
>> struct_ops types. New types may exist only in module itself, and the
>> kernel
>> BTF (vmlinux) doesn't know it at all. The attached module BTF here is
>> going
>> to be used to get correct btf and resolve type IDs of a struct_ops map.
>>
>> However, it doesn't use the attached module BTF until we are ready to
>> switch to registration function in subsequent patches.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> include/linux/bpf.h | 5 +++--
>> kernel/bpf/bpf_struct_ops.c | 27 ++++++++++++++++++---------
>> kernel/bpf/verifier.c | 2 +-
>> 3 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 67554f2f81b7..0776cb584b3f 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1626,6 +1626,7 @@ struct bpf_struct_ops {
>> void (*unreg)(void *kdata);
>> int (*update)(void *kdata, void *old_kdata);
>> int (*validate)(void *kdata);
>> + const struct btf *btf;
>> const struct btf_type *type;
>> const struct btf_type *value_type;
>> const char *name;
>> @@ -1641,7 +1642,7 @@ struct bpf_struct_ops_mod {
>> #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
>> #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) +
>> POISON_POINTER_DELTA))
>> -const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
>> +const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct
>> btf *btf);
>> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log
>> *log);
>> bool bpf_struct_ops_get(const void *kdata);
>> void bpf_struct_ops_put(const void *kdata);
>> @@ -1684,7 +1685,7 @@ int bpf_struct_ops_test_run(struct bpf_prog
>> *prog, const union bpf_attr *kattr,
>> union bpf_attr __user *uattr);
>> #endif
>> #else
>> -static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32
>> type_id)
>> +static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32
>> type_id, struct btf *btf)
>> {
>> return NULL;
>> }
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index cd688e9033b5..7c2ef53687ef 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -174,6 +174,10 @@ static void bpf_struct_ops_init_one(struct
>> bpf_struct_ops *st_ops,
>> pr_warn("Error in init bpf_struct_ops %s\n",
>> st_ops->name);
>> } else {
>> + /* XXX: We need a owner (module) here to company
>> + * with type_id and value_id.
>> + */
>> + st_ops->btf = btf;
>
> I looked ahead in patch 5 and 7, I suspect I sort of getting why it does
> not need a refcount for the btf here.
>
> Instead of having st_ops->btf pointing back to its containing btf, is it
> enough to store the btf in st_"map"->btf?
Basically, we can put the pointer to btf at either st_ops or st_maps.
Since a st_ops is always associated with a btf, and its st_maps need
a pointer to st_ops, keep the btf pointer at st_ops is reasonable for me
and efficient.
What is your concern about st_ops->btf?
>
>> st_ops->type_id = type_id;
>> st_ops->type = t;
>> st_ops->value_id = value_id;
>> @@ -210,7 +214,7 @@ void bpf_struct_ops_init(struct btf *btf, struct
>> bpf_verifier_log *log)
>> extern struct btf *btf_vmlinux;
>> static const struct bpf_struct_ops *
>> -bpf_struct_ops_find_value(u32 value_id)
>> +bpf_struct_ops_find_value(u32 value_id, struct btf *btf)
>
> nit. 'struct btf *btf' as the first argument, consistent with other btf
> search functions.
Got it!
>
>> {
>> unsigned int i;
>> @@ -225,7 +229,7 @@ bpf_struct_ops_find_value(u32 value_id)
>> return NULL;
>> }
>> -const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
>> +const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct
>> btf *btf)
>
> same here.
>
>> {
>> unsigned int i;
>> @@ -305,7 +309,7 @@ static void bpf_struct_ops_map_put_progs(struct
>> bpf_struct_ops_map *st_map)
>> }
>> }
>> -static int check_zero_holes(const struct btf_type *t, void *data)
>> +static int check_zero_holes(const struct btf *btf, const struct
>> btf_type *t, void *data)
>> {
>> const struct btf_member *member;
>> u32 i, moff, msize, prev_mend = 0;
>> @@ -317,8 +321,8 @@ static int check_zero_holes(const struct btf_type
>> *t, void *data)
>> memchr_inv(data + prev_mend, 0, moff - prev_mend))
>> return -EINVAL;
>> - mtype = btf_type_by_id(btf_vmlinux, member->type);
>> - mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
>> + mtype = btf_type_by_id(btf, member->type);
>> + mtype = btf_resolve_size(btf, mtype, &msize);
>> if (IS_ERR(mtype))
>> return PTR_ERR(mtype);
>> prev_mend = moff + msize;
>> @@ -371,7 +375,7 @@ static long bpf_struct_ops_map_update_elem(struct
>> bpf_map *map, void *key,
>> const struct bpf_struct_ops *st_ops = st_map->st_ops;
>> struct bpf_struct_ops_value *uvalue, *kvalue;
>> const struct btf_member *member;
>> - const struct btf_type *t = st_ops->type;
>> + const struct btf_type *t;
>> struct bpf_tramp_links *tlinks;
>> void *udata, *kdata;
>> int prog_fd, err;
>> @@ -381,15 +385,20 @@ static long
>> bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>> if (flags)
>> return -EINVAL;
>> + if (!st_ops)
>> + return -EINVAL;
>
> Why this new NULL check is needed?
>
>> +
>> + t = st_ops->type;
>> +
>> if (*(u32 *)key != 0)
>> return -E2BIG;
>> - err = check_zero_holes(st_ops->value_type, value);
>> + err = check_zero_holes(st_ops->btf, st_ops->value_type, value);
>> if (err)
>> return err;
>> uvalue = value;
>> - err = check_zero_holes(t, uvalue->data);
>> + err = check_zero_holes(st_ops->btf, t, uvalue->data);
>> if (err)
>> return err;
>> @@ -660,7 +669,7 @@ static struct bpf_map
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>> struct bpf_map *map;
>> int ret;
>> - st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
>> + st_ops =
>> bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
>> if (!st_ops)
>> return ERR_PTR(-ENOTSUPP);
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index a7178ecf676d..99b45501951c 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19631,7 +19631,7 @@ static int check_struct_ops_btf_id(struct
>> bpf_verifier_env *env)
>> }
>> btf_id = prog->aux->attach_btf_id;
>> - st_ops = bpf_struct_ops_find(btf_id);
>> + st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
>> if (!st_ops) {
>> verbose(env, "attach_btf_id %u is not a supported struct\n",
>> btf_id);
>
next prev parent reply other threads:[~2023-09-25 23:25 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-20 15:59 [RFC bpf-next v3 00/11] Registrating struct_ops types from modules thinker.li
2023-09-20 15:59 ` [RFC bpf-next v3 01/11] bpf: refactory struct_ops type initialization to a function thinker.li
2023-09-20 15:59 ` [RFC bpf-next v3 02/11] bpf: add struct_ops_tab to btf thinker.li
2023-09-25 21:10 ` Martin KaFai Lau
2023-09-25 21:45 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops thinker.li
2023-09-25 23:07 ` Martin KaFai Lau
2023-09-25 23:13 ` Kui-Feng Lee
2023-09-25 23:31 ` Martin KaFai Lau
2023-09-26 0:19 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 04/11] bpf: attach a module BTF to a bpf_struct_ops thinker.li
2023-09-25 22:57 ` Martin KaFai Lau
2023-09-25 23:25 ` Kui-Feng Lee [this message]
2023-09-20 15:59 ` [RFC bpf-next v3 05/11] bpf: hold module for bpf_struct_ops_map thinker.li
2023-09-25 23:23 ` Martin KaFai Lau
2023-09-25 23:42 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 06/11] bpf: validate value_type thinker.li
2023-09-26 1:03 ` Martin KaFai Lau
2023-09-27 20:27 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 07/11] bpf, net: switch to storing struct_ops in btf thinker.li
2023-09-26 0:02 ` Martin KaFai Lau
2023-09-26 0:18 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs thinker.li
2023-09-25 22:58 ` Andrii Nakryiko
2023-09-25 23:50 ` Kui-Feng Lee
2023-09-26 0:24 ` Martin KaFai Lau
2023-09-26 0:58 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 09/11] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
2023-09-25 23:09 ` Andrii Nakryiko
2023-09-26 0:12 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 10/11] bpf: export btf_ctx_access to modules thinker.li
2023-09-20 15:59 ` [RFC bpf-next v3 11/11] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
2023-09-26 1:19 ` Martin KaFai Lau
2023-09-26 1:33 ` [RFC bpf-next v3 00/11] Registrating struct_ops types from modules Martin KaFai Lau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5826f846-b3f3-cf24-5a7e-9ee12d24ec4e@gmail.com \
--to=sinquersw@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=kernel-team@meta.com \
--cc=kuifeng@meta.com \
--cc=martin.lau@linux.dev \
--cc=song@kernel.org \
--cc=thinker.li@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox