Date:   Tue, 5 Apr 2022 11:27:40 +0100 (IST)
From:   Alan Maguire <alan.maguire@...cle.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
cc:     Alan Maguire <alan.maguire@...cle.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Yucong Sun <sunyucong@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v5 bpf-next 0/5] libbpf: name-based u[ret]probe attach

On Mon, 4 Apr 2022, Andrii Nakryiko wrote:

> On Wed, Mar 30, 2022 at 8:27 AM Alan Maguire <alan.maguire@...cle.com> wrote:
> >
> > This patch series focuses on supporting name-based attach - similar
> > to that supported for kprobes - for uprobe BPF programs.
> >
> > Currently attach for such probes is done by determining the offset
> > manually, so the aim is to try and mimic the simplicity of kprobe
> > attach, making use of uprobe opts to specify a name string.
> > Patch 1 supports expansion of the binary_path argument used for
> > bpf_program__attach_uprobe_opts(), allowing it to determine paths
> > for programs and shared objects automatically, allowing for
> > specification of "libc.so.6" rather than the full path
> > "/usr/lib64/libc.so.6".
> >
> > Patch 2 adds the "func_name" option to allow uprobe attach by
> > name; the mechanics are described there.
> >
> > Having name-based support allows us to support auto-attach for
> > uprobes; patch 3 adds auto-attach support while attempting
> > to handle backwards-compatibility issues that arise.  The format
> > supported is
> >
> > u[ret]probe/binary_path:[raw_offset|function[+offset]]
> >
> > For example, to attach to libc malloc:
> >
> > SEC("uprobe//usr/lib64/libc.so.6:malloc")
> >
> > ..or, making use of the path computation mechanisms introduced in patch 1
> >
> > SEC("uprobe/libc.so.6:malloc")
> >
> > Finally patch 4 add tests to the attach_probe selftests covering
> > attach by name, with patch 5 covering skeleton auto-attach.
> >
> > Changes since v4 [1]:
> > - replaced strtok_r() usage with copying segments from static char *; avoids
> >   unneeded string allocation (Andrii, patch 1)
> > - switched to using access() instead of stat() when checking path-resolved
> >   binary (Andrii, patch 1)
> > - removed computation of .plt offset for instrumenting shared library calls
> >   within binaries.  Firstly it proved too brittle, and secondly it was somewhat
> >   unintuitive in that this form of instrumentation did not support function+offset
> >   as the "local function in binary" and "shared library function in shared library"
> >   cases did.  We can still instrument library calls, just need to do it in the
> >   library .so (patch 2)
> 
> ah, that's too bad, it seemed like a nice and clever idea. What was
> brittle? Curious to learn for the future.
> 

On Ubuntu, Daniel reported selftest failures which corresponded to the 
cases where we attached to a library function in a non-library - i.e. used 
the .plt computations.  I reproduced this failure myself, and it seemed
that although we were correctly computing the size of the .plt initial
code - 16 bytes - and each .plt entry - again 16 bytes - finding the 
.rela.plt entry and using its index as the basis for figuring out which
.plt entry to instrument wasn't working. 
 
Specifically, the .rela.plt entry for "free" was 146, but the actual .plt 
location of free was the 372 entry in the .plt table.  I'm not clear on 
why, but the the correspondence betweeen .rela.plt and .plt order 
isn't present on Ubuntu.  

> The fact that function+offset isn't supported int this "mode" 
seems
> totally fine to me, we can provide a nice descriptive error in this
> case anyways.
> 

I'll try and figure out exactly what's going on above; would be nice if we 
can still add this in the future.

> Anyways, all the added functionality makes sense and we can further
> improve on it if necessary and possible. I've found a few small issues
> with your patches and fixed some of them while applying, please do a
> follow up for the rest.

Yep, will do, thanks for the fix-ups! Ilya has fixed a few of the issues 
too, so I'll have some follow-ups for the rest shortly. I'll take a look
at adding aarch64 to libbpf CI too, that would be great.

> Thanks, great work and great addition to
> libbpf!
> 

Thanks again!

Powered by blists - more mailing lists