[PATCH bpf v2] selftests/bpf: Fix freplace_link segfault in tailcalls prog test

[PATCH bpf v2] selftests/bpf: Fix freplace_link segfault in tailcalls prog test

[Tengda Wu]

There are two bpf_link__destroy(freplace_link) calls in
test_tailcall_bpf2bpf_freplace(). After the first bpf_link__destroy()
is called, if the following bpf_map_{update,delete}_elem() throws an
exception, it will jump to the "out" label and call bpf_link__destroy()
again, causing double free and eventually leading to a segfault.

Fix it by directly resetting freplace_link to NULL after the first
bpf_link__destroy() call.

Fixes: 021611d33e78 ("selftests/bpf: Add test to verify tailcall and freplace restrictions")
Signed-off-by: Tengda Wu <wutengda@huaweicloud.com>
---
 tools/testing/selftests/bpf/prog_tests/tailcalls.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index 544144620ca6..a12fa0521ccc 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -1602,6 +1602,7 @@ static void test_tailcall_bpf2bpf_freplace(void)
 	err = bpf_link__destroy(freplace_link);
 	if (!ASSERT_OK(err, "destroy link"))
 		goto out;
+	freplace_link = NULL;
 
 	/* OK to update prog_array map then delete element from the map. */
 
-- 
2.34.1

Re: [PATCH bpf v2] selftests/bpf: Fix freplace_link segfault in tailcalls prog test

[Leon Hwang]

On 22/1/25 10:28, Tengda Wu wrote:
> There are two bpf_link__destroy(freplace_link) calls in
> test_tailcall_bpf2bpf_freplace(). After the first bpf_link__destroy()
> is called, if the following bpf_map_{update,delete}_elem() throws an
> exception, it will jump to the "out" label and call bpf_link__destroy()
> again, causing double free and eventually leading to a segfault.
> 
> Fix it by directly resetting freplace_link to NULL after the first
> bpf_link__destroy() call.
> 
> Fixes: 021611d33e78 ("selftests/bpf: Add test to verify tailcall and freplace restrictions")
> Signed-off-by: Tengda Wu <wutengda@huaweicloud.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/tailcalls.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index 544144620ca6..a12fa0521ccc 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -1602,6 +1602,7 @@ static void test_tailcall_bpf2bpf_freplace(void)
>  	err = bpf_link__destroy(freplace_link);
>  	if (!ASSERT_OK(err, "destroy link"))
>  		goto out;
> +	freplace_link = NULL;
>  
>  	/* OK to update prog_array map then delete element from the map. */
>  

LGTM.

Reviewed-by: Leon Hwang <leon.hwang@linux.dev>

Re: [PATCH bpf v2] selftests/bpf: Fix freplace_link segfault in tailcalls prog test

[Andrii Nakryiko]

On Tue, Jan 21, 2025 at 6:29 PM Tengda Wu <wutengda@huaweicloud.com> wrote:
>
> There are two bpf_link__destroy(freplace_link) calls in
> test_tailcall_bpf2bpf_freplace(). After the first bpf_link__destroy()
> is called, if the following bpf_map_{update,delete}_elem() throws an
> exception, it will jump to the "out" label and call bpf_link__destroy()
> again, causing double free and eventually leading to a segfault.
>
> Fix it by directly resetting freplace_link to NULL after the first
> bpf_link__destroy() call.
>
> Fixes: 021611d33e78 ("selftests/bpf: Add test to verify tailcall and freplace restrictions")
> Signed-off-by: Tengda Wu <wutengda@huaweicloud.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/tailcalls.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index 544144620ca6..a12fa0521ccc 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -1602,6 +1602,7 @@ static void test_tailcall_bpf2bpf_freplace(void)
>         err = bpf_link__destroy(freplace_link);
>         if (!ASSERT_OK(err, "destroy link"))
>                 goto out;
> +       freplace_link = NULL;
>

libbpf will free the link even if bpf_link__destroy() returns error,
so goto out above will still cause double-free. I moved `freplace_link
= NULL` two lines up to avoid this. applied to bpf-next

>         /* OK to update prog_array map then delete element from the map. */
>
> --
> 2.34.1
>
>

Re: [PATCH bpf v2] selftests/bpf: Fix freplace_link segfault in tailcalls prog test

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 22 Jan 2025 10:28:38 +0800 you wrote:
> There are two bpf_link__destroy(freplace_link) calls in
> test_tailcall_bpf2bpf_freplace(). After the first bpf_link__destroy()
> is called, if the following bpf_map_{update,delete}_elem() throws an
> exception, it will jump to the "out" label and call bpf_link__destroy()
> again, causing double free and eventually leading to a segfault.
> 
> Fix it by directly resetting freplace_link to NULL after the first
> bpf_link__destroy() call.
> 
> [...]

Here is the summary with links:
  - [bpf,v2] selftests/bpf: Fix freplace_link segfault in tailcalls prog test
    https://git.kernel.org/bpf/bpf-next/c/b420b5756549

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf v2] selftests/bpf: Fix freplace_link segfault in tailcalls prog test

[Tengda Wu]

On 2025/1/25 2:34, Andrii Nakryiko wrote:
> On Tue, Jan 21, 2025 at 6:29 PM Tengda Wu <wutengda@huaweicloud.com> wrote:
>>
>> There are two bpf_link__destroy(freplace_link) calls in
>> test_tailcall_bpf2bpf_freplace(). After the first bpf_link__destroy()
>> is called, if the following bpf_map_{update,delete}_elem() throws an
>> exception, it will jump to the "out" label and call bpf_link__destroy()
>> again, causing double free and eventually leading to a segfault.
>>
>> Fix it by directly resetting freplace_link to NULL after the first
>> bpf_link__destroy() call.
>>
>> Fixes: 021611d33e78 ("selftests/bpf: Add test to verify tailcall and freplace restrictions")
>> Signed-off-by: Tengda Wu <wutengda@huaweicloud.com>
>> ---
>>  tools/testing/selftests/bpf/prog_tests/tailcalls.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> index 544144620ca6..a12fa0521ccc 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> @@ -1602,6 +1602,7 @@ static void test_tailcall_bpf2bpf_freplace(void)
>>         err = bpf_link__destroy(freplace_link);
>>         if (!ASSERT_OK(err, "destroy link"))
>>                 goto out;
>> +       freplace_link = NULL;
>>
> 
> libbpf will free the link even if bpf_link__destroy() returns error,
> so goto out above will still cause double-free. I moved `freplace_link
> = NULL` two lines up to avoid this. applied to bpf-next

Yes, you're right. Sorry that I didn't consider this case. Thanks for pointing it out.

> 
>>         /* OK to update prog_array map then delete element from the map. */
>>
>> --
>> 2.34.1
>>
>>