On Tue, Apr 5, 2022 at 3:28 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On Mon, 4 Apr 2022, Andrii Nakryiko wrote:
>
> > On Wed, Mar 30, 2022 at 8:27 AM Alan Maguire <alan.maguire@xxxxxxxxxx> 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.
Ok, curious. I'm not a big expert on PLT and stuff, but would be
curious to look at such an ELF file and see if we are missing
something that can be recovered from PLT relocations maybe? If you
happen to have a small ELF with repro case, please share.
>
> > 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.
Yep, definitely, provided we are sure it will keep working reliably :)
>
> > 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.
Awesome, aarch64 seems like a logical addition, thanks!
>
> > Thanks, great work and great addition to
> > libbpf!
> >
>
> Thanks again!
