From: Martin KaFai Lau <martin.lau@linux.dev>
To: thinker.li@gmail.com
Cc: sinquersw@gmail.com, 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 15:57:04 -0700 [thread overview]
Message-ID: <dc241e84-bc0a-b529-f032-9bd27abc3d41@linux.dev> (raw)
In-Reply-To: <20230920155923.151136-5-thinker.li@gmail.com>
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?
> 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.
> {
> 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 22:57 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 [this message]
2023-09-25 23:25 ` Kui-Feng Lee
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=dc241e84-bc0a-b529-f032-9bd27abc3d41@linux.dev \
--to=martin.lau@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=kernel-team@meta.com \
--cc=kuifeng@meta.com \
--cc=sinquersw@gmail.com \
--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