by Alexei Starovoitov

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] bpf/btf: Add a function to search a member of a struct/union

On Wed, Jul 26, 2023 at 6:00 AM Masami Hiramatsu (Google)
<[email protected]> wrote:
>
> From: Masami Hiramatsu (Google) <[email protected]>
>
> Add btf_find_struct_member() API to search a member of a given data structure
> or union from the member's name.
>
> Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
> Reviewed-by: Alan Maguire <[email protected]>
> ---
> Changes in v3:
> - Remove simple input check.
> - Fix unneeded IS_ERR_OR_NULL() check for btf_type_by_id().
> - Move the code next to btf_get_func_param().
> - Use for_each_member() macro instead of for-loop.
> - Use btf_type_skip_modifiers() instead of btf_type_by_id().
> ---
> include/linux/btf.h | 3 +++
> kernel/bpf/btf.c | 35 +++++++++++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 20e3a07eef8f..4b10d57ceee0 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -226,6 +226,9 @@ const struct btf_type *btf_find_func_proto(const char *func_name,
> struct btf **btf_p);
> const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> s32 *nr);
> +const struct btf_member *btf_find_struct_member(struct btf *btf,
> + const struct btf_type *type,
> + const char *member_name);
>
> #define for_each_member(i, struct_type, member) \
> for (i = 0, member = btf_type_member(struct_type); \
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f7b25c615269..5258870030fc 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -958,6 +958,41 @@ const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s3
> return NULL;
> }
>
> +/*
> + * Find a member of data structure/union by name and return it.
> + * Return NULL if not found, or -EINVAL if parameter is invalid.
> + */
> +const struct btf_member *btf_find_struct_member(struct btf *btf,
> + const struct btf_type *type,
> + const char *member_name)
> +{
> + const struct btf_member *member, *ret;
> + const char *name;
> + int i;
> +
> + if (!btf_type_is_struct(type))
> + return ERR_PTR(-EINVAL);
> +
> + for_each_member(i, type, member) {
> + if (!member->name_off) {
> + /* unnamed member: dig deeper */
> + type = btf_type_skip_modifiers(btf, member->type, NULL);
> + if (type) {
> + ret = btf_find_struct_member(btf, type,
> + member_name);

Unbounded recursion in the kernel? Ouch. That might cause issues.
Please convert it to a loop. It doesn't have to be recursive.

> + if (!IS_ERR_OR_NULL(ret))
> + return ret;
> + }
> + } else {
> + name = btf_name_by_offset(btf, member->name_off);
> + if (name && !strcmp(member_name, name))
> + return member;
> + }
> + }
> +
> + return NULL;
> +}
> +
> #define BTF_SHOW_MAX_ITER 10
>
> #define BTF_KIND_BIT(kind) (1ULL << kind)
>

2023-07-28 00:31:12

by Masami Hiramatsu

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] bpf/btf: Add a function to search a member of a struct/union

On Thu, 27 Jul 2023 08:39:02 -0700
Alexei Starovoitov <[email protected]> wrote:

> On Wed, Jul 26, 2023 at 6:00 AM Masami Hiramatsu (Google)
> <[email protected]> wrote:
> >
> > From: Masami Hiramatsu (Google) <[email protected]>
> >
> > Add btf_find_struct_member() API to search a member of a given data structure
> > or union from the member's name.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
> > Reviewed-by: Alan Maguire <[email protected]>
> > ---
> > Changes in v3:
> > - Remove simple input check.
> > - Fix unneeded IS_ERR_OR_NULL() check for btf_type_by_id().
> > - Move the code next to btf_get_func_param().
> > - Use for_each_member() macro instead of for-loop.
> > - Use btf_type_skip_modifiers() instead of btf_type_by_id().
> > ---
> > include/linux/btf.h | 3 +++
> > kernel/bpf/btf.c | 35 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 20e3a07eef8f..4b10d57ceee0 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -226,6 +226,9 @@ const struct btf_type *btf_find_func_proto(const char *func_name,
> > struct btf **btf_p);
> > const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> > s32 *nr);
> > +const struct btf_member *btf_find_struct_member(struct btf *btf,
> > + const struct btf_type *type,
> > + const char *member_name);
> >
> > #define for_each_member(i, struct_type, member) \
> > for (i = 0, member = btf_type_member(struct_type); \
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index f7b25c615269..5258870030fc 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -958,6 +958,41 @@ const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s3
> > return NULL;
> > }
> >
> > +/*
> > + * Find a member of data structure/union by name and return it.
> > + * Return NULL if not found, or -EINVAL if parameter is invalid.
> > + */
> > +const struct btf_member *btf_find_struct_member(struct btf *btf,
> > + const struct btf_type *type,
> > + const char *member_name)
> > +{
> > + const struct btf_member *member, *ret;
> > + const char *name;
> > + int i;
> > +
> > + if (!btf_type_is_struct(type))
> > + return ERR_PTR(-EINVAL);
> > +
> > + for_each_member(i, type, member) {
> > + if (!member->name_off) {
> > + /* unnamed member: dig deeper */
> > + type = btf_type_skip_modifiers(btf, member->type, NULL);
> > + if (type) {
> > + ret = btf_find_struct_member(btf, type,
> > + member_name);
>
> Unbounded recursion in the kernel? Ouch. That might cause issues.
> Please convert it to a loop. It doesn't have to be recursive.

Oh, I thought non-name union will not be nested so much. But yes, if there is
any issue on BTF itself, it is not safe.
Let me make it fixed stacked loop.

Thank you!

>
> > + if (!IS_ERR_OR_NULL(ret))
> > + return ret;
> > + }
> > + } else {
> > + name = btf_name_by_offset(btf, member->name_off);
> > + if (name && !strcmp(member_name, name))
> > + return member;
> > + }
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > #define BTF_SHOW_MAX_ITER 10
> >
> > #define BTF_KIND_BIT(kind) (1ULL << kind)
> >

--
Masami Hiramatsu (Google) <[email protected]>