diff mbox series

[bpf-next] selftests/xsk: fix double free

Message ID 20220929090133.7869-1-magnus.karlsson@gmail.com (mailing list archive)
State Accepted
Commit 5f388bba7acbdb097a9e7ed932a39b40f7eb2acf
Delegated to: BPF
Headers show
Series [bpf-next] selftests/xsk: fix double free | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 12 maintainers not CCed: sdf@google.com john.fastabend@gmail.com andrii@kernel.org yhs@fb.com haoluo@google.com linux-kselftest@vger.kernel.org jolsa@kernel.org kpsingh@kernel.org song@kernel.org shuah@kernel.org mykolal@fb.com martin.lau@linux.dev
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix

Commit Message

Magnus Karlsson Sept. 29, 2022, 9:01 a.m. UTC 
From: Magnus Karlsson <magnus.karlsson@intel.com>

Fix a double free at exit of the test suite.

Fixes: a693ff3ed561 ("selftests/xsk: Add support for executing tests on physical device")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/testing/selftests/bpf/xskxceiver.c | 3 ---
 1 file changed, 3 deletions(-)


base-commit: 8526f0d6135f77451566463ace6f0fb8b72cedaa

Comments

Fijalkowski, Maciej Sept. 29, 2022, 10:44 a.m. UTC | #1 
On Thu, Sep 29, 2022 at 11:01:33AM +0200, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Fix a double free at exit of the test suite.
> 
> Fixes: a693ff3ed561 ("selftests/xsk: Add support for executing tests on physical device")
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index ef33309bbe49..d1a5f3218c34 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -1953,9 +1953,6 @@ int main(int argc, char **argv)
>  
>  	pkt_stream_delete(tx_pkt_stream_default);
>  	pkt_stream_delete(rx_pkt_stream_default);
> -	free(ifobj_rx->umem);
> -	if (!ifobj_tx->shared_umem)
> -		free(ifobj_tx->umem);
>  	ifobject_delete(ifobj_tx);
>  	ifobject_delete(ifobj_rx);

So basically we free this inside ifobject_delete().

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

>  
> 
> base-commit: 8526f0d6135f77451566463ace6f0fb8b72cedaa
> -- 
> 2.34.1
>
Martin KaFai Lau Sept. 30, 2022, 12:52 a.m. UTC | #2 
On 9/29/22 3:44 AM, Maciej Fijalkowski wrote:
> On Thu, Sep 29, 2022 at 11:01:33AM +0200, Magnus Karlsson wrote:
>> From: Magnus Karlsson <magnus.karlsson@intel.com>
>>
>> Fix a double free at exit of the test suite.
>>
>> Fixes: a693ff3ed561 ("selftests/xsk: Add support for executing tests on physical device")
>> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
>> ---
>>   tools/testing/selftests/bpf/xskxceiver.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
>> index ef33309bbe49..d1a5f3218c34 100644
>> --- a/tools/testing/selftests/bpf/xskxceiver.c
>> +++ b/tools/testing/selftests/bpf/xskxceiver.c
>> @@ -1953,9 +1953,6 @@ int main(int argc, char **argv)
>>   
>>   	pkt_stream_delete(tx_pkt_stream_default);
>>   	pkt_stream_delete(rx_pkt_stream_default);
>> -	free(ifobj_rx->umem);
>> -	if (!ifobj_tx->shared_umem)
shared_umem means ifobj_rx->umem and ifobj_tx->umem are the same?  No special 
handling is needed and ifobject_delete() will handle it?

>> -		free(ifobj_tx->umem);
>>   	ifobject_delete(ifobj_tx);
>>   	ifobject_delete(ifobj_rx);
> 
> So basically we free this inside ifobject_delete().
Magnus Karlsson Sept. 30, 2022, 7:52 a.m. UTC | #3 
On Fri, Sep 30, 2022 at 2:52 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 9/29/22 3:44 AM, Maciej Fijalkowski wrote:
> > On Thu, Sep 29, 2022 at 11:01:33AM +0200, Magnus Karlsson wrote:
> >> From: Magnus Karlsson <magnus.karlsson@intel.com>
> >>
> >> Fix a double free at exit of the test suite.
> >>
> >> Fixes: a693ff3ed561 ("selftests/xsk: Add support for executing tests on physical device")
> >> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> >> ---
> >>   tools/testing/selftests/bpf/xskxceiver.c | 3 ---
> >>   1 file changed, 3 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> >> index ef33309bbe49..d1a5f3218c34 100644
> >> --- a/tools/testing/selftests/bpf/xskxceiver.c
> >> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> >> @@ -1953,9 +1953,6 @@ int main(int argc, char **argv)
> >>
> >>      pkt_stream_delete(tx_pkt_stream_default);
> >>      pkt_stream_delete(rx_pkt_stream_default);
> >> -    free(ifobj_rx->umem);
> >> -    if (!ifobj_tx->shared_umem)
> shared_umem means ifobj_rx->umem and ifobj_tx->umem are the same?  No special
> handling is needed and ifobject_delete() will handle it?

You are correct, we will still have a double free in that case. Thanks
for spotting. Will send a v2.

> >> -            free(ifobj_tx->umem);
> >>      ifobject_delete(ifobj_tx);
> >>      ifobject_delete(ifobj_rx);
> >
> > So basically we free this inside ifobject_delete().
>
Magnus Karlsson Sept. 30, 2022, 2:31 p.m. UTC | #4 
On Fri, Sep 30, 2022 at 9:52 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:
>
> On Fri, Sep 30, 2022 at 2:52 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
> >
> > On 9/29/22 3:44 AM, Maciej Fijalkowski wrote:
> > > On Thu, Sep 29, 2022 at 11:01:33AM +0200, Magnus Karlsson wrote:
> > >> From: Magnus Karlsson <magnus.karlsson@intel.com>
> > >>
> > >> Fix a double free at exit of the test suite.
> > >>
> > >> Fixes: a693ff3ed561 ("selftests/xsk: Add support for executing tests on physical device")
> > >> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > >> ---
> > >>   tools/testing/selftests/bpf/xskxceiver.c | 3 ---
> > >>   1 file changed, 3 deletions(-)
> > >>
> > >> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > >> index ef33309bbe49..d1a5f3218c34 100644
> > >> --- a/tools/testing/selftests/bpf/xskxceiver.c
> > >> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > >> @@ -1953,9 +1953,6 @@ int main(int argc, char **argv)
> > >>
> > >>      pkt_stream_delete(tx_pkt_stream_default);
> > >>      pkt_stream_delete(rx_pkt_stream_default);
> > >> -    free(ifobj_rx->umem);
> > >> -    if (!ifobj_tx->shared_umem)
> > shared_umem means ifobj_rx->umem and ifobj_tx->umem are the same?  No special
> > handling is needed and ifobject_delete() will handle it?
>
> You are correct, we will still have a double free in that case. Thanks
> for spotting. Will send a v2.

Sorry, but I have to take my statement back. The v1 is actually
correct. The umem structure is unconditionally allocated in
ifobject_create(). Later when setting up the shared_umem, the
information from one of them is copied over to the other, except for
some information that is changed for the second umem structure. So the
v1 still stands.

> > >> -            free(ifobj_tx->umem);
> > >>      ifobject_delete(ifobj_tx);
> > >>      ifobject_delete(ifobj_rx);
> > >
> > > So basically we free this inside ifobject_delete().
> >
patchwork-bot+netdevbpf@kernel.org Sept. 30, 2022, 10:50 p.m. UTC | #5 
Hello:

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

On Thu, 29 Sep 2022 11:01:33 +0200 you wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Fix a double free at exit of the test suite.
> 
> Fixes: a693ff3ed561 ("selftests/xsk: Add support for executing tests on physical device")
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/xsk: fix double free
    https://git.kernel.org/bpf/bpf-next/c/5f388bba7acb

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index ef33309bbe49..d1a5f3218c34 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -1953,9 +1953,6 @@  int main(int argc, char **argv)
 
 	pkt_stream_delete(tx_pkt_stream_default);
 	pkt_stream_delete(rx_pkt_stream_default);
-	free(ifobj_rx->umem);
-	if (!ifobj_tx->shared_umem)
-		free(ifobj_tx->umem);
 	ifobject_delete(ifobj_tx);
 	ifobject_delete(ifobj_rx);