Messages in this thread Patch in this message | | | Date | Tue, 15 Mar 2022 13:55:04 +0100 | From | Stefano Garzarella <> | Subject | Re: [RFC PATCH v1 3/3] af_vsock: SOCK_SEQPACKET broken buffer test |
| |
On Tue, Mar 15, 2022 at 12:43:13PM +0000, Krasnov Arseniy Vladimirovich wrote: >On 15.03.2022 12:35, Arseniy Krasnov wrote: >> 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@sberdevices.ru> >>>> --- >>>> 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()? > >memcmp() will require special buffer with BUF_PATTERN_2 data as >second argument. I think 'if()' condition is better here, as we >compare each element of buffer with single byte. Anyway, 'memcmp()' >does the same things inside itself.
Ah, I see. Okay, I agree on for()/if(), maybe we can also print more info (index, expected value, current value).
> >> >> 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. > >So i've checked that assumption for SEQPACKET + AF_UNIX: when user passes broken buffer to >the kernel(for example with unmapped page in the mid), rest of message will be dropped. Next >read will never get tail of the dropped message.
Thanks for checking, so it seems the same behaviour. Let's go ahead with this test :-)
Stefano
| |