| Message ID | 20220421132317.1583867-1-asavkov@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6a12b8e20d7e72386594a9dbe7bf2d7fae3b3aa6 |
| Headers | show |
| Series | [bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error | expand |
On Thu, Apr 21, 2022 at 6:23 AM Artem Savkov <asavkov@redhat.com> wrote: > > I am getting the following compilation error for prog_tests/uprobe_autoattach.c > > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c: In function ‘test_uprobe_autoattach’: > ./test_progs.h:209:26: error: pointer ‘mem’ may be used after ‘free’ [-Werror=use-after-free] > > mem variable is now used in one of the asserts so it shouldn't be freed right > away. Move free(mem) after the assert block. The memory is not used, we only compare the value of the pointer itself, we don't dereference. So the compiler is being paranoid here. But while initial version relied on free() to happen before all the ASSERTs, now we don't, so moving free after asserts is fine. > > Fixes: 1717e248014c ("selftests/bpf: Uprobe tests should verify param/return values") > Signed-off-by: Artem Savkov <asavkov@redhat.com> > --- > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > index d6003dc8cc99..35b87c7ba5be 100644 > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > @@ -34,7 +34,6 @@ void test_uprobe_autoattach(void) > > /* trigger & validate shared library u[ret]probes attached by name */ > mem = malloc(malloc_sz); > - free(mem); > > ASSERT_EQ(skel->bss->uprobe_byname_parm1, trigger_val, "check_uprobe_byname_parm1"); > ASSERT_EQ(skel->bss->uprobe_byname_ran, 1, "check_uprobe_byname_ran"); > @@ -44,6 +43,8 @@ void test_uprobe_autoattach(void) > ASSERT_EQ(skel->bss->uprobe_byname2_ran, 3, "check_uprobe_byname2_ran"); > ASSERT_EQ(skel->bss->uretprobe_byname2_rc, mem, "check_uretprobe_byname2_rc"); > ASSERT_EQ(skel->bss->uretprobe_byname2_ran, 4, "check_uretprobe_byname2_ran"); > + > + free(mem); > cleanup: > test_uprobe_autoattach__destroy(skel); > } > -- > 2.35.1 >
On 4/21/22 3:23 PM, Artem Savkov wrote: > I am getting the following compilation error for prog_tests/uprobe_autoattach.c > > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c: In function ‘test_uprobe_autoattach’: > ./test_progs.h:209:26: error: pointer ‘mem’ may be used after ‘free’ [-Werror=use-after-free] > > mem variable is now used in one of the asserts so it shouldn't be freed right > away. Move free(mem) after the assert block. Looks good, but I rephrased this a bit to avoid confusion. It's false positive given we only compare the addresses but don't deref mem, which the compiler might not be able to follow in this case. https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=6a12b8e20d7e72386594a9dbe7bf2d7fae3b3aa6 Thanks, Daniel > Fixes: 1717e248014c ("selftests/bpf: Uprobe tests should verify param/return values") > Signed-off-by: Artem Savkov <asavkov@redhat.com> > --- > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > index d6003dc8cc99..35b87c7ba5be 100644 > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > @@ -34,7 +34,6 @@ void test_uprobe_autoattach(void) > > /* trigger & validate shared library u[ret]probes attached by name */ > mem = malloc(malloc_sz); > - free(mem); > > ASSERT_EQ(skel->bss->uprobe_byname_parm1, trigger_val, "check_uprobe_byname_parm1"); > ASSERT_EQ(skel->bss->uprobe_byname_ran, 1, "check_uprobe_byname_ran"); > @@ -44,6 +43,8 @@ void test_uprobe_autoattach(void) > ASSERT_EQ(skel->bss->uprobe_byname2_ran, 3, "check_uprobe_byname2_ran"); > ASSERT_EQ(skel->bss->uretprobe_byname2_rc, mem, "check_uretprobe_byname2_rc"); > ASSERT_EQ(skel->bss->uretprobe_byname2_ran, 4, "check_uretprobe_byname2_ran"); > + > + free(mem); > cleanup: > test_uprobe_autoattach__destroy(skel); > } >
On Thu, Apr 21, 2022 at 9:53 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 4/21/22 3:23 PM, Artem Savkov wrote: > > I am getting the following compilation error for prog_tests/uprobe_autoattach.c > > > > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c: In function ‘test_uprobe_autoattach’: > > ./test_progs.h:209:26: error: pointer ‘mem’ may be used after ‘free’ [-Werror=use-after-free] > > > > mem variable is now used in one of the asserts so it shouldn't be freed right > > away. Move free(mem) after the assert block. > > Looks good, but I rephrased this a bit to avoid confusion. It's false positive given we > only compare the addresses but don't deref mem, which the compiler might not be able to > follow in this case. > Great, thanks Daniel! > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=6a12b8e20d7e72386594a9dbe7bf2d7fae3b3aa6 > > Thanks, > Daniel > > > Fixes: 1717e248014c ("selftests/bpf: Uprobe tests should verify param/return values") > > Signed-off-by: Artem Savkov <asavkov@redhat.com> > > --- > > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > > index d6003dc8cc99..35b87c7ba5be 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > > @@ -34,7 +34,6 @@ void test_uprobe_autoattach(void) > > > > /* trigger & validate shared library u[ret]probes attached by name */ > > mem = malloc(malloc_sz); > > - free(mem); > > > > ASSERT_EQ(skel->bss->uprobe_byname_parm1, trigger_val, "check_uprobe_byname_parm1"); > > ASSERT_EQ(skel->bss->uprobe_byname_ran, 1, "check_uprobe_byname_ran"); > > @@ -44,6 +43,8 @@ void test_uprobe_autoattach(void) > > ASSERT_EQ(skel->bss->uprobe_byname2_ran, 3, "check_uprobe_byname2_ran"); > > ASSERT_EQ(skel->bss->uretprobe_byname2_rc, mem, "check_uretprobe_byname2_rc"); > > ASSERT_EQ(skel->bss->uretprobe_byname2_ran, 4, "check_uretprobe_byname2_ran"); > > + > > + free(mem); > > cleanup: > > test_uprobe_autoattach__destroy(skel); > > } > > >
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Thu, 21 Apr 2022 15:23:17 +0200 you wrote: > I am getting the following compilation error for prog_tests/uprobe_autoattach.c > > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c: In function ‘test_uprobe_autoattach’: > ./test_progs.h:209:26: error: pointer ‘mem’ may be used after ‘free’ [-Werror=use-after-free] > > mem variable is now used in one of the asserts so it shouldn't be freed right > away. Move free(mem) after the assert block. > > [...] Here is the summary with links: - [bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error https://git.kernel.org/bpf/bpf-next/c/6a12b8e20d7e You are awesome, thank you!
On Thu, Apr 21, 2022 at 06:53:22PM +0200, Daniel Borkmann wrote: > On 4/21/22 3:23 PM, Artem Savkov wrote: > > I am getting the following compilation error for prog_tests/uprobe_autoattach.c > > > > tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c: In function ‘test_uprobe_autoattach’: > > ./test_progs.h:209:26: error: pointer ‘mem’ may be used after ‘free’ [-Werror=use-after-free] > > > > mem variable is now used in one of the asserts so it shouldn't be freed right > > away. Move free(mem) after the assert block. > > Looks good, but I rephrased this a bit to avoid confusion. It's false positive given we > only compare the addresses but don't deref mem, which the compiler might not be able to > follow in this case. > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=6a12b8e20d7e72386594a9dbe7bf2d7fae3b3aa6 Right. Thank you for fixing up the commit message.
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c index d6003dc8cc99..35b87c7ba5be 100644 --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c @@ -34,7 +34,6 @@ void test_uprobe_autoattach(void) /* trigger & validate shared library u[ret]probes attached by name */ mem = malloc(malloc_sz); - free(mem); ASSERT_EQ(skel->bss->uprobe_byname_parm1, trigger_val, "check_uprobe_byname_parm1"); ASSERT_EQ(skel->bss->uprobe_byname_ran, 1, "check_uprobe_byname_ran"); @@ -44,6 +43,8 @@ void test_uprobe_autoattach(void) ASSERT_EQ(skel->bss->uprobe_byname2_ran, 3, "check_uprobe_byname2_ran"); ASSERT_EQ(skel->bss->uretprobe_byname2_rc, mem, "check_uretprobe_byname2_rc"); ASSERT_EQ(skel->bss->uretprobe_byname2_ran, 4, "check_uretprobe_byname2_ran"); + + free(mem); cleanup: test_uprobe_autoattach__destroy(skel); }
I am getting the following compilation error for prog_tests/uprobe_autoattach.c tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c: In function ‘test_uprobe_autoattach’: ./test_progs.h:209:26: error: pointer ‘mem’ may be used after ‘free’ [-Werror=use-after-free] mem variable is now used in one of the asserts so it shouldn't be freed right away. Move free(mem) after the assert block. Fixes: 1717e248014c ("selftests/bpf: Uprobe tests should verify param/return values") Signed-off-by: Artem Savkov <asavkov@redhat.com> --- tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)