netdev - Re: [PATCH net-next v2 4/6] vsock/test: Add test for accept

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGxU2F6oyu0pGbFOhM-KhmmQaYzib8pTWt3xEpOjO2VRYT8ucw@mail.gmail.com>
Date: Mon, 16 Dec 2024 16:02:38 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 4/6] vsock/test: Add test for accept_queue
 memory leak

On Mon, Dec 16, 2024 at 3:52 PM Michal Luczaj <mhal@...x.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@...x.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