[bpf-next,V2,2/4] selftests/bpf: xdp_hw_metadata cleanup cause segfault

Commit Message

Jesper Dangaard Brouer Feb. 1, 2023, 5:31 p.m. UTC

Using xdp_hw_metadata I experince Segmentation fault
after seeing "detaching bpf program....".

On my system the segfault happened when accessing bpf_obj->skeleton
in xdp_hw_metadata__destroy(bpf_obj) call. That doesn't make any sense
as this memory have not been freed by program at this point in time.

Prior to calling xdp_hw_metadata__destroy(bpf_obj) the function
close_xsk() is called for each RX-queue xsk.  The real bug lays
in close_xsk() that unmap via munmap() the wrong memory pointer.
The call xsk_umem__delete(xsk->umem) will free xsk->umem, thus
the call to munmap(xsk->umem, UMEM_SIZE) will have unpredictable
behavior. And man page explain subsequent references to these
pages will generate SIGSEGV.

Unmapping xsk->umem_area instead removes the segfault.

Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
Signed-off-by: Jesper Dangaard Brouer <[e-mail]>
---
 tools/testing/selftests/bpf/xdp_hw_metadata.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Martin KaFai Lau Feb. 1, 2023, 5:46 p.m. UTC | #1

On 2/1/23 9:31 AM, Jesper Dangaard Brouer wrote:
> diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> index 3823b1c499cc..438083e34cce 100644
> --- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
> +++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> @@ -121,7 +121,7 @@ static void close_xsk(struct xsk *xsk)
>   		xsk_umem__delete(xsk->umem);
>   	if (xsk->socket)
>   		xsk_socket__delete(xsk->socket);
> -	munmap(xsk->umem, UMEM_SIZE);
> +	munmap(xsk->umem_area, UMEM_SIZE);

Ah. Good catch. This should also explain a similar issue that CI is seeing in 
the prog_tests/xdp_metadata.c.

Jesper Dangaard Brouer Feb. 1, 2023, 5:53 p.m. UTC | #2

On 01/02/2023 18.46, Martin KaFai Lau wrote:
> On 2/1/23 9:31 AM, Jesper Dangaard Brouer wrote:
>> diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c 
>> b/tools/testing/selftests/bpf/xdp_hw_metadata.c
>> index 3823b1c499cc..438083e34cce 100644
>> --- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
>> +++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
>> @@ -121,7 +121,7 @@ static void close_xsk(struct xsk *xsk)
>>           xsk_umem__delete(xsk->umem);
>>       if (xsk->socket)
>>           xsk_socket__delete(xsk->socket);
>> -    munmap(xsk->umem, UMEM_SIZE);
>> +    munmap(xsk->umem_area, UMEM_SIZE);
> 
> Ah. Good catch. This should also explain a similar issue that CI is 
> seeing in the prog_tests/xdp_metadata.c.

Yes, very likely same bug in prog_tests/xdp_metadata.c.

It was super tricky (and time consuming) to find as I was debugging in
GDB and it didn't make sense that checking a value against NULL would
cause a segfault.  Plus, sometimes it worked without issues.

We also need this fix:

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c 
b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
index e033d48288c0..241909d71c7e 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
@@ -121,7 +121,7 @@ static void close_xsk(struct xsk *xsk)
                 xsk_umem__delete(xsk->umem);
         if (xsk->socket)
                 xsk_socket__delete(xsk->socket);
-       munmap(xsk->umem, UMEM_SIZE);
+       munmap(xsk->umem_area, UMEM_SIZE);
  }

Patch

diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
index 3823b1c499cc..438083e34cce 100644
--- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
@@ -121,7 +121,7 @@  static void close_xsk(struct xsk *xsk)
 		xsk_umem__delete(xsk->umem);
 	if (xsk->socket)
 		xsk_socket__delete(xsk->socket);
-	munmap(xsk->umem, UMEM_SIZE);
+	munmap(xsk->umem_area, UMEM_SIZE);
 }
 
 static void refill_rx(struct xsk *xsk, __u64 addr)