Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Jul 2023 09:09:20 +0900
From:   Masami Hiramatsu (Google) <mhiramat@kernel.org>
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     linux-trace-kernel@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        bpf <bpf@vger.kernel.org>, Sven Schnelle <svens@linux.ibm.com>,
        Alexei Starovoitov <ast@kernel.org>
Subject: Re: [PATCH v3 3/9] bpf/btf: Add a function to search a member of a
 struct/union
Message-Id: <20230728090920.82a2088da3406847a8bc3d48@kernel.org>
In-Reply-To: <CAADnVQJjZt=e-nSOmrxGJa59DLEQfaJupyx3RfwQhqXx8Vghmw@mail.gmail.com>
References: <169037639315.607919.2613476171148037242.stgit@devnote2>
        <169037642351.607919.10234149030120807556.stgit@devnote2>
        <CAADnVQJjZt=e-nSOmrxGJa59DLEQfaJupyx3RfwQhqXx8Vghmw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 27 Jul 2023 08:39:02 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Wed, Jul 26, 2023 at 6:00 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > 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) <mhiramat@kernel.org>
> > Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >  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) <mhiramat@kernel.org>