Messages in this thread Patch in this message | | | From | Arseniy Krasnov <> | Subject | Re: [RFC PATCH v1 1/3] test/vsock: rework message bound test | Date | Mon, 21 Nov 2022 16:49:23 +0000 |
| |
On 21.11.2022 17:46, Stefano Garzarella wrote: > On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote: >> This updates message bound test making it more complex. Instead of >> sending 1 bytes messages with one MSG_EOR bit, it sends messages of >> random length(one half of messages are smaller than page size, second >> half are bigger) with random number of MSG_EOR bits set. Receiver >> also don't know total number of messages. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> tools/testing/vsock/control.c | 34 +++++++++ >> tools/testing/vsock/control.h | 2 + >> tools/testing/vsock/util.c | 13 ++++ >> tools/testing/vsock/util.h | 1 + >> tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++---- >> 5 files changed, 152 insertions(+), 13 deletions(-) >> >> diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c >> index 4874872fc5a3..bed1649bdf3d 100644 >> --- a/tools/testing/vsock/control.c >> +++ b/tools/testing/vsock/control.c >> @@ -141,6 +141,40 @@ void control_writeln(const char *str) >> timeout_end(); >> } >> >> +void control_writeulong(unsigned long value) >> +{ >> + char str[32]; >> + >> + if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) { >> + perror("snprintf"); >> + exit(EXIT_FAILURE); >> + } >> + >> + control_writeln(str); >> +} >> + >> +unsigned long control_readulong(bool *ok) >> +{ >> + unsigned long value; >> + char *str; >> + >> + if (ok) >> + *ok = false; >> + >> + str = control_readln(); >> + >> + if (str == NULL) > > checkpatch suggests to use !str > >> + return 0; > > Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok` > parameter, since I'm not sure we can recover from this error. > >> + >> + value = strtoul(str, NULL, 10); >> + free(str); >> + >> + if (ok) >> + *ok = true; >> + >> + return value; >> +} >> + >> /* Return the next line from the control socket (without the trailing newline). >> * >> * The program terminates if a timeout occurs. >> diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h >> index 51814b4f9ac1..cdd922dfea68 100644 >> --- a/tools/testing/vsock/control.h >> +++ b/tools/testing/vsock/control.h >> @@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port, >> void control_cleanup(void); >> void control_writeln(const char *str); >> char *control_readln(void); >> +unsigned long control_readulong(bool *ok); >> void control_expectln(const char *str); >> bool control_cmpln(char *line, const char *str, bool fail); >> +void control_writeulong(unsigned long value); >> >> #endif /* CONTROL_H */ >> diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c >> index 2acbb7703c6a..351903836774 100644 >> --- a/tools/testing/vsock/util.c >> +++ b/tools/testing/vsock/util.c >> @@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len, >> >> test_cases[test_id].skip = true; >> } >> + >> +unsigned long djb2(const void *data, size_t len) > > I would add hash_ as a prefix (or suffix). > >> +{ >> + unsigned long hash = 5381; >> + int i = 0; >> + >> + while (i < len) { >> + hash = ((hash << 5) + hash) + ((unsigned char *)data)[i]; >> + i++; >> + } >> + >> + return hash; >> +} >> diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h >> index a3375ad2fb7f..988cc69a4642 100644 >> --- a/tools/testing/vsock/util.h >> +++ b/tools/testing/vsock/util.h >> @@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases, >> void list_tests(const struct test_case *test_cases); >> void skip_test(struct test_case *test_cases, size_t test_cases_len, >> const char *test_id_str); >> +unsigned long djb2(const void *data, size_t len); >> #endif /* UTIL_H */ >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >> index bb6d691cb30d..107c11165887 100644 >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts) >> close(fd); >> } >> >> -#define MESSAGES_CNT 7 >> -#define MSG_EOR_IDX (MESSAGES_CNT / 2) >> +#define SOCK_BUF_SIZE (2 * 1024 * 1024) >> +#define MAX_MSG_SIZE (32 * 1024) >> + >> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts) >> { >> + unsigned long curr_hash; >> + int page_size; >> + int msg_count; >> int fd; >> >> fd = vsock_seqpacket_connect(opts->peer_cid, 1234); >> @@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts) >> exit(EXIT_FAILURE); >> } >> >> - /* Send several messages, one with MSG_EOR flag */ >> - for (int i = 0; i < MESSAGES_CNT; i++) >> - send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0); >> + /* Wait, until receiver sets buffer size. */ >> + control_expectln("SRVREADY"); >> + >> + curr_hash = 0; >> + page_size = getpagesize(); >> + msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE; >> + >> + for (int i = 0; i < msg_count; i++) { >> + ssize_t send_size; >> + size_t buf_size; >> + int flags; >> + void *buf; >> + >> + /* Use "small" buffers and "big" buffers. */ >> + if (i & 1) >> + buf_size = page_size + >> + (rand() % (MAX_MSG_SIZE - page_size)); >> + else >> + buf_size = 1 + (rand() % page_size); >> + >> + buf = malloc(buf_size); >> + >> + if (!buf) { >> + perror("malloc"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Set at least one MSG_EOR + some random. */ >> + if (i == (msg_count / 2) || (rand() & 1)) { >> + flags = MSG_EOR; >> + curr_hash++; >> + } else { >> + flags = 0; >> + } >> + >> + send_size = send(fd, buf, buf_size, flags); >> + >> + if (send_size < 0) { >> + perror("send"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (send_size != buf_size) { >> + fprintf(stderr, "Invalid send size\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + curr_hash += send_size; >> + curr_hash = djb2(&curr_hash, sizeof(curr_hash)); >> + } >> >> control_writeln("SENDDONE"); >> + control_writeulong(curr_hash); > > Why do we need to hash the size? > > Maybe we can send it without making the hash, anyway even if it wraps, > it should wrap the same way in both the server and the client. > (Or maybe we can use uin32_t or uint64_t to make sure both were > using 4 or 8 bytes) Hello, thanks for review. I think if we will use sum of message size(IIUC), in most paranoic case it won't guarantee message bounds control: single 4 bytes message could be read as 4 x 1 byte message(IIUC of course). Idea of hashing is simple: every iteration we do current_hash = hash(previous_hash + size of current message); I think this is more reliable and protects from case described above.
All other comments - ack. > >> close(fd); >> } >> >> static void test_seqpacket_msg_bounds_server(const struct test_opts *opts) >> { >> + unsigned long sock_buf_size; >> + unsigned long remote_hash; >> + unsigned long curr_hash; >> int fd; >> - char buf[16]; >> + char buf[MAX_MSG_SIZE]; >> struct msghdr msg = {0}; >> struct iovec iov = {0}; >> >> @@ -317,25 +372,58 @@ static void test_seqpacket_msg_bounds_server(const struct test_opts *opts) >> exit(EXIT_FAILURE); >> } >> >> + sock_buf_size = SOCK_BUF_SIZE; >> + >> + if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE, >> + &sock_buf_size, sizeof(sock_buf_size))) { >> + perror("getsockopt"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE, >> + &sock_buf_size, sizeof(sock_buf_size))) { >> + perror("getsockopt"); >> + exit(EXIT_FAILURE); >> + } >> + >> + /* Ready to receive data. */ >> + control_writeln("SRVREADY"); >> + /* Wait, until peer sends whole data. */ >> control_expectln("SENDDONE"); >> iov.iov_base = buf; >> iov.iov_len = sizeof(buf); >> msg.msg_iov = &iov; >> msg.msg_iovlen = 1; >> >> - for (int i = 0; i < MESSAGES_CNT; i++) { >> - if (recvmsg(fd, &msg, 0) != 1) { >> - perror("message bound violated"); >> - exit(EXIT_FAILURE); >> - } >> + curr_hash = 0; >> >> - if ((i == MSG_EOR_IDX) ^ !!(msg.msg_flags & MSG_EOR)) { >> - perror("MSG_EOR"); >> + while (1) { >> + ssize_t recv_size; >> + >> + recv_size = recvmsg(fd, &msg, 0); >> + >> + if (!recv_size) >> + break; >> + >> + if (recv_size < 0) { >> + perror("recvmsg"); >> exit(EXIT_FAILURE); >> } >> + >> + if (msg.msg_flags & MSG_EOR) >> + curr_hash++; >> + >> + curr_hash += recv_size; >> + curr_hash = djb2(&curr_hash, sizeof(curr_hash)); >> } >> >> close(fd); >> + remote_hash = control_readulong(NULL); >> + >> + if (curr_hash != remote_hash) { >> + fprintf(stderr, "Message bounds broken\n"); >> + exit(EXIT_FAILURE); >> + } >> } >> >> #define MESSAGE_TRUNC_SZ 32 >> @@ -837,6 +925,7 @@ int main(int argc, char **argv) >> .peer_cid = VMADDR_CID_ANY, >> }; >> >> + srand(time(NULL)); >> init_signals(); >> >> for (;;) { >> -- >> 2.25.1 >
| |