[xdp-hints] [PATCH bpf-next V2 0/4] selftests/bpf: xdp_hw_metadata fixes series

[xdp-hints] [PATCH bpf-next V2 0/4] selftests/bpf: xdp_hw_metadata fixes series

[Jesper Dangaard Brouer]

This series contains a number of small fixes to the BPF selftest
xdp_hw_metadata that I've run into when using it for testing XDP
hardware hints on different NIC hardware.

Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

---

Jesper Dangaard Brouer (4):
      selftests/bpf: xdp_hw_metadata clear metadata when -EOPNOTSUPP
      selftests/bpf: xdp_hw_metadata cleanup cause segfault
      selftests/bpf: xdp_hw_metadata correct status value in error(3)
      selftests/bpf: xdp_hw_metadata use strncpy for ifname


 .../selftests/bpf/progs/xdp_hw_metadata.c     |  6 +++-
 tools/testing/selftests/bpf/xdp_hw_metadata.c | 34 +++++++++----------
 2 files changed, 22 insertions(+), 18 deletions(-)

--

[xdp-hints] [PATCH bpf-next V2 1/4] selftests/bpf: xdp_hw_metadata clear metadata when -EOPNOTSUPP

[Jesper Dangaard Brouer]

The AF_XDP userspace part of xdp_hw_metadata see non-zero as a signal of
the availability of rx_timestamp and rx_hash in data_meta area. The
kernel-side BPF-prog code doesn't initialize these members when kernel
returns an error e.g. -EOPNOTSUPP.  This memory area is not guaranteed to
be zeroed, and can contain garbage/previous values, which will be read
and interpreted by AF_XDP userspace side.

Tested this on different drivers. The experiences are that for most
packets they will have zeroed this data_meta area, but occasionally it
will contain garbage data.

Example of failure tested on ixgbe:
 poll: 1 (0)
 xsk_ring_cons__peek: 1
 0x18ec788: rx_desc[0]->addr=100000000008000 addr=8100 comp_addr=8000
 rx_hash: 3697961069
 rx_timestamp:  9024981991734834796 (sec:9024981991.7348)
 0x18ec788: complete idx=8 addr=8000

Converting to date:
 date -d @9024981991
 2255-12-28T20:26:31 CET

I choose a simple fix in this patch. When kfunc fails or isn't supported
assign zero to the corresponding struct meta value.

It's up to the individual BPF-programmer to do something smarter e.g.
that fits their use-case, like getting a software timestamp and marking
a flag that gives the type of timestamp.

Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 .../testing/selftests/bpf/progs/xdp_hw_metadata.c  |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
index 25b8178735ee..4c55b4d79d3d 100644
--- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
@@ -70,10 +70,14 @@ int rx(struct xdp_md *ctx)
 	}
 
 	if (!bpf_xdp_metadata_rx_timestamp(ctx, &meta->rx_timestamp))
-		bpf_printk("populated rx_timestamp with %u", meta->rx_timestamp);
+		bpf_printk("populated rx_timestamp with %llu", meta->rx_timestamp);
+	else
+		meta->rx_timestamp = 0; /* Used by AF_XDP as not avail signal */
 
 	if (!bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash))
 		bpf_printk("populated rx_hash with %u", meta->rx_hash);
+	else
+		meta->rx_hash = 0; /* Used by AF_XDP as not avail signal */
 
 	return bpf_redirect_map(&xsk, ctx->rx_queue_index, XDP_PASS);
 }

[xdp-hints] [PATCH bpf-next V2 2/4] selftests/bpf: xdp_hw_metadata cleanup cause segfault

[Jesper Dangaard Brouer]

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 <brouer@redhat.com>
---
 tools/testing/selftests/bpf/xdp_hw_metadata.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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)

[xdp-hints] [PATCH bpf-next V2 3/4] selftests/bpf: xdp_hw_metadata correct status value in error(3)

[Jesper Dangaard Brouer]

The glibc error reporting function error():
 void error(int status, int errnum, const char *format, ...);

The status argument should be a positive value between 0-255 as it
is passed over to the exit(3) function as the value as the shell exit
status. The least significant byte of status (i.e., status & 0xFF) is
returned to the shell parent.

Fix this by using 1 instead of -1. As 1 corresponds to C standard
constant EXIT_FAILURE.

Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 tools/testing/selftests/bpf/xdp_hw_metadata.c |   28 +++++++++++++------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
index 438083e34cce..58fde35abad7 100644
--- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
@@ -165,7 +165,7 @@ static void verify_skb_metadata(int fd)
 	hdr.msg_controllen = sizeof(cmsg_buf);
 
 	if (recvmsg(fd, &hdr, 0) < 0)
-		error(-1, errno, "recvmsg");
+		error(1, errno, "recvmsg");
 
 	for (cmsg = CMSG_FIRSTHDR(&hdr); cmsg != NULL;
 	     cmsg = CMSG_NXTHDR(&hdr, cmsg)) {
@@ -275,11 +275,11 @@ static int rxq_num(const char *ifname)
 
 	fd = socket(AF_UNIX, SOCK_DGRAM, 0);
 	if (fd < 0)
-		error(-1, errno, "socket");
+		error(1, errno, "socket");
 
 	ret = ioctl(fd, SIOCETHTOOL, &ifr);
 	if (ret < 0)
-		error(-1, errno, "ioctl(SIOCETHTOOL)");
+		error(1, errno, "ioctl(SIOCETHTOOL)");
 
 	close(fd);
 
@@ -296,11 +296,11 @@ static void hwtstamp_ioctl(int op, const char *ifname, struct hwtstamp_config *c
 
 	fd = socket(AF_UNIX, SOCK_DGRAM, 0);
 	if (fd < 0)
-		error(-1, errno, "socket");
+		error(1, errno, "socket");
 
 	ret = ioctl(fd, op, &ifr);
 	if (ret < 0)
-		error(-1, errno, "ioctl(%d)", op);
+		error(1, errno, "ioctl(%d)", op);
 
 	close(fd);
 }
@@ -360,7 +360,7 @@ static void timestamping_enable(int fd, int val)
 
 	ret = setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING, &val, sizeof(val));
 	if (ret < 0)
-		error(-1, errno, "setsockopt(SO_TIMESTAMPING)");
+		error(1, errno, "setsockopt(SO_TIMESTAMPING)");
 }
 
 int main(int argc, char *argv[])
@@ -386,13 +386,13 @@ int main(int argc, char *argv[])
 
 	rx_xsk = malloc(sizeof(struct xsk) * rxq);
 	if (!rx_xsk)
-		error(-1, ENOMEM, "malloc");
+		error(1, ENOMEM, "malloc");
 
 	for (i = 0; i < rxq; i++) {
 		printf("open_xsk(%s, %p, %d)\n", ifname, &rx_xsk[i], i);
 		ret = open_xsk(ifindex, &rx_xsk[i], i);
 		if (ret)
-			error(-1, -ret, "open_xsk");
+			error(1, -ret, "open_xsk");
 
 		printf("xsk_socket__fd() -> %d\n", xsk_socket__fd(rx_xsk[i].socket));
 	}
@@ -400,7 +400,7 @@ int main(int argc, char *argv[])
 	printf("open bpf program...\n");
 	bpf_obj = xdp_hw_metadata__open();
 	if (libbpf_get_error(bpf_obj))
-		error(-1, libbpf_get_error(bpf_obj), "xdp_hw_metadata__open");
+		error(1, libbpf_get_error(bpf_obj), "xdp_hw_metadata__open");
 
 	prog = bpf_object__find_program_by_name(bpf_obj->obj, "rx");
 	bpf_program__set_ifindex(prog, ifindex);
@@ -409,12 +409,12 @@ int main(int argc, char *argv[])
 	printf("load bpf program...\n");
 	ret = xdp_hw_metadata__load(bpf_obj);
 	if (ret)
-		error(-1, -ret, "xdp_hw_metadata__load");
+		error(1, -ret, "xdp_hw_metadata__load");
 
 	printf("prepare skb endpoint...\n");
 	server_fd = start_server(AF_INET6, SOCK_DGRAM, NULL, 9092, 1000);
 	if (server_fd < 0)
-		error(-1, errno, "start_server");
+		error(1, errno, "start_server");
 	timestamping_enable(server_fd,
 			    SOF_TIMESTAMPING_SOFTWARE |
 			    SOF_TIMESTAMPING_RAW_HARDWARE);
@@ -427,7 +427,7 @@ int main(int argc, char *argv[])
 		printf("map[%d] = %d\n", queue_id, sock_fd);
 		ret = bpf_map_update_elem(bpf_map__fd(bpf_obj->maps.xsk), &queue_id, &sock_fd, 0);
 		if (ret)
-			error(-1, -ret, "bpf_map_update_elem");
+			error(1, -ret, "bpf_map_update_elem");
 	}
 
 	printf("attach bpf program...\n");
@@ -435,12 +435,12 @@ int main(int argc, char *argv[])
 			     bpf_program__fd(bpf_obj->progs.rx),
 			     XDP_FLAGS, NULL);
 	if (ret)
-		error(-1, -ret, "bpf_xdp_attach");
+		error(1, -ret, "bpf_xdp_attach");
 
 	signal(SIGINT, handle_signal);
 	ret = verify_metadata(rx_xsk, rxq, server_fd);
 	close(server_fd);
 	cleanup();
 	if (ret)
-		error(-1, -ret, "verify_metadata");
+		error(1, -ret, "verify_metadata");
 }

[xdp-hints] [PATCH bpf-next V2 4/4] selftests/bpf: xdp_hw_metadata use strncpy for ifname

[Jesper Dangaard Brouer]

The ifname char pointer is taken directly from the command line
as input and the string is copied directly into struct ifreq
via strcpy. This makes it easy to corrupt other members of ifreq
and generally do stack overflows.

Most often the ioctl will fail with:
 ./xdp_hw_metadata: ioctl(SIOCETHTOOL): Bad address

As people will likely copy-paste code for getting NIC queue
channels (rxq_num) and enabling HW timestamping (hwtstamp_ioctl)
lets make this code a bit more secure by using strncpy.

Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 tools/testing/selftests/bpf/xdp_hw_metadata.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
index 58fde35abad7..2a66bd3f2c9f 100644
--- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
@@ -270,7 +270,7 @@ static int rxq_num(const char *ifname)
 	struct ifreq ifr = {
 		.ifr_data = (void *)&ch,
 	};
-	strcpy(ifr.ifr_name, ifname);
+	strncpy(ifr.ifr_name, ifname, IF_NAMESIZE - 1);
 	int fd, ret;
 
 	fd = socket(AF_UNIX, SOCK_DGRAM, 0);
@@ -291,7 +291,7 @@ static void hwtstamp_ioctl(int op, const char *ifname, struct hwtstamp_config *c
 	struct ifreq ifr = {
 		.ifr_data = (void *)cfg,
 	};
-	strcpy(ifr.ifr_name, ifname);
+	strncpy(ifr.ifr_name, ifname, IF_NAMESIZE - 1);
 	int fd, ret;
 
 	fd = socket(AF_UNIX, SOCK_DGRAM, 0);

[xdp-hints] Re: [PATCH bpf-next V2 2/4] selftests/bpf: xdp_hw_metadata cleanup cause segfault

[Martin KaFai Lau]

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.

[xdp-hints] Re: [PATCH bpf-next V2 2/4] selftests/bpf: xdp_hw_metadata cleanup cause segfault

[Jesper Dangaard Brouer]

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

[xdp-hints] Re: [PATCH bpf-next V2 0/4] selftests/bpf: xdp_hw_metadata fixes series

[Stanislav Fomichev]

On Wed, Feb 1, 2023 at 9:31 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> This series contains a number of small fixes to the BPF selftest
> xdp_hw_metadata that I've run into when using it for testing XDP
> hardware hints on different NIC hardware.
>
> Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Thank you!

Acked-by: Stanislav Fomichev <sdf@google.com>

> ---
>
> Jesper Dangaard Brouer (4):
>       selftests/bpf: xdp_hw_metadata clear metadata when -EOPNOTSUPP
>       selftests/bpf: xdp_hw_metadata cleanup cause segfault
>       selftests/bpf: xdp_hw_metadata correct status value in error(3)
>       selftests/bpf: xdp_hw_metadata use strncpy for ifname
>
>
>  .../selftests/bpf/progs/xdp_hw_metadata.c     |  6 +++-
>  tools/testing/selftests/bpf/xdp_hw_metadata.c | 34 +++++++++----------
>  2 files changed, 22 insertions(+), 18 deletions(-)
>
> --
>

[xdp-hints] Re: [PATCH bpf-next V2 0/4] selftests/bpf: xdp_hw_metadata fixes series

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Wed, 01 Feb 2023 18:31:45 +0100 you wrote:
> This series contains a number of small fixes to the BPF selftest
> xdp_hw_metadata that I've run into when using it for testing XDP
> hardware hints on different NIC hardware.
> 
> Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next,V2,1/4] selftests/bpf: xdp_hw_metadata clear metadata when -EOPNOTSUPP
    https://git.kernel.org/bpf/bpf-next/c/3fd9dcd689a5
  - [bpf-next,V2,2/4] selftests/bpf: xdp_hw_metadata cleanup cause segfault
    https://git.kernel.org/bpf/bpf-next/c/a19a62e56478
  - [bpf-next,V2,3/4] selftests/bpf: xdp_hw_metadata correct status value in error(3)
    https://git.kernel.org/bpf/bpf-next/c/7bd4224deecd
  - [bpf-next,V2,4/4] selftests/bpf: xdp_hw_metadata use strncpy for ifname
    https://git.kernel.org/bpf/bpf-next/c/e8a3c8bd6870

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