Re: [PATCH bpf] bpf: search_bpf_extables should search subprogram extables

[Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>]

On Mon, Jun 5, 2023 at 5:46 PM Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jun 05, 2023 at 04:30:29PM -0700, Alexei Starovoitov wrote:
> > On Mon, Jun 5, 2023 at 9:50 AM Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx> wrote:
> > > +                       if (!aux->func[i]->aux->num_exentries ||
> > > +                           aux->func[i]->aux->extable == NULL)
> > > +                               continue;
> > > +                       e = search_extable(aux->func[i]->aux->extable,
> > > +                           aux->func[i]->aux->num_exentries, addr);
> > > +               }
> > > +       }
> >
> > something odd here.
> > We do bpf_prog_kallsyms_add(func[i]); for each subprog.
> > So bpf_prog_ksym_find() in search_bpf_extables()
> > should be finding ksym and extable of the subprog
> > and not the main prog.
> > The bug is probably elsewhere.
>
> I have a kdump (or more) of this bug so if there's additional state
> you'd like me to share, let me know.

Please convert the test into selftest.
Then everyone will be able to reproduce easily
and it will serve us later to make sure we don't regress.

> With your comments in mind, I took
> another look at the ksym fields in the aux structs.  I have this in the
> main program:
>
>   ksym = {
>     start = 18446744072638420852,
>     end = 18446744072638423040,
>     name = <...>
>     lnode = {
>       next = 0xffff88d9c1065168,
>       prev = 0xffff88da91609168
>     },
>     tnode = {
>       node = {{
>           __rb_parent_color = 18446613068361611640,
>           rb_right = 0xffff88da91609178,
>           rb_left = 0xffff88d9f0c5a578
>         }, {
>           __rb_parent_color = 18446613068361611664,
>           rb_right = 0xffff88da91609190,
>           rb_left = 0xffff88d9f0c5a590
>         }}
>     },
>     prog = true
>   },
>
> and this in the func[0] subprogram:
>
>   ksym = {
>     start = 18446744072638420852,
>     end = 18446744072638423040,
>     name = <...>
>     lnode = {
>       next = 0xffff88da91609168,
>       prev = 0xffffffff981f8990 <bpf_kallsyms>
>     },
>     tnode = {
>       node = {{
>           __rb_parent_color = 18446613068361606520,
>           rb_right = 0x0,
>           rb_left = 0x0
>         }, {
>           __rb_parent_color = 18446613068361606544,
>           rb_right = 0x0,
>           rb_left = 0x0
>         }}
>     },
>     prog = true
>   },
>
> That sure looks like func[0] is a leaf in the rbtree and the main
> program is an intermediate node with leaves.  If that's the case, then
> bpf_prog_ksym_find may have found the main program instead of the
> subprogram.  In that case, do you think it's better to skip the main
> program's call to bpf_prog_ksym_set_addr() if it has subprograms instead
> of searching for subprograms if the main program is found?

I see.
Looks like we're doing double bpf_prog_kallsyms_add().
First in in jit_subprogs():
        for (i = 0; i < env->subprog_cnt; i++) {
                bpf_prog_lock_ro(func[i]);
                bpf_prog_kallsyms_add(func[i]);
        }
and then again:
bpf_prog_kallsyms_add(prog);
in bpf_prog_load().

because func[0] is the main prog.

We are also doing double bpf_prog_lock_ro() for main prog,
but that's not causing harm.

The fix is probably just this:

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1e38584d497c..89266dac9c12 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17633,7 +17633,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
        /* finally lock prog and jit images for all functions and
         * populate kallsysm
         */
-       for (i = 0; i < env->subprog_cnt; i++) {
+       for (i = 1; i < env->subprog_cnt; i++) {
                bpf_prog_lock_ro(func[i]);
                bpf_prog_kallsyms_add(func[i]);
        }