[bpf-next,v2,6/6] selftests/bpf: fix file descriptor assertion in open_tuntap helper

Commit Message

Marcus Wichelmann Feb. 17, 2025, 5:23 p.m. UTC

The open_tuntap helper function uses open() to get a file descriptor for
/dev/net/tun.

The open(2) manpage writes this about its return value:

  On success, open(), openat(), and creat() return the new file
  descriptor (a nonnegative integer).  On error, -1 is returned and
  errno is set to indicate the error.

This means that the fd > 0 assertion in the open_tuntap helper is
incorrect and should rather check for fd >= 0.

When running the BPF selftests locally, this incorrect assertion was not
an issue, but the BPF kernel-patches CI failed because of this:

  open_tuntap:FAIL:open(/dev/net/tun) unexpected open(/dev/net/tun):
  actual 0 <= expected 0

Signed-off-by: Marcus Wichelmann <[e-mail]>
---
 tools/testing/selftests/bpf/network_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Willem de Bruijn Feb. 18, 2025, 1:56 a.m. UTC | #1

Marcus Wichelmann wrote:
> The open_tuntap helper function uses open() to get a file descriptor for
> /dev/net/tun.
> 
> The open(2) manpage writes this about its return value:
> 
>   On success, open(), openat(), and creat() return the new file
>   descriptor (a nonnegative integer).  On error, -1 is returned and
>   errno is set to indicate the error.
> 
> This means that the fd > 0 assertion in the open_tuntap helper is
> incorrect and should rather check for fd >= 0.
> 
> When running the BPF selftests locally, this incorrect assertion was not
> an issue, but the BPF kernel-patches CI failed because of this:
> 
>   open_tuntap:FAIL:open(/dev/net/tun) unexpected open(/dev/net/tun):
>   actual 0 <= expected 0

Wow. What kind of environment is this that 0 is not assigned stdin.
 
> Signed-off-by: Marcus Wichelmann <[e-mail]>

The code makes sense.

I suppose that if this condition can hit, then it can also affect
existing lwt_* tests and thus should be a fix to commit 43a7c3ef8a15
("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT"), sent
separately to bpf (not bpf-next)?

Since it's a test and no failure was reported so far, maybe fine
to just merge as part of this bpf-next series, not my call.

> ---
>  tools/testing/selftests/bpf/network_helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index e1cfa1b37754..9b59bfd5d912 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -571,7 +571,7 @@ int open_tuntap(const char *dev_name, bool need_mac)
>  	struct ifreq ifr;
>  	int fd = open("/dev/net/tun", O_RDWR);
>  
> -	if (!ASSERT_GT(fd, 0, "open(/dev/net/tun)"))
> +	if (!ASSERT_GE(fd, 0, "open(/dev/net/tun)"))
>  		return -1;
>  
>  	ifr.ifr_flags = IFF_NO_PI | (need_mac ? IFF_TAP : IFF_TUN);
> -- 
> 2.43.0
>

Marcus Wichelmann Feb. 19, 2025, 2:58 p.m. UTC | #2

Am 18.02.25 um 02:56 schrieb Willem de Bruijn:
> Marcus Wichelmann wrote:
>> [...]
>> When running the BPF selftests locally, this incorrect assertion was not
>> an issue, but the BPF kernel-patches CI failed because of this:
>>
>>    open_tuntap:FAIL:open(/dev/net/tun) unexpected open(/dev/net/tun):
>>    actual 0 <= expected 0
> 
> Wow. What kind of environment is this that 0 is not assigned stdin.
>   
>> Signed-off-by: Marcus Wichelmann <[e-mail]>
> 
> The code makes sense.
> 
> I suppose that if this condition can hit, then it can also affect
> existing lwt_* tests and thus should be a fix to commit 43a7c3ef8a15
> ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT"), sent
> separately to bpf (not bpf-next)?
> 
> Since it's a test and no failure was reported so far, maybe fine
> to just merge as part of this bpf-next series, not my call.
I'm not sure why this only became an issue after I added the
xdp_context_tuntap test and never before. This may have to do with
the order of test execution.

If nobody speaks up, I'll leave it in this patch series for now.

Marcus

Patch

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index e1cfa1b37754..9b59bfd5d912 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -571,7 +571,7 @@  int open_tuntap(const char *dev_name, bool need_mac)
 	struct ifreq ifr;
 	int fd = open("/dev/net/tun", O_RDWR);
 
-	if (!ASSERT_GT(fd, 0, "open(/dev/net/tun)"))
+	if (!ASSERT_GE(fd, 0, "open(/dev/net/tun)"))
 		return -1;
 
 	ifr.ifr_flags = IFF_NO_PI | (need_mac ? IFF_TAP : IFF_TUN);