| Message ID | 20241216-test-vsock-leaks-v2-4-55e1405742fc@rbox.co (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock/test: Tests for memory leaks | expand |
On Mon, Dec 16, 2024 at 01:01:00PM +0100, Michal Luczaj wrote: >Attempt to enqueue a child after the queue was flushed, but before >SOCK_DONE flag has been set. > >Test tries to produce a memory leak, kmemleak should be employed. Dealing >with a race condition, test by its very nature may lead to a false >negative. > >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 | 51 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > >diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >index 1ad1fbba10307c515e31816a2529befd547f7fd7..1a9bd81758675a0f2b9b6b0ad9271c45f89a4860 100644 >--- a/tools/testing/vsock/vsock_test.c >+++ b/tools/testing/vsock/vsock_test.c >@@ -1474,6 +1474,52 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts) > test_stream_credit_update_test(opts, false); > } > >+/* The goal of test leak_acceptq is to stress the race between connect() and >+ * close(listener). Implementation of client/server loops boils down to: >+ * >+ * client server >+ * ------ ------ >+ * write(CONTINUE) >+ * expect(CONTINUE) >+ * listen() >+ * write(LISTENING) >+ * expect(LISTENING) >+ * connect() close() >+ */ >+#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ >+ >+#define CONTINUE 1 >+#define DONE 0 I would add a prefix here, looking at the timeout, I would say ACCEPTQ_LEAK_RACE_CONTINUE and ACCEPTQ_LEAK_RACE_DONE. The rest LKGT! Stefano >+ >+static void test_stream_leak_acceptq_client(const struct test_opts *opts) >+{ >+ time_t tout; >+ int fd; >+ >+ tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; >+ do { >+ control_writeulong(CONTINUE); >+ >+ fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); >+ if (fd >= 0) >+ close(fd); >+ } while (current_nsec() < tout); >+ >+ control_writeulong(DONE); >+} >+ >+/* Test for a memory leak. User is expected to run kmemleak scan, see README. */ >+static void test_stream_leak_acceptq_server(const struct test_opts *opts) >+{ >+ int fd; >+ >+ while (control_readulong() == CONTINUE) { >+ fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); >+ control_writeln("LISTENING"); >+ close(fd); >+ } >+} >+ > static struct test_case test_cases[] = { > { > .name = "SOCK_STREAM connection reset", >@@ -1604,6 +1650,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/16/24 15:35, Stefano Garzarella wrote: > On Mon, Dec 16, 2024 at 01:01:00PM +0100, Michal Luczaj wrote: >> Attempt to enqueue a child after the queue was flushed, but before >> SOCK_DONE flag has been set. >> >> Test tries to produce a memory leak, kmemleak should be employed. Dealing >> with a race condition, test by its very nature may lead to a false >> negative. >> >> 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 | 51 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >> index 1ad1fbba10307c515e31816a2529befd547f7fd7..1a9bd81758675a0f2b9b6b0ad9271c45f89a4860 100644 >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -1474,6 +1474,52 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts) >> test_stream_credit_update_test(opts, false); >> } >> >> +/* The goal of test leak_acceptq is to stress the race between connect() and >> + * close(listener). Implementation of client/server loops boils down to: >> + * >> + * client server >> + * ------ ------ >> + * write(CONTINUE) >> + * expect(CONTINUE) >> + * listen() >> + * write(LISTENING) >> + * expect(LISTENING) >> + * connect() close() >> + */ >> +#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ >> + >> +#define CONTINUE 1 >> +#define DONE 0 > > I would add a prefix here, looking at the timeout, I would say > ACCEPTQ_LEAK_RACE_CONTINUE and ACCEPTQ_LEAK_RACE_DONE. I was hoping to make them useful for other tests (see failslab example in patch 6/6). If CONTINUE/DONE is too generic, how about prefixing them with something like LOOP_ or TEST_ or CONTROL_ ?
On Mon, Dec 16, 2024 at 3:52 PM Michal Luczaj <mhal@rbox.co> wrote: > > On 12/16/24 15:35, Stefano Garzarella wrote: > > On Mon, Dec 16, 2024 at 01:01:00PM +0100, Michal Luczaj wrote: > >> Attempt to enqueue a child after the queue was flushed, but before > >> SOCK_DONE flag has been set. > >> > >> Test tries to produce a memory leak, kmemleak should be employed. Dealing > >> with a race condition, test by its very nature may lead to a false > >> negative. > >> > >> 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 | 51 ++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 51 insertions(+) > >> > >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c > >> index 1ad1fbba10307c515e31816a2529befd547f7fd7..1a9bd81758675a0f2b9b6b0ad9271c45f89a4860 100644 > >> --- a/tools/testing/vsock/vsock_test.c > >> +++ b/tools/testing/vsock/vsock_test.c > >> @@ -1474,6 +1474,52 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts) > >> test_stream_credit_update_test(opts, false); > >> } > >> > >> +/* The goal of test leak_acceptq is to stress the race between connect() and > >> + * close(listener). Implementation of client/server loops boils down to: > >> + * > >> + * client server > >> + * ------ ------ > >> + * write(CONTINUE) > >> + * expect(CONTINUE) > >> + * listen() > >> + * write(LISTENING) > >> + * expect(LISTENING) > >> + * connect() close() > >> + */ > >> +#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ > >> + > >> +#define CONTINUE 1 > >> +#define DONE 0 > > > > I would add a prefix here, looking at the timeout, I would say > > ACCEPTQ_LEAK_RACE_CONTINUE and ACCEPTQ_LEAK_RACE_DONE. > > I was hoping to make them useful for other tests (see failslab example in > patch 6/6). If CONTINUE/DONE is too generic, how about prefixing them with > something like LOOP_ or TEST_ or CONTROL_ ? > In that case, I'd add them on top of the file. CONTROL_CONTINUE, CONTROL_DONE looks good, but also others are okay, up to you. Thanks, Stefano
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c index 1ad1fbba10307c515e31816a2529befd547f7fd7..1a9bd81758675a0f2b9b6b0ad9271c45f89a4860 100644 --- a/tools/testing/vsock/vsock_test.c +++ b/tools/testing/vsock/vsock_test.c @@ -1474,6 +1474,52 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts) test_stream_credit_update_test(opts, false); } +/* The goal of test leak_acceptq is to stress the race between connect() and + * close(listener). Implementation of client/server loops boils down to: + * + * client server + * ------ ------ + * write(CONTINUE) + * expect(CONTINUE) + * listen() + * write(LISTENING) + * expect(LISTENING) + * connect() close() + */ +#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */ + +#define CONTINUE 1 +#define DONE 0 + +static void test_stream_leak_acceptq_client(const struct test_opts *opts) +{ + time_t tout; + int fd; + + tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC; + do { + control_writeulong(CONTINUE); + + fd = vsock_stream_connect(opts->peer_cid, opts->peer_port); + if (fd >= 0) + close(fd); + } while (current_nsec() < tout); + + control_writeulong(DONE); +} + +/* Test for a memory leak. User is expected to run kmemleak scan, see README. */ +static void test_stream_leak_acceptq_server(const struct test_opts *opts) +{ + int fd; + + while (control_readulong() == CONTINUE) { + fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port); + control_writeln("LISTENING"); + close(fd); + } +} + static struct test_case test_cases[] = { { .name = "SOCK_STREAM connection reset", @@ -1604,6 +1650,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. Test tries to produce a memory leak, kmemleak should be employed. Dealing with a race condition, test by its very nature may lead to a false negative. 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 | 51 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)