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;
MIME-Version: 1.0
References: <20230113093427.1666466-1-imagedong@tencent.com>
 <bdca73eb-07e3-2187-c46f-a3f14a9e50a4@oracle.com> <CAEf4BzZ5FNw-j3F8cUpy4knRiM1sqQOOPZnM43Kj8peN9kKQLg@mail.gmail.com>
In-Reply-To: <CAEf4BzZ5FNw-j3F8cUpy4knRiM1sqQOOPZnM43Kj8peN9kKQLg@mail.gmail.com>
From:   Menglong Dong <menglong8.dong@gmail.com>
Date:   Mon, 16 Jan 2023 10:27:37 +0800
Message-ID: <CADxym3YqgvYt71+WhMM4jzp+9uqkNdq3nB9kvBxT=CVM7hwRsA@mail.gmail.com>
Subject: Re: [PATCH] libbpf: replace '.' with '_' in legacy kprobe event name
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     Alan Maguire <alan.maguire@oracle.com>, andrii@kernel.org,
        ast@kernel.org, daniel@iogearbox.net, martin.lau@linux.dev,
        song@kernel.org, yhs@fb.com, john.fastabend@gmail.com,
        kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
        jolsa@kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, Menglong Dong <imagedong@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello,

On Sat, Jan 14, 2023 at 6:07 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 13, 2023 at 6:13 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On 13/01/2023 09:34, menglong8.dong@gmail.com wrote:
> > > From: Menglong Dong <imagedong@tencent.com>
> > >
> > > '.' is not allowed in the event name of kprobe. Therefore, we will get a
> > > EINVAL if the kernel function name has a '.' in legacy kprobe attach
> > > case, such as 'icmp_reply.constprop.0'.
> > >
> > > In order to adapt this case, we need to replace the '.' with other char
> > > in gen_kprobe_legacy_event_name(). And I use '_' for this propose.
> > >
> > > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index fdfb1ca34ced..5d6f6675c2f2 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -9994,9 +9994,16 @@ static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
> > >                                        const char *kfunc_name, size_t offset)
> > >  {
> > >       static int index = 0;
> > > +     int i = 0;
> > >
> > >       snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(), kfunc_name, offset,
> > >                __sync_fetch_and_add(&index, 1));
> > > +
> > > +     while (buf[i] != '\0') {
> > > +             if (buf[i] == '.')
> > > +                     buf[i] = '_';
> > > +             i++;
> > > +     }
> > >  }
> >
> > probably more naturally expressed as a for() loop as is done in
> > gen_uprobe_legacy_event_name(), but not a big deal.
> >
> > Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
> Applied, but tuned to be exactly the same loop as in
> gen_uprobe_legacy_event_name. Thanks.
>

Thanks for your modification, it looks much better now!

> >
> > One issue with the legacy kprobe code is that we don't get test coverage
> > with it on new kernels - I wonder if it would be worth adding a force_legacy
> > option to bpf_kprobe_opts? A separate issue to this change of course, but
> > if we had that we could add some legacy kprobe tests that would run
> > for new kernels as well.
>
> Yep, good idea. If we ever have some bug in the latest greatest kprobe
> implementation, users will have an option to work around that with
> this.
>
> The only thing is that we already have 3 modes: legacy, perf-based
> through ioctl, and bpf_link-based, so I think it should be something
> like
>
> enum kprobe_mode {
>     KPROBE_MODE_DEFAULT = 0, /* latest supported by kernel */
>     KPROBE_MODE_LEGACY,
>     KPROBE_MODE_PERF,
>     KPROBE_MODE_LINK,
> };
>
> LEGACY/PERF/LINK naming should be thought through, just a quick example.
>
> And then just have `enum kprobe_mode mode;` in kprobe_opts, which
> would default to 0 (KPROBE_MODE_DEFAULT).
>
> Would that work?
>

Sounds great, which means I don't have to switch to an older
kernel to test this function for my app.

BTW, should I do this job, (which is my pleasure), or Alan?


Thanks!
Menglong Dong

> >
> > Alan
> > >
> > >  static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
> > >