| Message ID | 20241206-test-vsock-leaks-v1-2-c31e8c875797@rbox.co (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock/test: Tests for memory leaks | expand |
On Fri, Dec 06, 2024 at 07:34:52PM +0100, Michal Luczaj wrote: >Attempt to enqueue a child after the queue was flushed, but before >SOCK_DONE flag has been set. > >Fixed by commit d7b0ff5a8667 ("virtio/vsock: Fix accept_queue memory >leak"). > >Signed-off-by: Michal Luczaj <mhal@rbox.co> >--- > tools/testing/vsock/vsock_test.c | 44 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > >diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >index 38fd8d96eb83ef1bd45728cfaac6adb3c1e07cfe..48b6d970bcfa95f957facb7ba2e729a32d256b4a 100644 >--- a/tools/testing/vsock/vsock_test.c >+++ b/tools/testing/vsock/vsock_test.c >@@ -1474,6 +1474,45 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts) > test_stream_credit_update_test(opts, false); > } > >+#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ >+ >+static void test_stream_leak_acceptq_client(const struct test_opts *opts) >+{ >+ struct sockaddr_vm addr = { >+ .svm_family = AF_VSOCK, >+ .svm_port = opts->peer_port, >+ .svm_cid = opts->peer_cid >+ }; >+ time_t tout; >+ int fd; >+ >+ tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; >+ do { >+ control_writeulong(1); Can we use control_writeln() and control_expectln()? >+ >+ fd = socket(AF_VSOCK, SOCK_STREAM, 0); >+ if (fd < 0) { >+ perror("socket"); >+ exit(EXIT_FAILURE); >+ } >+ Do we need another control messages (server -> client) here to be sure the server is listening? >+ connect(fd, (struct sockaddr *)&addr, sizeof(addr)); What about using `vsock_stream_connect` so you can remove a lot of code from this function (e.g. sockaddr_vm, socket(), etc.) We only need to add `control_expectln("LISTENING")` in the server which should also fix my previous comment. >+ close(fd); >+ } while (current_nsec() < tout); >+ >+ control_writeulong(0); >+} >+ >+static void test_stream_leak_acceptq_server(const struct test_opts *opts) >+{ >+ int fd; >+ >+ while (control_readulong()) { Ah I see, the loop is easier by sending a number. I would just add some comments when we send 1 and 0 to explain it. Thanks, Stefano >+ fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); >+ close(fd); >+ } >+} >+ > static struct test_case test_cases[] = { > { > .name = "SOCK_STREAM connection reset", >@@ -1604,6 +1643,11 @@ static struct test_case test_cases[] = { > .run_client = test_seqpacket_unsent_bytes_client, > .run_server = test_seqpacket_unsent_bytes_server, > }, >+ { >+ .name = "SOCK_STREAM leak accept queue", >+ .run_client = test_stream_leak_acceptq_client, >+ .run_server = test_stream_leak_acceptq_server, >+ }, > {}, > }; > > >-- >2.47.1 >
On 12/10/24 17:18, Stefano Garzarella wrote: > On Fri, Dec 06, 2024 at 07:34:52PM +0100, Michal Luczaj wrote: >> [...] >> +#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ >> + >> +static void test_stream_leak_acceptq_client(const struct test_opts *opts) >> +{ >> + struct sockaddr_vm addr = { >> + .svm_family = AF_VSOCK, >> + .svm_port = opts->peer_port, >> + .svm_cid = opts->peer_cid >> + }; >> + time_t tout; >> + int fd; >> + >> + tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; >> + do { >> + control_writeulong(1); > > Can we use control_writeln() and control_expectln()? Please see below. >> + >> + fd = socket(AF_VSOCK, SOCK_STREAM, 0); >> + if (fd < 0) { >> + perror("socket"); >> + exit(EXIT_FAILURE); >> + } >> + > > Do we need another control messages (server -> client) here to be sure > the server is listening? Ahh, I get your point. >> + connect(fd, (struct sockaddr *)&addr, sizeof(addr)); > > What about using `vsock_stream_connect` so you can remove a lot of > code from this function (e.g. sockaddr_vm, socket(), etc.) > > We only need to add `control_expectln("LISTENING")` in the server which > should also fix my previous comment. Sure, I followed your suggestion with tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; do { control_writeulong(RACE_CONTINUE); fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); if (fd >= 0) close(fd); } while (current_nsec() < tout); control_writeulong(RACE_DONE); vs. while (control_readulong() == RACE_CONTINUE) { fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); control_writeln("LISTENING"); close(fd); } and it works just fine. >> +static void test_stream_leak_acceptq_server(const struct test_opts *opts) >> +{ >> + int fd; >> + >> + while (control_readulong()) { > > Ah I see, the loop is easier by sending a number. > I would just add some comments when we send 1 and 0 to explain it. How about the #defines above? Thanks! Michal
On Thu, Dec 12, 2024 at 11:12:19PM +0100, Michal Luczaj wrote: >On 12/10/24 17:18, Stefano Garzarella wrote: >> On Fri, Dec 06, 2024 at 07:34:52PM +0100, Michal Luczaj wrote: >>> [...] >>> +#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ >>> + >>> +static void test_stream_leak_acceptq_client(const struct test_opts *opts) >>> +{ >>> + struct sockaddr_vm addr = { >>> + .svm_family = AF_VSOCK, >>> + .svm_port = opts->peer_port, >>> + .svm_cid = opts->peer_cid >>> + }; >>> + time_t tout; >>> + int fd; >>> + >>> + tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; >>> + do { >>> + control_writeulong(1); >> >> Can we use control_writeln() and control_expectln()? > >Please see below. > >>> + >>> + fd = socket(AF_VSOCK, SOCK_STREAM, 0); >>> + if (fd < 0) { >>> + perror("socket"); >>> + exit(EXIT_FAILURE); >>> + } >>> + >> >> Do we need another control messages (server -> client) here to be sure >> the server is listening? > >Ahh, I get your point. > >>> + connect(fd, (struct sockaddr *)&addr, sizeof(addr)); >> >> What about using `vsock_stream_connect` so you can remove a lot of >> code from this function (e.g. sockaddr_vm, socket(), etc.) >> >> We only need to add `control_expectln("LISTENING")` in the server which >> should also fix my previous comment. > >Sure, I followed your suggestion with > > tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; > do { > control_writeulong(RACE_CONTINUE); > fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); > if (fd >= 0) > close(fd); I'd do if (fd < 0) { perror("connect"); exit(EXIT_FAILURE); } close(fd); apart of that LGTM! > } while (current_nsec() < tout); > control_writeulong(RACE_DONE); > >vs. > > while (control_readulong() == RACE_CONTINUE) { > fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); > control_writeln("LISTENING"); > close(fd); > } > >and it works just fine. > >>> +static void test_stream_leak_acceptq_server(const struct test_opts *opts) >>> +{ >>> + int fd; >>> + >>> + while (control_readulong()) { >> >> Ah I see, the loop is easier by sending a number. >> I would just add some comments when we send 1 and 0 to explain it. > >How about the #defines above? yeah, I like it! Thanks, Stefano > >Thanks! >Michal >
On 12/13/24 12:55, Stefano Garzarella wrote: > On Thu, Dec 12, 2024 at 11:12:19PM +0100, Michal Luczaj wrote: >> On 12/10/24 17:18, Stefano Garzarella wrote: >>> [...] >>> What about using `vsock_stream_connect` so you can remove a lot of >>> code from this function (e.g. sockaddr_vm, socket(), etc.) >>> >>> We only need to add `control_expectln("LISTENING")` in the server which >>> should also fix my previous comment. >> >> Sure, I followed your suggestion with >> >> tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; >> do { >> control_writeulong(RACE_CONTINUE); >> fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); >> if (fd >= 0) >> close(fd); > > I'd do > if (fd < 0) { > perror("connect"); > exit(EXIT_FAILURE); > } > close(fd); I think that won't fly. We're racing here with close(listener), so a failing connect() is expected. Michal
On Fri, Dec 13, 2024 at 03:27:53PM +0100, Michal Luczaj wrote: >On 12/13/24 12:55, Stefano Garzarella wrote: >> On Thu, Dec 12, 2024 at 11:12:19PM +0100, Michal Luczaj wrote: >>> On 12/10/24 17:18, Stefano Garzarella wrote: >>>> [...] >>>> What about using `vsock_stream_connect` so you can remove a lot of >>>> code from this function (e.g. sockaddr_vm, socket(), etc.) >>>> >>>> We only need to add `control_expectln("LISTENING")` in the server which >>>> should also fix my previous comment. >>> >>> Sure, I followed your suggestion with >>> >>> tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; >>> do { >>> control_writeulong(RACE_CONTINUE); >>> fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); >>> if (fd >= 0) >>> close(fd); >> >> I'd do >> if (fd < 0) { >> perror("connect"); >> exit(EXIT_FAILURE); >> } >> close(fd); > >I think that won't fly. We're racing here with close(listener), so a >failing connect() is expected. Oh right! If it doesn't matter, fine with your version, but please add a comment there, otherwise we need another barrier with control messages. Or another option is to reuse the control message we already have to close the previous listening socket, so something like this: static void test_stream_leak_acceptq_server(const struct test_opts *opts) { int fd = -1; while (control_readulong() == RACE_CONTINUE) { /* Close the previous listening socket after receiving * a control message, so we are sure the other side * already connected. */ if (fd >= 0) close(fd); fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); control_writeln("LISTENING"); } if (fd >= 0) close(fd); } Thanks, Stefano
On 12/13/24 15:47, Stefano Garzarella wrote: > On Fri, Dec 13, 2024 at 03:27:53PM +0100, Michal Luczaj wrote: >> On 12/13/24 12:55, Stefano Garzarella wrote: >>> On Thu, Dec 12, 2024 at 11:12:19PM +0100, Michal Luczaj wrote: >>>> On 12/10/24 17:18, Stefano Garzarella wrote: >>>>> [...] >>>>> What about using `vsock_stream_connect` so you can remove a lot of >>>>> code from this function (e.g. sockaddr_vm, socket(), etc.) >>>>> >>>>> We only need to add `control_expectln("LISTENING")` in the server which >>>>> should also fix my previous comment. >>>> >>>> Sure, I followed your suggestion with >>>> >>>> tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; >>>> do { >>>> control_writeulong(RACE_CONTINUE); >>>> fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); >>>> if (fd >= 0) >>>> close(fd); >>> >>> I'd do >>> if (fd < 0) { >>> perror("connect"); >>> exit(EXIT_FAILURE); >>> } >>> close(fd); >> >> I think that won't fly. We're racing here with close(listener), so a >> failing connect() is expected. > > Oh right! > If it doesn't matter, fine with your version, but please add a comment > there, otherwise we need another barrier with control messages. > > Or another option is to reuse the control message we already have to > close the previous listening socket, so something like this: > > static void test_stream_leak_acceptq_server(const struct test_opts *opts) > { > int fd = -1; > > while (control_readulong() == RACE_CONTINUE) { > /* Close the previous listening socket after receiving > * a control message, so we are sure the other side > * already connected. > */ > if (fd >= 0) > close(fd); > fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); > control_writeln("LISTENING"); > } > > if (fd >= 0) > close(fd); > } I'm afraid this won't work either. Just to be clear: the aim is to attempt connect() in parallel with close(listener). It's not about establishing connection. In fact, if the connection has been established, it means we failed reaching the right condition. In other words, what I propose is: client loop server loop ----------- ----------- write(CONTINUE) expect(CONTINUE) listen() write(LISTENING) expect(LISTENING) connect() close() // bang, maybe And, if I understand correctly, you are suggesting: client loop server loop ----------- ----------- write(CONTINUE) expect(CONTINUE) listen() write(LISTENING) expect(LISTENING) connect() // no close() to race // 2nd iteration write(CONTINUE) // 2nd iteration expect(CONTINUE) close() // no connect() to race listen() write(LISTENING) expect(LISTENING) connect() // no close() to race Hope it makes sense?
On Fri, Dec 13, 2024 at 5:15 PM Michal Luczaj <mhal@rbox.co> wrote: > > On 12/13/24 15:47, Stefano Garzarella wrote: > > On Fri, Dec 13, 2024 at 03:27:53PM +0100, Michal Luczaj wrote: > >> On 12/13/24 12:55, Stefano Garzarella wrote: > >>> On Thu, Dec 12, 2024 at 11:12:19PM +0100, Michal Luczaj wrote: > >>>> On 12/10/24 17:18, Stefano Garzarella wrote: > >>>>> [...] > >>>>> What about using `vsock_stream_connect` so you can remove a lot of > >>>>> code from this function (e.g. sockaddr_vm, socket(), etc.) > >>>>> > >>>>> We only need to add `control_expectln("LISTENING")` in the server which > >>>>> should also fix my previous comment. > >>>> > >>>> Sure, I followed your suggestion with > >>>> > >>>> tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; > >>>> do { > >>>> control_writeulong(RACE_CONTINUE); > >>>> fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); > >>>> if (fd >= 0) > >>>> close(fd); > >>> > >>> I'd do > >>> if (fd < 0) { > >>> perror("connect"); > >>> exit(EXIT_FAILURE); > >>> } > >>> close(fd); > >> > >> I think that won't fly. We're racing here with close(listener), so a > >> failing connect() is expected. > > > > Oh right! > > If it doesn't matter, fine with your version, but please add a comment > > there, otherwise we need another barrier with control messages. > > > > Or another option is to reuse the control message we already have to > > close the previous listening socket, so something like this: > > > > static void test_stream_leak_acceptq_server(const struct test_opts *opts) > > { > > int fd = -1; > > > > while (control_readulong() == RACE_CONTINUE) { > > /* Close the previous listening socket after receiving > > * a control message, so we are sure the other side > > * already connected. > > */ > > if (fd >= 0) > > close(fd); > > fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); > > control_writeln("LISTENING"); > > } > > > > if (fd >= 0) > > close(fd); > > } > > I'm afraid this won't work either. Just to be clear: the aim is to attempt > connect() in parallel with close(listener). It's not about establishing > connection. In fact, if the connection has been established, it means we > failed reaching the right condition. > > In other words, what I propose is: > > client loop server loop > ----------- ----------- > write(CONTINUE) > expect(CONTINUE) > listen() > write(LISTENING) > expect(LISTENING) > connect() close() // bang, maybe > > And, if I understand correctly, you are suggesting: > > client loop server loop > ----------- ----------- > write(CONTINUE) > expect(CONTINUE) > listen() > write(LISTENING) > expect(LISTENING) > connect() // no close() to race > // 2nd iteration > write(CONTINUE) > // 2nd iteration > expect(CONTINUE) > close() // no connect() to race > listen() > write(LISTENING) > expect(LISTENING) > connect() // no close() to race > > Hope it makes sense? > Sorry, it's Friday ;-P Yep, now it makes sense, so please add a little comment that the goal is to stress the race between connect() and close(listener). Have a nice weekend, Stefano
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c index 38fd8d96eb83ef1bd45728cfaac6adb3c1e07cfe..48b6d970bcfa95f957facb7ba2e729a32d256b4a 100644 --- a/tools/testing/vsock/vsock_test.c +++ b/tools/testing/vsock/vsock_test.c @@ -1474,6 +1474,45 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts) test_stream_credit_update_test(opts, false); } +#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ + +static void test_stream_leak_acceptq_client(const struct test_opts *opts) +{ + struct sockaddr_vm addr = { + .svm_family = AF_VSOCK, + .svm_port = opts->peer_port, + .svm_cid = opts->peer_cid + }; + time_t tout; + int fd; + + tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; + do { + control_writeulong(1); + + fd = socket(AF_VSOCK, SOCK_STREAM, 0); + if (fd < 0) { + perror("socket"); + exit(EXIT_FAILURE); + } + + connect(fd, (struct sockaddr *)&addr, sizeof(addr)); + close(fd); + } while (current_nsec() < tout); + + control_writeulong(0); +} + +static void test_stream_leak_acceptq_server(const struct test_opts *opts) +{ + int fd; + + while (control_readulong()) { + fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); + close(fd); + } +} + static struct test_case test_cases[] = { { .name = "SOCK_STREAM connection reset", @@ -1604,6 +1643,11 @@ static struct test_case test_cases[] = { .run_client = test_seqpacket_unsent_bytes_client, .run_server = test_seqpacket_unsent_bytes_server, }, + { + .name = "SOCK_STREAM leak accept queue", + .run_client = test_stream_leak_acceptq_client, + .run_server = test_stream_leak_acceptq_server, + }, {}, };
Attempt to enqueue a child after the queue was flushed, but before SOCK_DONE flag has been set. Fixed by commit d7b0ff5a8667 ("virtio/vsock: Fix accept_queue memory leak"). Signed-off-by: Michal Luczaj <mhal@rbox.co> --- tools/testing/vsock/vsock_test.c | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)