[bpf-next,v5,1/2] selftests/bpf: Check recv lengths in test_sockmap

Commit Message

Geliang Tang April 23, 2024, 10:26 a.m. UTC

From: Geliang Tang <[e-mail]>

The values of recv and recvp in msg_loop may be negative, so it's necessary
to check if they are positive before using them.

Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
Fixes: 753fb2ee0934 ("bpf: sockmap, add msg_peek tests to test_sockmap")
Signed-off-by: Geliang Tang <[e-mail]>
Acked-by: Yonghong Song <[e-mail]>
---
 tools/testing/selftests/bpf/test_sockmap.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Jakub Sitnicki April 29, 2024, 8:15 p.m. UTC | #1

On Tue, Apr 23, 2024 at 06:26 PM +08, Geliang Tang wrote:
> From: Geliang Tang <[e-mail]>
>
> The values of recv and recvp in msg_loop may be negative, so it's necessary
> to check if they are positive before using them.
>
> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> Fixes: 753fb2ee0934 ("bpf: sockmap, add msg_peek tests to test_sockmap")
> Signed-off-by: Geliang Tang <[e-mail]>
> Acked-by: Yonghong Song <[e-mail]>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index 43612de44fbf..24b55da9d4af 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -680,7 +680,8 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
>  				}
>  			}
>  
> -			s->bytes_recvd += recv;
> +			if (recv > 0)
> +				s->bytes_recvd += recv;
>  
>  			if (opt->check_recved_len && s->bytes_recvd > total_bytes) {
>  				errno = EMSGSIZE;

I'm concerned why are we getting false-positives from select() here?
This is what leads to test failures once socket is non-blocking.

[pid   544] pselect6(29, [28], NULL, NULL, {tv_sec=3, tv_nsec=0}, NULL) = 1 (in [28], left {tv_sec=2, tv_nsec=999997014})
[pid   544] recvmsg(28,  <unfinished ...>
[pid   545] +++ exited with 0 +++
[pid   544] <... recvmsg resumed>{msg_namelen=0}, MSG_NOSIGNAL) = -1 EAGAIN (Resource temporarily unavailable)

Is there an explanation? Or are we ignoring an issue in sockmap code by
"skipping" over EAGAIN errors from recvmsg() in the test?

Didn't have time to dig deeper yet.

Patch

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 43612de44fbf..24b55da9d4af 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -680,7 +680,8 @@  static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 				}
 			}
 
-			s->bytes_recvd += recv;
+			if (recv > 0)
+				s->bytes_recvd += recv;
 
 			if (opt->check_recved_len && s->bytes_recvd > total_bytes) {
 				errno = EMSGSIZE;
@@ -694,12 +695,14 @@  static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 						iov_length * cnt :
 						iov_length * iov_count;
 
-				errno = msg_verify_data(&msg, recv, chunk_sz);
-				if (errno) {
-					perror("data verify msg failed");
-					goto out_errno;
+				if (recv > 0) {
+					errno = msg_verify_data(&msg, recv, chunk_sz);
+					if (errno) {
+						perror("data verify msg failed");
+						goto out_errno;
+					}
 				}
-				if (recvp) {
+				if (recvp > 0) {
 					errno = msg_verify_data(&msg_peek,
 								recvp,
 								chunk_sz);