[v2] selftests/bpf: Add F_SETFL for fcntl

[PATCH bpf-next v2] selftests/bpf: Add F_SETFL for fcntl

Posted by Geliang Tang 4 weeks ago

From: Geliang Tang <tanggeliang@kylinos.cn>

Incorrect arguments are passed to fcntl() in test_sockmap.c when invoking
it to set file status flags. If O_NONBLOCK is used as 2nd argument and
passed into fcntl, -EINVAL will be returned (See do_fcntl() in fs/fcntl.c).
The correct approach is to use F_SETFL as 2nd argument, and O_NONBLOCK as
3rd one.

In nonblock mode, if EWOULDBLOCK is received, receive again, otherwise some
subtests of test_sockmap will fail.

Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 v2:
 - fix errors:
# 6/ 7  sockmap::txmsg test skb:FAIL
#21/ 7 sockhash::txmsg test skb:FAIL
#36/ 7 sockhash:ktls:txmsg test skb:FAIL
Pass: 42 Fail: 3
---
 tools/testing/selftests/bpf/test_sockmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 024a0faafb3b..bbc3fd57f349 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -603,7 +603,7 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 		struct timeval timeout;
 		fd_set w;
 
-		fcntl(fd, fd_flags);
+		fcntl(fd, F_SETFL, fd_flags);
 		/* Account for pop bytes noting each iteration of apply will
 		 * call msg_pop_data helper so we need to account for this
 		 * by calculating the number of apply iterations. Note user
@@ -671,12 +671,15 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 				flags = 0;
 			}
 
+again:
 			recv = recvmsg(fd, &msg, flags);
 			if (recv < 0) {
 				if (errno != EWOULDBLOCK) {
 					clock_gettime(CLOCK_MONOTONIC, &s->end);
 					perror("recv failed()");
 					goto out_errno;
+				} else {
+					goto again;
 				}
 			}
 
-- 
2.40.1

Re: [PATCH bpf-next v2] selftests/bpf: Add F_SETFL for fcntl

Posted by Geliang Tang 3 weeks, 6 days ago

On Sun, 2024-04-07 at 16:28 +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Incorrect arguments are passed to fcntl() in test_sockmap.c when
> invoking
> it to set file status flags. If O_NONBLOCK is used as 2nd argument
> and
> passed into fcntl, -EINVAL will be returned (See do_fcntl() in
> fs/fcntl.c).
> The correct approach is to use F_SETFL as 2nd argument, and
> O_NONBLOCK as
> 3rd one.
> 
> In nonblock mode, if EWOULDBLOCK is received, receive again,
> otherwise some
> subtests of test_sockmap will fail.
> 
> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  v2:
>  - fix errors:
> # 6/ 7  sockmap::txmsg test skb:FAIL
> #21/ 7 sockhash::txmsg test skb:FAIL
> #36/ 7 sockhash:ktls:txmsg test skb:FAIL
> Pass: 42 Fail: 3

Superseded. v3 is sent, with another fix for "umount cgroup2" error.

Thanks,
-Geliang

> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c
> b/tools/testing/selftests/bpf/test_sockmap.c
> index 024a0faafb3b..bbc3fd57f349 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -603,7 +603,7 @@ static int msg_loop(int fd, int iov_count, int
> iov_length, int cnt,
>  		struct timeval timeout;
>  		fd_set w;
>  
> -		fcntl(fd, fd_flags);
> +		fcntl(fd, F_SETFL, fd_flags);
>  		/* Account for pop bytes noting each iteration of
> apply will
>  		 * call msg_pop_data helper so we need to account
> for this
>  		 * by calculating the number of apply iterations.
> Note user
> @@ -671,12 +671,15 @@ static int msg_loop(int fd, int iov_count, int
> iov_length, int cnt,
>  				flags = 0;
>  			}
>  
> +again:
>  			recv = recvmsg(fd, &msg, flags);
>  			if (recv < 0) {
>  				if (errno != EWOULDBLOCK) {
>  					clock_gettime(CLOCK_MONOTONI
> C, &s->end);
>  					perror("recv failed()");
>  					goto out_errno;
> +				} else {
> +					goto again;
>  				}
>  			}
>