Message-ID: <5dcbb13e-322f-a8d0-3bcf-d91b72ab51b9@bytedance.com>
Date:   Fri, 24 Dec 2021 15:38:20 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.4.1
Subject: Re: [External] Re: Fix repeated legacy kprobes on same function
Content-Language: en-US
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>,
        Qiang Wang <wangqiang.wq.frank@bytedance.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>, Martin Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        john fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Muchun Song <songmuchun@bytedance.com>,
        duanxiongchun@bytedance.com, shekairui@bytedance.com,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <c84094d2-75c1-a50d-ea9e-9dded5f01fb9@bytedance.com>
 <CAEf4Bza200bB3d-E3rMyxZs7wxijbzJ_0xmRSy+=tHm2Ot14Eg@mail.gmail.com>
From:   Chengming Zhou <zhouchengming@bytedance.com>
In-Reply-To: <CAEf4Bza200bB3d-E3rMyxZs7wxijbzJ_0xmRSy+=tHm2Ot14Eg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2021/12/24 2:57 下午, Andrii Nakryiko wrote:
> On Thu, Dec 23, 2021 at 8:01 PM Qiang Wang
> <wangqiang.wq.frank@bytedance.com> wrote:
>>
>> If repeated legacy kprobes on same function in one process,
>> libbpf will register using the same probe name and got -EBUSY
>> error. So append index to the probe name format to fix this
>> problem.
>>
>> And fix a bug in commit 46ed5fc33db9, which wrongly used the
>> func_name instead of probe_name to register.
>>
>> Fixes: 46ed5fc33db9 ("libbpf: Refactor and simplify legacy kprobe code")
>> Co-developed-by: Chengming Zhou <zhouchengming@bytedance.com>
>> Signed-off-by: Qiang Wang <wangqiang.wq.frank@bytedance.com>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>>
>> ---
>>   tools/lib/bpf/libbpf.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 7c74342bb668..7d1097958459 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -9634,7 +9634,8 @@ static int append_to_file(const char *file, const
>> char *fmt, ...)
>>   static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
>>                                           const char *kfunc_name, size_t
>> offset)
>>   {
>> -       snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx", getpid(),
>> kfunc_name, offset);
>> +       static int index = 0;
>> +       snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(),
>> kfunc_name, offset, index++);
> 
> BCC doesn't add this auto-increment (which is also not thread-safe)
> and it seems like that works fine for all users.
> 

Yes, BCC has the same problem, we will send a fix patch to BCC later.
We thought libbpf was used in single-threaded environment, so will
change to use __sync_fetch_and_add() to keep thread-safe. Thanks for
pointing this out.

> What is the use case where you'd like to attach to the same kernel
> function multiple times with legacy kprobe?
> 

We used many different BPF modules writen by different people in one
monitor process to analyze all data, there maybe repeated legacy kprobes
on the same function. So we want to add a unique index suffix to support
this use case.

>>   }
>>
>>   static int add_kprobe_event_legacy(const char *probe_name, bool retprobe,
>> @@ -9735,7 +9736,7 @@ bpf_program__attach_kprobe_opts(const struct
>> bpf_program *prog,
>>                  gen_kprobe_legacy_event_name(probe_name,
>> sizeof(probe_name),
>>                                               func_name, offset);
>>
>> -               legacy_probe = strdup(func_name);
>> +               legacy_probe = strdup(probe_name);
> 
> please send this as a separate fix
> 

Ok, will do.

Thanks.

>>                  if (!legacy_probe)
>>                          return libbpf_err_ptr(-ENOMEM);
>>
>> --
>> 2.20.1
>>