[PATCH bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error

[PATCH bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error

[Artem Savkov]

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(-)

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

Re: [PATCH bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error

[Daniel Borkmann]

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);
>   }
> 

Re: [PATCH bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error

[Andrii Nakryiko]

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);
> >   }
> >
>

Re: [PATCH bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] selftests/bpf: fix prog_tests/uprobe_autoattach compilation error

[Artem Savkov]

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.

-- 
 Artem