On 15.03.2022 11:36, Stefano Garzarella wrote: > On Fri, Mar 11, 2022 at 10:58:32AM +0000, Krasnov Arseniy Vladimirovich wrote: >> Add test where sender sends two message, each with own >> data pattern. Reader tries to read first to broken buffer: >> it has three pages size, but middle page is unmapped. Then, >> reader tries to read second message to valid buffer. Test >> checks, that uncopied part of first message was dropped >> and thus not copied as part of second message. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@xxxxxxxxxxxxxx> >> --- >> tools/testing/vsock/vsock_test.c | 121 +++++++++++++++++++++++++++++++ >> 1 file changed, 121 insertions(+) >> >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >> index aa2de27d0f77..686af712b4ad 100644 >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -16,6 +16,7 @@ >> #include <linux/kernel.h> >> #include <sys/types.h> >> #include <sys/socket.h> >> +#include <sys/mman.h> >> >> #include "timeout.h" >> #include "control.h" >> @@ -435,6 +436,121 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts) >> close(fd); >> } >> >> +#define BUF_PATTERN_1 'a' >> +#define BUF_PATTERN_2 'b' >> + >> +static void test_seqpacket_invalid_rec_buffer_client(const struct test_opts *opts) >> +{ >> + int fd; >> + unsigned char *buf1; >> + unsigned char *buf2; >> + int buf_size = getpagesize() * 3; >> + >> + fd = vsock_seqpacket_connect(opts->peer_cid, 1234); >> + if (fd < 0) { >> + perror("connect"); >> + exit(EXIT_FAILURE); >> + } >> + >> + buf1 = malloc(buf_size); >> + if (buf1 == NULL) { >> + perror("'malloc()' for 'buf1'"); >> + exit(EXIT_FAILURE); >> + } >> + >> + buf2 = malloc(buf_size); >> + if (buf2 == NULL) { >> + perror("'malloc()' for 'buf2'"); >> + exit(EXIT_FAILURE); >> + } >> + >> + memset(buf1, BUF_PATTERN_1, buf_size); >> + memset(buf2, BUF_PATTERN_2, buf_size); >> + >> + if (send(fd, buf1, buf_size, 0) != buf_size) { >> + perror("send failed"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (send(fd, buf2, buf_size, 0) != buf_size) { >> + perror("send failed"); >> + exit(EXIT_FAILURE); >> + } >> + >> + close(fd); >> +} >> + >> +static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opts) >> +{ >> + int fd; >> + unsigned char *broken_buf; >> + unsigned char *valid_buf; >> + int page_size = getpagesize(); >> + int buf_size = page_size * 3; >> + ssize_t res; >> + int prot = PROT_READ | PROT_WRITE; >> + int flags = MAP_PRIVATE | MAP_ANONYMOUS; >> + int i; >> + >> + fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL); >> + if (fd < 0) { >> + perror("accept"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Setup first buffer. */ >> + broken_buf = mmap(NULL, buf_size, prot, flags, -1, 0); >> + if (broken_buf == MAP_FAILED) { >> + perror("mmap for 'broken_buf'"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Unmap "hole" in buffer. */ >> + if (munmap(broken_buf + page_size, page_size)) { >> + perror("'broken_buf' setup"); >> + exit(EXIT_FAILURE); >> + } >> + >> + valid_buf = mmap(NULL, buf_size, prot, flags, -1, 0); >> + if (valid_buf == MAP_FAILED) { >> + perror("mmap for 'valid_buf'"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Try to fill buffer with unmapped middle. */ >> + res = read(fd, broken_buf, buf_size); >> + if (res != -1) { >> + perror("invalid read result of 'broken_buf'"); > > if `res` is valid, errno is not set, better to use fprintf(stderr, ...) printing the expected and received result. > Take a look at test_stream_connection_reset() Ack, fix it in v2 > >> + exit(EXIT_FAILURE); >> + } >> + >> + if (errno != ENOMEM) { >> + perror("invalid errno of 'broken_buf'"); > > Instead of "invalid", I would say "unexpected". Ack > >> + exit(EXIT_FAILURE); >> + } > > >> + >> + /* Try to fill valid buffer. */ >> + res = read(fd, valid_buf, buf_size); >> + if (res != buf_size) { >> + perror("invalid read result of 'valid_buf'"); > > I would split in 2 checks: > - (res < 0) then use perror() > - (res != buf_size) then use fprintf(stderr, ...) printing the expected and received result. Ack > >> + exit(EXIT_FAILURE); >> + } >> + >> + for (i = 0; i < buf_size; i++) { >> + if (valid_buf[i] != BUF_PATTERN_2) { >> + perror("invalid pattern for valid buf"); > > errno is not set here, better to use fprintf(stderr, ...) Ack > >> + exit(EXIT_FAILURE); >> + } >> + } > > What about replace this for with a memcmp()? Ack > >> + >> + >> + /* Unmap buffers. */ >> + munmap(broken_buf, page_size); >> + munmap(broken_buf + page_size * 2, page_size); >> + munmap(valid_buf, buf_size); >> + close(fd); >> +} >> + >> static struct test_case test_cases[] = { >> { >> .name = "SOCK_STREAM connection reset", >> @@ -480,6 +596,11 @@ static struct test_case test_cases[] = { >> .run_client = test_seqpacket_timeout_client, >> .run_server = test_seqpacket_timeout_server, >> }, >> + { >> + .name = "SOCK_SEQPACKET invalid receive buffer", >> + .run_client = test_seqpacket_invalid_rec_buffer_client, >> + .run_server = test_seqpacket_invalid_rec_buffer_server, >> + }, > > > Is this the right behavior? If read() fails because the buffer is invalid, do we throw out the whole packet? > > I was expecting the packet not to be consumed, have you tried AF_UNIX, does it have the same behavior? I've just checked AF_UNIX implementation of SEQPACKET receive in net/unix/af_unix.c. So, if 'skb_copy_datagram_msg()' fails, it calls 'skb_free_datagram()'. I think this means that whole sk buff will be dropped, but anyway, i'll check this behaviour in practice. See '__unix_dgram_recvmsg()' in net/unix/af_unix.c. > > Thanks, > Stefano >