Re: [PATCH bpf-next v3 1/2] selftests/bpf: Add F_SETFL for fcntl in test_sockmap

[Martin KaFai Lau <martin.lau@linux.dev>]

On 4/7/24 6:36 PM, 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, continue receiving, otherwise
> some subtests of test_sockmap will fail.
> 
> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>   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..4f32a5eb3864 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);

Should it just error out here if fcntl did fail (unlikely?) ...

>   		/* 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
> @@ -678,6 +678,9 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
>   					perror("recv failed()");
>   					goto out_errno;
>   				}
> +				fd_flags = fcntl(fd, F_GETFL);
> +				if (fd_flags & O_NONBLOCK)

... then no need to test fd_flags here?

pw-bot: cr

> +					continue;
>   			}
>   
>   			s->bytes_recvd += recv;