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>
In-Reply-To: <bdca73eb-07e3-2187-c46f-a3f14a9e50a4@oracle.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 13 Jan 2023 14:07:12 -0800
Message-ID: <CAEf4BzZ5FNw-j3F8cUpy4knRiM1sqQOOPZnM43Kj8peN9kKQLg@mail.gmail.com>
Subject: Re: [PATCH] libbpf: replace '.' with '_' in legacy kprobe event name
To:     Alan Maguire <alan.maguire@oracle.com>
Cc:     menglong8.dong@gmail.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

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.

>
> 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?

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