On Wed, Mar 01, 2023 at 11:31:09AM -0800, Andrii Nakryiko wrote: > On Wed, Mar 1, 2023 at 10:40 AM Daniel Müller <deso@xxxxxxxxxx> wrote: > > > > This change adds support for attaching uprobes to shared objects located > > in APKs, which is relevant for Android systems where various libraries > > may reside in APKs. To make that happen, we extend the syntax for the > > "binary path" argument to attach to with that supported by various > > Android tools: > > <archive>!/<binary-in-archive> > > > > For example: > > /system/app/test-app/test-app.apk!/lib/arm64-v8a/libc++_shared.so > > > > APKs need to be specified via full path, i.e., we do not attempt to > > resolve mere file names by searching system directories. > > > > We cannot currently test this functionality end-to-end in an automated > > fashion, because it relies on an Android system being present, but there > > is no support for that in CI. I have tested the functionality manually, > > by creating a libbpf program containing a uretprobe, attaching it to a > > function inside a shared object inside an APK, and verifying the sanity > > of the returned values. > > > > Signed-off-by: Daniel Müller <deso@xxxxxxxxxx> > > --- > > tools/lib/bpf/libbpf.c | 92 ++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 85 insertions(+), 7 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 4543e9..e6b99a 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -53,6 +53,7 @@ > > #include "libbpf_internal.h" > > #include "hashmap.h" > > #include "bpf_gen_internal.h" > > +#include "zip.h" > > > > #ifndef BPF_FS_MAGIC > > #define BPF_FS_MAGIC 0xcafe4a11 > > @@ -10702,6 +10703,69 @@ static long elf_find_func_offset_from_file(const char *binary_path, const char * > > return ret; > > } > > > > +/* Find offset of function name in archive specified by path. Currently > > + * supported are .zip files that do not compress their contents, as used on > > + * Android in the form of APKs, for example. "file_name" is the name of the ELF > > + * file inside the archive. "func_name" matches symbol name or name@@LIB for > > + * library functions. > > + * > > + * An overview of the APK format specifically provided here: > > + * https://en.wikipedia.org/w/index.php?title=Apk_(file_format)&oldid=1139099120#Package_contents > > + */ > > +static long elf_find_func_offset_from_archive(const char *archive_path, const char *file_name, > > + const char *func_name) > > +{ > > + struct zip_archive *archive; > > + struct zip_entry entry; > > + long ret = -ENOENT; > > + int err; > > + Elf *elf; > > + > > + archive = zip_archive_open(archive_path); > > + if (IS_ERR(archive)) { > > Unfortunately, this won't work with the libbpf_err_ptr() approach that > you used inside zip_archive_open(). Since libbpf v1.0 libbpf_err_ptr() > will return NULL on error (and so this IS_ERR() check will always be > false, and subsequent PTR_ERR() would be returning 0) and only set > errno to actual error. This was meant to be used mostly for > user-facing APIs. Thanks for pointing that out. > Given zip_archive_open() is internal, explicit PTR_ERR() use as you do > below makes most sense. Please update and respin. Sure. > > + pr_warn("zip: failed to open %s: %ld\n", archive_path, PTR_ERR(archive)); > > + return PTR_ERR(archive); > > err = PTR_ERR(archive); and use err in pr_warn() and return? > > and it's not clear why you need both ret and err, it should be fine to > just use ret (long vs int doesn't hurt error propagation) Changed. Thanks, Daniel
