diff mbox series

[bpf-next] selftests/bpf: make use of PROCMAP_QUERY ioctl if available

Message ID 20240806230319.869734-1-andrii@kernel.org (mailing list archive)
State Accepted
Commit 4e9e07603ecdfd0816838132f354239771c51edd
Delegated to: BPF
Headers show
Series [bpf-next] selftests/bpf: make use of PROCMAP_QUERY ioctl if available | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 7 this patch: 7
netdev/build_tools success Errors and warnings before: 10 this patch: 10
netdev/cc_maintainers warning 12 maintainers not CCed: kpsingh@kernel.org shuah@kernel.org haoluo@google.com john.fastabend@gmail.com jolsa@kernel.org linux-kselftest@vger.kernel.org yonghong.song@linux.dev martin.lau@linux.dev mykolal@fb.com song@kernel.org eddyz87@gmail.com sdf@fomichev.me
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch fail ERROR: do not initialise globals to 0 WARNING: line length of 105 exceeds 80 columns WARNING: line length of 120 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 fail Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18

Commit Message

Andrii Nakryiko Aug. 6, 2024, 11:03 p.m. UTC 
Instead of parsing text-based /proc/<pid>/maps file, try to use
PROCMAP_QUERY ioctl() to simplify and speed up data fetching.
This logic is used to do uprobe file offset calculation, so any bugs in
this logic would manifest as failing uprobe BPF selftests.

This also serves as a simple demonstration of one of the intended uses.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/test_progs.c    |   3 +
 tools/testing/selftests/bpf/test_progs.h    |   2 +
 tools/testing/selftests/bpf/trace_helpers.c | 104 +++++++++++++++++---
 3 files changed, 94 insertions(+), 15 deletions(-)

Comments

Jiri Olsa Aug. 7, 2024, 9:07 a.m. UTC | #1 
On Tue, Aug 06, 2024 at 04:03:19PM -0700, Andrii Nakryiko wrote:

SNIP

>  ssize_t get_uprobe_offset(const void *addr)
>  {
> -	size_t start, end, base;
> -	char buf[256];
> -	bool found = false;
> +	size_t start, base, end;
>  	FILE *f;
> +	char buf[256];
> +	int err, flags;
>  
>  	f = fopen("/proc/self/maps", "r");
>  	if (!f)
>  		return -errno;
>  
> -	while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> -		if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> -			found = true;
> -			break;
> +	/* requested executable VMA only */
> +	err = procmap_query(fileno(f), addr, PROCMAP_QUERY_VMA_EXECUTABLE, &start, &base, &flags);
> +	if (err == -EOPNOTSUPP) {
> +		bool found = false;
> +
> +		while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> +			if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> +				found = true;
> +				break;
> +			}
> +		}
> +		if (!found) {
> +			fclose(f);
> +			return -ESRCH;
>  		}
> +	} else if (err) {
> +		fclose(f);
> +		return err;

I feel like I commented on this before, so feel free to ignore me,
but this seems similar to the code below, could be in one function

anyway it's good for follow up

there was another selftest in the original patchset adding benchmark
for the procfs query interface, is it coming in as well?

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

>  	}
> -
>  	fclose(f);
>  
> -	if (!found)
> -		return -ESRCH;
> -
>  #if defined(__powerpc64__) && defined(_CALL_ELF) && _CALL_ELF == 2
>  
>  #define OP_RT_RA_MASK   0xffff0000UL
> @@ -307,15 +371,25 @@ ssize_t get_rel_offset(uintptr_t addr)
>  	size_t start, end, offset;
>  	char buf[256];
>  	FILE *f;
> +	int err, flags;
>  
>  	f = fopen("/proc/self/maps", "r");
>  	if (!f)
>  		return -errno;
>  
> -	while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &offset) == 4) {
> -		if (addr >= start && addr < end) {
> -			fclose(f);
> -			return (size_t)addr - start + offset;
> +	err = procmap_query(fileno(f), (const void *)addr, 0, &start, &offset, &flags);
> +	if (err == 0) {
> +		fclose(f);
> +		return (size_t)addr - start + offset;
> +	} else if (err != -EOPNOTSUPP) {
> +		fclose(f);
> +		return err;
> +	} else if (err) {
> +		while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &offset) == 4) {
> +			if (addr >= start && addr < end) {
> +				fclose(f);
> +				return (size_t)addr - start + offset;
> +			}
>  		}
>  	}
>  
> -- 
> 2.43.5
> 
>
Andrii Nakryiko Aug. 7, 2024, 3:17 p.m. UTC | #2 
On Wed, Aug 7, 2024 at 2:07 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Aug 06, 2024 at 04:03:19PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> >  ssize_t get_uprobe_offset(const void *addr)
> >  {
> > -     size_t start, end, base;
> > -     char buf[256];
> > -     bool found = false;
> > +     size_t start, base, end;
> >       FILE *f;
> > +     char buf[256];
> > +     int err, flags;
> >
> >       f = fopen("/proc/self/maps", "r");
> >       if (!f)
> >               return -errno;
> >
> > -     while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > -             if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > -                     found = true;
> > -                     break;
> > +     /* requested executable VMA only */
> > +     err = procmap_query(fileno(f), addr, PROCMAP_QUERY_VMA_EXECUTABLE, &start, &base, &flags);
> > +     if (err == -EOPNOTSUPP) {
> > +             bool found = false;
> > +
> > +             while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > +                     if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > +                             found = true;
> > +                             break;
> > +                     }
> > +             }
> > +             if (!found) {
> > +                     fclose(f);
> > +                     return -ESRCH;
> >               }
> > +     } else if (err) {
> > +             fclose(f);
> > +             return err;
>
> I feel like I commented on this before, so feel free to ignore me,
> but this seems similar to the code below, could be in one function

Do you mean get_rel_offset()? That one is for data symbols (USDT
semaphores), so it a) doesn't do arch-specific adjustments and b)
doesn't filter by executable flag. So while the logic of parsing and
finding VMA is similar, conditions and adjustments are different. It
feels not worth combining them, tbh.

>
> anyway it's good for follow up
>
> there was another selftest in the original patchset adding benchmark
> for the procfs query interface, is it coming in as well?

I didn't plan to send it, given it's not really a test. But I can put
it on Github somewhere, probably, if it's useful.

>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> jirka
>
> >       }
> > -
> >       fclose(f);
> >
> > -     if (!found)
> > -             return -ESRCH;
> > -
> >  #if defined(__powerpc64__) && defined(_CALL_ELF) && _CALL_ELF == 2
> >
> >  #define OP_RT_RA_MASK   0xffff0000UL
> > @@ -307,15 +371,25 @@ ssize_t get_rel_offset(uintptr_t addr)
> >       size_t start, end, offset;
> >       char buf[256];
> >       FILE *f;
> > +     int err, flags;
> >
> >       f = fopen("/proc/self/maps", "r");
> >       if (!f)
> >               return -errno;
> >
> > -     while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &offset) == 4) {
> > -             if (addr >= start && addr < end) {
> > -                     fclose(f);
> > -                     return (size_t)addr - start + offset;
> > +     err = procmap_query(fileno(f), (const void *)addr, 0, &start, &offset, &flags);
> > +     if (err == 0) {
> > +             fclose(f);
> > +             return (size_t)addr - start + offset;
> > +     } else if (err != -EOPNOTSUPP) {
> > +             fclose(f);
> > +             return err;
> > +     } else if (err) {
> > +             while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &offset) == 4) {
> > +                     if (addr >= start && addr < end) {
> > +                             fclose(f);
> > +                             return (size_t)addr - start + offset;
> > +                     }
> >               }
> >       }
> >
> > --
> > 2.43.5
> >
> >
Alexei Starovoitov Aug. 23, 2024, 2:47 p.m. UTC | #3 
On Wed, Aug 7, 2024 at 8:17 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 2:07 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Tue, Aug 06, 2024 at 04:03:19PM -0700, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > >  ssize_t get_uprobe_offset(const void *addr)
> > >  {
> > > -     size_t start, end, base;
> > > -     char buf[256];
> > > -     bool found = false;
> > > +     size_t start, base, end;
> > >       FILE *f;
> > > +     char buf[256];
> > > +     int err, flags;
> > >
> > >       f = fopen("/proc/self/maps", "r");
> > >       if (!f)
> > >               return -errno;
> > >
> > > -     while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > > -             if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > > -                     found = true;
> > > -                     break;
> > > +     /* requested executable VMA only */
> > > +     err = procmap_query(fileno(f), addr, PROCMAP_QUERY_VMA_EXECUTABLE, &start, &base, &flags);
> > > +     if (err == -EOPNOTSUPP) {
> > > +             bool found = false;
> > > +
> > > +             while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > > +                     if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > > +                             found = true;
> > > +                             break;
> > > +                     }
> > > +             }
> > > +             if (!found) {
> > > +                     fclose(f);
> > > +                     return -ESRCH;
> > >               }
> > > +     } else if (err) {
> > > +             fclose(f);
> > > +             return err;
> >
> > I feel like I commented on this before, so feel free to ignore me,
> > but this seems similar to the code below, could be in one function
>
> Do you mean get_rel_offset()? That one is for data symbols (USDT
> semaphores), so it a) doesn't do arch-specific adjustments and b)
> doesn't filter by executable flag. So while the logic of parsing and
> finding VMA is similar, conditions and adjustments are different. It
> feels not worth combining them, tbh.
>
> >
> > anyway it's good for follow up
> >
> > there was another selftest in the original patchset adding benchmark
> > for the procfs query interface, is it coming in as well?
>
> I didn't plan to send it, given it's not really a test. But I can put
> it on Github somewhere, probably, if it's useful.

With and without this selftest applied I see:
./test_progs -t uprobe
#416     uprobe:OK
#417     uprobe_autoattach:OK
[   47.448908] ref_ctr_offset mismatch. inode: 0x16b5f921 offset:
0x2d4297 ref_ctr_offset(old): 0x45e8b56 ref_ctr_offset(new): 0x45e8b54
#418/1   uprobe_multi_test/skel_api:OK

Is this a known issue?

Applied anyway.
patchwork-bot+netdevbpf@kernel.org Aug. 23, 2024, 2:50 p.m. UTC | #4 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue,  6 Aug 2024 16:03:19 -0700 you wrote:
> Instead of parsing text-based /proc/<pid>/maps file, try to use
> PROCMAP_QUERY ioctl() to simplify and speed up data fetching.
> This logic is used to do uprobe file offset calculation, so any bugs in
> this logic would manifest as failing uprobe BPF selftests.
> 
> This also serves as a simple demonstration of one of the intended uses.
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: make use of PROCMAP_QUERY ioctl if available
    https://git.kernel.org/bpf/bpf-next/c/4e9e07603ecd

You are awesome, thank you!
Andrii Nakryiko Aug. 23, 2024, 4:53 p.m. UTC | #5 
On Fri, Aug 23, 2024 at 7:48 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 8:17 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Aug 7, 2024 at 2:07 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Tue, Aug 06, 2024 at 04:03:19PM -0700, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > >  ssize_t get_uprobe_offset(const void *addr)
> > > >  {
> > > > -     size_t start, end, base;
> > > > -     char buf[256];
> > > > -     bool found = false;
> > > > +     size_t start, base, end;
> > > >       FILE *f;
> > > > +     char buf[256];
> > > > +     int err, flags;
> > > >
> > > >       f = fopen("/proc/self/maps", "r");
> > > >       if (!f)
> > > >               return -errno;
> > > >
> > > > -     while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > > > -             if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > > > -                     found = true;
> > > > -                     break;
> > > > +     /* requested executable VMA only */
> > > > +     err = procmap_query(fileno(f), addr, PROCMAP_QUERY_VMA_EXECUTABLE, &start, &base, &flags);
> > > > +     if (err == -EOPNOTSUPP) {
> > > > +             bool found = false;
> > > > +
> > > > +             while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > > > +                     if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > > > +                             found = true;
> > > > +                             break;
> > > > +                     }
> > > > +             }
> > > > +             if (!found) {
> > > > +                     fclose(f);
> > > > +                     return -ESRCH;
> > > >               }
> > > > +     } else if (err) {
> > > > +             fclose(f);
> > > > +             return err;
> > >
> > > I feel like I commented on this before, so feel free to ignore me,
> > > but this seems similar to the code below, could be in one function
> >
> > Do you mean get_rel_offset()? That one is for data symbols (USDT
> > semaphores), so it a) doesn't do arch-specific adjustments and b)
> > doesn't filter by executable flag. So while the logic of parsing and
> > finding VMA is similar, conditions and adjustments are different. It
> > feels not worth combining them, tbh.
> >
> > >
> > > anyway it's good for follow up
> > >
> > > there was another selftest in the original patchset adding benchmark
> > > for the procfs query interface, is it coming in as well?
> >
> > I didn't plan to send it, given it's not really a test. But I can put
> > it on Github somewhere, probably, if it's useful.
>
> With and without this selftest applied I see:
> ./test_progs -t uprobe
> #416     uprobe:OK
> #417     uprobe_autoattach:OK
> [   47.448908] ref_ctr_offset mismatch. inode: 0x16b5f921 offset:
> 0x2d4297 ref_ctr_offset(old): 0x45e8b56 ref_ctr_offset(new): 0x45e8b54
> #418/1   uprobe_multi_test/skel_api:OK
>
> Is this a known issue?

Yeah, that's not due to my changes. It's an old warning in uprobe
internals, but I think we should remove it, because it can trivially
be triggered by a user. Which is what Jiri is doing intentionally in
one of selftests to test uprobe failure handling.

Jiri, maybe let's get rid of this warning?

>
> Applied anyway.

Thanks! I just found another auto-archived patch of mine, the one
adding multi-uprobe benchmarks (see patchworks). Please take a look
and maybe apply, when you get a chance.
Jiri Olsa Aug. 23, 2024, 9:31 p.m. UTC | #6 
On Fri, Aug 23, 2024 at 09:53:03AM -0700, Andrii Nakryiko wrote:
> On Fri, Aug 23, 2024 at 7:48 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Aug 7, 2024 at 8:17 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Aug 7, 2024 at 2:07 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Tue, Aug 06, 2024 at 04:03:19PM -0700, Andrii Nakryiko wrote:
> > > >
> > > > SNIP
> > > >
> > > > >  ssize_t get_uprobe_offset(const void *addr)
> > > > >  {
> > > > > -     size_t start, end, base;
> > > > > -     char buf[256];
> > > > > -     bool found = false;
> > > > > +     size_t start, base, end;
> > > > >       FILE *f;
> > > > > +     char buf[256];
> > > > > +     int err, flags;
> > > > >
> > > > >       f = fopen("/proc/self/maps", "r");
> > > > >       if (!f)
> > > > >               return -errno;
> > > > >
> > > > > -     while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > > > > -             if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > > > > -                     found = true;
> > > > > -                     break;
> > > > > +     /* requested executable VMA only */
> > > > > +     err = procmap_query(fileno(f), addr, PROCMAP_QUERY_VMA_EXECUTABLE, &start, &base, &flags);
> > > > > +     if (err == -EOPNOTSUPP) {
> > > > > +             bool found = false;
> > > > > +
> > > > > +             while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > > > > +                     if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > > > > +                             found = true;
> > > > > +                             break;
> > > > > +                     }
> > > > > +             }
> > > > > +             if (!found) {
> > > > > +                     fclose(f);
> > > > > +                     return -ESRCH;
> > > > >               }
> > > > > +     } else if (err) {
> > > > > +             fclose(f);
> > > > > +             return err;
> > > >
> > > > I feel like I commented on this before, so feel free to ignore me,
> > > > but this seems similar to the code below, could be in one function
> > >
> > > Do you mean get_rel_offset()? That one is for data symbols (USDT
> > > semaphores), so it a) doesn't do arch-specific adjustments and b)
> > > doesn't filter by executable flag. So while the logic of parsing and
> > > finding VMA is similar, conditions and adjustments are different. It
> > > feels not worth combining them, tbh.
> > >
> > > >
> > > > anyway it's good for follow up
> > > >
> > > > there was another selftest in the original patchset adding benchmark
> > > > for the procfs query interface, is it coming in as well?
> > >
> > > I didn't plan to send it, given it's not really a test. But I can put
> > > it on Github somewhere, probably, if it's useful.
> >
> > With and without this selftest applied I see:
> > ./test_progs -t uprobe
> > #416     uprobe:OK
> > #417     uprobe_autoattach:OK
> > [   47.448908] ref_ctr_offset mismatch. inode: 0x16b5f921 offset:
> > 0x2d4297 ref_ctr_offset(old): 0x45e8b56 ref_ctr_offset(new): 0x45e8b54
> > #418/1   uprobe_multi_test/skel_api:OK
> >
> > Is this a known issue?
> 
> Yeah, that's not due to my changes. It's an old warning in uprobe
> internals, but I think we should remove it, because it can trivially
> be triggered by a user. Which is what Jiri is doing intentionally in
> one of selftests to test uprobe failure handling.
> 
> Jiri, maybe let's get rid of this warning?

fine with me.. or change to pr_debug if that makes sense for anyone,
Oleg, any idea/preference?

jirka

> 
> >
> > Applied anyway.
> 
> Thanks! I just found another auto-archived patch of mine, the one
> adding multi-uprobe benchmarks (see patchworks). Please take a look
> and maybe apply, when you get a chance.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 60fafa2f1ed7..bf262b42a488 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -33,6 +33,8 @@  __weak void backtrace_symbols_fd(void *const *buffer, int size, int fd)
 	dprintf(fd, "<backtrace not supported>\n");
 }
 
+int env_verbosity = 0;
+
 static bool verbose(void)
 {
 	return env.verbosity > VERBOSE_NONE;
@@ -862,6 +864,7 @@  static error_t parse_arg(int key, char *arg, struct argp_state *state)
 				return -EINVAL;
 			}
 		}
+		env_verbosity = env->verbosity;
 
 		if (verbose()) {
 			if (setenv("SELFTESTS_VERBOSE", "1", 1) == -1) {
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index cb9d6d46826b..43e3136166b3 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -95,6 +95,8 @@  struct test_state {
 	FILE *stdout_saved;
 };
 
+extern int env_verbosity;
+
 struct test_env {
 	struct test_selector test_selector;
 	struct test_selector subtest_selector;
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 465d196c7165..1bfd881c0e07 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -10,6 +10,8 @@ 
 #include <pthread.h>
 #include <unistd.h>
 #include <linux/perf_event.h>
+#include <linux/fs.h>
+#include <sys/ioctl.h>
 #include <sys/mman.h>
 #include "trace_helpers.h"
 #include <linux/limits.h>
@@ -244,29 +246,91 @@  int kallsyms_find(const char *sym, unsigned long long *addr)
 	return err;
 }
 
+#ifdef PROCMAP_QUERY
+int env_verbosity __weak = 0;
+
+int procmap_query(int fd, const void *addr, __u32 query_flags, size_t *start, size_t *offset, int *flags)
+{
+	char path_buf[PATH_MAX], build_id_buf[20];
+	struct procmap_query q;
+	int err;
+
+	memset(&q, 0, sizeof(q));
+	q.size = sizeof(q);
+	q.query_flags = query_flags;
+	q.query_addr = (__u64)addr;
+	q.vma_name_addr = (__u64)path_buf;
+	q.vma_name_size = sizeof(path_buf);
+	q.build_id_addr = (__u64)build_id_buf;
+	q.build_id_size = sizeof(build_id_buf);
+
+	err = ioctl(fd, PROCMAP_QUERY, &q);
+	if (err < 0) {
+		err = -errno;
+		if (err == -ENOTTY)
+			return -EOPNOTSUPP; /* ioctl() not implemented yet */
+		if (err == -ENOENT)
+			return -ESRCH; /* vma not found */
+		return err;
+	}
+
+	if (env_verbosity >= 1) {
+		printf("VMA FOUND (addr %08lx): %08lx-%08lx %c%c%c%c %08lx %02x:%02x %ld %s (build ID: %s, %d bytes)\n",
+		       (long)addr, (long)q.vma_start, (long)q.vma_end,
+		       (q.vma_flags & PROCMAP_QUERY_VMA_READABLE) ? 'r' : '-',
+		       (q.vma_flags & PROCMAP_QUERY_VMA_WRITABLE) ? 'w' : '-',
+		       (q.vma_flags & PROCMAP_QUERY_VMA_EXECUTABLE) ? 'x' : '-',
+		       (q.vma_flags & PROCMAP_QUERY_VMA_SHARED) ? 's' : 'p',
+		       (long)q.vma_offset, q.dev_major, q.dev_minor, (long)q.inode,
+		       q.vma_name_size ? path_buf : "",
+		       q.build_id_size ? "YES" : "NO",
+		       q.build_id_size);
+	}
+
+	*start = q.vma_start;
+	*offset = q.vma_offset;
+	*flags = q.vma_flags;
+	return 0;
+}
+#else
+int procmap_query(int fd, const void *addr, size_t *start, size_t *offset, int *flags)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
 ssize_t get_uprobe_offset(const void *addr)
 {
-	size_t start, end, base;
-	char buf[256];
-	bool found = false;
+	size_t start, base, end;
 	FILE *f;
+	char buf[256];
+	int err, flags;
 
 	f = fopen("/proc/self/maps", "r");
 	if (!f)
 		return -errno;
 
-	while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
-		if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
-			found = true;
-			break;
+	/* requested executable VMA only */
+	err = procmap_query(fileno(f), addr, PROCMAP_QUERY_VMA_EXECUTABLE, &start, &base, &flags);
+	if (err == -EOPNOTSUPP) {
+		bool found = false;
+
+		while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
+			if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
+				found = true;
+				break;
+			}
+		}
+		if (!found) {
+			fclose(f);
+			return -ESRCH;
 		}
+	} else if (err) {
+		fclose(f);
+		return err;
 	}
-
 	fclose(f);
 
-	if (!found)
-		return -ESRCH;
-
 #if defined(__powerpc64__) && defined(_CALL_ELF) && _CALL_ELF == 2
 
 #define OP_RT_RA_MASK   0xffff0000UL
@@ -307,15 +371,25 @@  ssize_t get_rel_offset(uintptr_t addr)
 	size_t start, end, offset;
 	char buf[256];
 	FILE *f;
+	int err, flags;
 
 	f = fopen("/proc/self/maps", "r");
 	if (!f)
 		return -errno;
 
-	while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &offset) == 4) {
-		if (addr >= start && addr < end) {
-			fclose(f);
-			return (size_t)addr - start + offset;
+	err = procmap_query(fileno(f), (const void *)addr, 0, &start, &offset, &flags);
+	if (err == 0) {
+		fclose(f);
+		return (size_t)addr - start + offset;
+	} else if (err != -EOPNOTSUPP) {
+		fclose(f);
+		return err;
+	} else if (err) {
+		while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &offset) == 4) {
+			if (addr >= start && addr < end) {
+				fclose(f);
+				return (size_t)addr - start + offset;
+			}
 		}
 	}