Re: [RFC PATCH v1 3/3] af_vsock: SOCK_SEQPACKET broken buffer test

[Krasnov Arseniy Vladimirovich <AVKrasnov@sberdevices.ru>]

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()?

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
>