Virtualization - Mar 2022 - [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 at
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