Message ID | 20230113093427.1666466-1-imagedong@tencent.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp177790wrt; Fri, 13 Jan 2023 01:49:40 -0800 (PST) X-Google-Smtp-Source: AMrXdXs0v/9ZK6JfMNj7+Ri3rXVMZozFqGcxufDCtHrOeyGflV18vKl3fa+2nuVmN+mPCQKoX3VN X-Received: by 2002:aa7:db47:0:b0:46d:b89a:de1e with SMTP id n7-20020aa7db47000000b0046db89ade1emr65089804edt.1.1673603380030; Fri, 13 Jan 2023 01:49:40 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673603379; cv=none; d=google.com; s=arc-20160816; b=05I1csTmUDixp/M/KX7AsLmBROL4eliZXKeuHX6xsF0U9ExaiEc+SRyk7kRXqwgufF K1kH2S84kswwgx79t47J1EESRdc37iK9t1DqAofj7uM4i6U14HoWYor/przgv9g9mnoA jCc+nEznJnaQQD7g2ub5b6X8SRpBrGWzcBygvh+tvzlfcGcQ8I9Fn+B21XydyceiDvUo O2DwJgUq7ult0oG5dZlWBEXLUunHGx54xBsbn28wdcT6BREnVOB8LrPwyR6HZ1DP1Jbd F0k/Wuzz7MsUBQ58v4ZcmlvtNMsLxHIBMwF4Je7v8EUz4ILNBUYkP9mgJBZy2PVeLowh iloQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=LuIB1D5VRsHEDevL2A4uFAIi6WCJULM9GsA/lvt8gRg=; b=y/wwuQQAqpNGcPPO3bo/b8GBzckxhwcZgYs5Ffm6U4zy3WF5rBthUDuCpUdySVpjZD eCm+BCRSNrHLqmmkCBrN95xJ5Q/o3g0TAXYbfD++1olnIsCUjUtIec+4ui1TWlDsAaRe 9/OXklVQ1P5ALtk4EPxEl9Hrq2fWds8ZRC2mMuzITSXfMx1o7aVnfI33+wCIVTl7PGyf 362ub5Y95SCfhXrqsLoSpBUioKxUXFI8YCMqAl8xjyXao0ty7Qz0zgnYF/BxWu1J15s5 Gu0SxoX3/ic72PHkubtbDYAKqoKGQqW1j9KNZTEdk/QjWzcPK7Irx9Y165HlqvU2lGCb F9oA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b="mnDB/X5D"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y14-20020a056402270e00b0046a84b922a2si24554334edd.499.2023.01.13.01.49.16; Fri, 13 Jan 2023 01:49:39 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b="mnDB/X5D"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241050AbjAMJr2 (ORCPT <rfc822;callmefire3@gmail.com> + 99 others); Fri, 13 Jan 2023 04:47:28 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36514 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237529AbjAMJqo (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 13 Jan 2023 04:46:44 -0500 Received: from mail-pl1-x643.google.com (mail-pl1-x643.google.com [IPv6:2607:f8b0:4864:20::643]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8C56781D69; Fri, 13 Jan 2023 01:35:42 -0800 (PST) Received: by mail-pl1-x643.google.com with SMTP id y1so22918736plb.2; Fri, 13 Jan 2023 01:35:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=LuIB1D5VRsHEDevL2A4uFAIi6WCJULM9GsA/lvt8gRg=; b=mnDB/X5DlObF+SjP5BeF0jehFm9HeXGYxNn4oN8HTB5R4Fqbz441FRARuNYzyJuGGF 9Wc35fGmyl8YZeGv5nrkcOOG/VEZtx+VzOdELjedCFaH4aaXLZO+12PR0Gq0EsaIo+Bb SlgdQ/yKtYlScsrl9EDoGckqyYL8SHYnZl5WAv+nRcjQAuMS0pSQozp8r6G3WJYHu9oU e92u++dtOC1/mPIVhqA5IQ72filvcgQbdcFLwElLthbc+WovdtqTkLgYjjBCa8YsPiCx KvkgHka8A5MdFYIGvEnTmKCMaN9lxFgyEhHuFZ9om9sa3IUH4f9/44t5Y1KEH7Hvkoqq QrZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=LuIB1D5VRsHEDevL2A4uFAIi6WCJULM9GsA/lvt8gRg=; b=Ht3FcFiSSm39tNFYYR22y2DtqboRjZz6V3b2z95U653nC8t8smoIafxQPu9OyrIgCe GseTWUYA8NA8jDNEefXE1U15QqXgip7rILDRHGr1PS4T36420OsVJX3jrqrdlQSrzWXF iSyUZM82E/FcqFq7bcUCwQUXOz0+//w8fei8kky4a7qneYreeU0/n8lQa25Yh6JKhBQP Y1qFSR2+PJkipo7qAy4efdv/GRWVLWvJX3aLNBZSCyO+p8l3wjzeBQ9TJeUGJXRV9Xrs GBYnMopjKE3ebksJddlVtbRoW174OLETnJq/5lTLX2nl18bMhIo32mc8sJbXgg8Zq0s8 FZ6g== X-Gm-Message-State: AFqh2kpTi5R2QVDzU8hcFtlLVpFaniwih/0CgRKpXrrusGWxrIqjMfpz 1FxGqobyqqZIbrEMJm9dkVo= X-Received: by 2002:a17:902:7c17:b0:192:6b28:2b4e with SMTP id x23-20020a1709027c1700b001926b282b4emr65625792pll.69.1673602542027; Fri, 13 Jan 2023 01:35:42 -0800 (PST) Received: from localhost.localdomain ([203.205.141.12]) by smtp.gmail.com with ESMTPSA id y12-20020a17090322cc00b001896af10ca7sm13674311plg.134.2023.01.13.01.35.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 Jan 2023 01:35:41 -0800 (PST) From: menglong8.dong@gmail.com X-Google-Original-From: imagedong@tencent.com To: andrii@kernel.org Cc: 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> Subject: [PATCH] libbpf: replace '.' with '_' in legacy kprobe event name Date: Fri, 13 Jan 2023 17:34:27 +0800 Message-Id: <20230113093427.1666466-1-imagedong@tencent.com> X-Mailer: git-send-email 2.39.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1754900337810755766?= X-GMAIL-MSGID: =?utf-8?q?1754900337810755766?= |
Series |
libbpf: replace '.' with '_' in legacy kprobe event name
|
|
Commit Message
Menglong Dong
Jan. 13, 2023, 9:34 a.m. UTC
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(+)
Comments
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> 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. Alan > > static int add_kprobe_event_legacy(const char *probe_name, bool retprobe, >
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, > >
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Fri, 13 Jan 2023 17:34:27 +0800 you 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. > > [...] Here is the summary with links: - libbpf: replace '.' with '_' in legacy kprobe event name https://git.kernel.org/bpf/bpf-next/c/2fa074536590 You are awesome, thank you!
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, > > >
On 16/01/2023 02:27, Menglong Dong wrote: > 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? >> Looks good - I'd missed the "no BPF link" case, it'd be great to test that too. So for legacy mode, we'd force using the legacy codepath, and to simulate the PERF mode where BPF link isn't supported I think we'd need to add to bpf_perf_event_opts so that we could choose the "no bpf link" code path rather than purely relying on the kernel support test. This would be nice as it would allow us to test other "pre-BPF link" attach targets too. So I think we need add an option to bpf_perf_event_opts for when KPROBE_MODE_PERF is set, such as PE_MODE_PERF or PE_MODE_NO_BPF_LINK. All of this would generalize to uprobe too I think; having a perf option makes that straightforward I suspect. > > 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? > > Feel free to take this on if you've got time; I'm trying to get the dwarves patches covering support for .isra functions out as soon as possible so it would probably be a week or so before I get to this. Something like the above combined with updating the attach_probe selftests to run through the various attach modes would be great for testing legacy codepaths on newer kernels. We could perhaps rework the test_attach_probe() function to take an attach mode argument, and add subtests for each attach mode (skipping if it wasn't supported). Thanks! Alan > Thanks! > Menglong Dong > >>> >>> Alan >>>> >>>> static int add_kprobe_event_legacy(const char *probe_name, bool retprobe, >>>>
On Mon, Jan 16, 2023 at 6:27 PM Alan Maguire <alan.maguire@oracle.com> wrote: > > On 16/01/2023 02:27, Menglong Dong wrote: > > 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? > >> > > Looks good - I'd missed the "no BPF link" case, it'd be great to test that too. > My mistake, I forgot to add the 'bpf-next' tag :) > So for legacy mode, we'd force using the legacy codepath, and to simulate the > PERF mode where BPF link isn't supported I think we'd need to add to bpf_perf_event_opts > so that we could choose the "no bpf link" code path rather than purely relying on the > kernel support test. > > This would be nice as it would allow us to test other "pre-BPF link" attach targets > too. > > So I think we need add an option to bpf_perf_event_opts for when KPROBE_MODE_PERF is set, > such as PE_MODE_PERF or PE_MODE_NO_BPF_LINK. > > All of this would generalize to uprobe too I think; having a perf option makes that > straightforward I suspect. > > > > > 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? > > > > > > Feel free to take this on if you've got time; I'm trying to get the dwarves patches > covering support for .isra functions out as soon as possible so it would probably be > a week or so before I get to this. Something like the above combined with updating > the attach_probe selftests to run through the various attach modes would be great for > testing legacy codepaths on newer kernels. We could perhaps rework the test_attach_probe() > function to take an attach mode argument, and add subtests for each attach mode (skipping > if it wasn't supported). Thanks! > Okay, I'll have a try! > Alan > > > Thanks! > > Menglong Dong > > > >>> > >>> Alan > >>>> > >>>> static int add_kprobe_event_legacy(const char *probe_name, bool retprobe, > >>>>
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++; + } } static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,