Re: [PATCH RFC net 2/2] tcp: Add selftest for bind() and TIME_WAIT.

[Joanne Koong <joannelkoong@gmail.com>]

On Wed, Dec 21, 2022 at 7:14 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> bhash2 split the bind() validation logic into wildcard and non-wildcard
> cases.  Let's add a test to catch the same regression.
>
> Before the previous patch:
>
>   # ./bind_timewait
>   TAP version 13
>   1..2
>   # Starting 2 tests from 3 test cases.
>   #  RUN           bind_timewait.localhost.1 ...
>   # bind_timewait.c:87:1:Expected ret (0) == -1 (-1)
>   # 1: Test terminated by assertion
>   #          FAIL  bind_timewait.localhost.1
>   not ok 1 bind_timewait.localhost.1
>   #  RUN           bind_timewait.addrany.1 ...
>   #            OK  bind_timewait.addrany.1
>   ok 2 bind_timewait.addrany.1
>   # FAILED: 1 / 2 tests passed.
>   # Totals: pass:1 fail:1 xfail:0 xpass:0 skip:0 error:0
>
> After:
>
>   # ./bind_timewait
>   TAP version 13
>   1..2
>   # Starting 2 tests from 3 test cases.
>   #  RUN           bind_timewait.localhost.1 ...
>   #            OK  bind_timewait.localhost.1
>   ok 1 bind_timewait.localhost.1
>   #  RUN           bind_timewait.addrany.1 ...
>   #            OK  bind_timewait.addrany.1
>   ok 2 bind_timewait.addrany.1
>   # PASSED: 2 / 2 tests passed.
>   # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  tools/testing/selftests/net/.gitignore      |  1 +
>  tools/testing/selftests/net/bind_timewait.c | 93 +++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>  create mode 100644 tools/testing/selftests/net/bind_timewait.c
>
> diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
> index 9cc84114741d..a6911cae368c 100644
> --- a/tools/testing/selftests/net/.gitignore
> +++ b/tools/testing/selftests/net/.gitignore
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  bind_bhash
> +bind_timewait
>  csum
>  cmsg_sender
>  diag_uid
> diff --git a/tools/testing/selftests/net/bind_timewait.c b/tools/testing/selftests/net/bind_timewait.c
> new file mode 100644
> index 000000000000..2d40403128ff
> --- /dev/null
> +++ b/tools/testing/selftests/net/bind_timewait.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright Amazon.com Inc. or its affiliates. */
> +
> +#include "../kselftest_harness.h"

nit: Not sure if this matters or not, but from looking at the other
selftests/net it seems like the convention is to have relative path
#include defined below absolute path #includes.

> +
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <netinet/tcp.h>

nit: i don't think we need this netinet/tcp.h include

> +
> +FIXTURE(bind_timewait)
> +{
> +       struct sockaddr_in addr;
> +       socklen_t addrlen;
> +};
> +
> +FIXTURE_VARIANT(bind_timewait)
> +{
> +       __u32 addr_const;
> +};
> +
> +FIXTURE_VARIANT_ADD(bind_timewait, localhost)
> +{
> +       .addr_const = INADDR_LOOPBACK
> +};
> +
> +FIXTURE_VARIANT_ADD(bind_timewait, addrany)
> +{
> +       .addr_const = INADDR_ANY
> +};
> +
> +FIXTURE_SETUP(bind_timewait)
> +{
> +       self->addr.sin_family = AF_INET;
> +       self->addr.sin_port = 0;
> +       self->addr.sin_addr.s_addr = htonl(variant->addr_const);
> +       self->addrlen = sizeof(self->addr);
> +}
> +
> +FIXTURE_TEARDOWN(bind_timewait)
> +{
> +}
> +
> +void create_timewait_socket(struct __test_metadata *_metadata,
> +                           FIXTURE_DATA(bind_timewait) *self)
> +{
> +       int server_fd, client_fd, child_fd, ret;
> +       struct sockaddr_in addr;
> +       socklen_t addrlen;
> +
> +       server_fd = socket(AF_INET, SOCK_STREAM, 0);
> +       ASSERT_GT(server_fd, 0);

If any of these assertions fail, do we leak fds because we don't get
to calling the close()s at the end of this function? Do we need to
have the fds cleaned up in the teardown fixture function?

> +
> +       ret = bind(server_fd, (struct sockaddr *)&self->addr, self->addrlen);
> +       ASSERT_EQ(ret, 0);
> +
> +       ret = listen(server_fd, 1);
> +       ASSERT_EQ(ret, 0);
> +
> +       ret = getsockname(server_fd, (struct sockaddr *)&self->addr, &self->addrlen);
> +       ASSERT_EQ(ret, 0);
> +
> +       client_fd = socket(AF_INET, SOCK_STREAM, 0);
> +       ASSERT_GT(client_fd, 0);
> +
> +       ret = connect(client_fd, (struct sockaddr *)&self->addr, self->addrlen);
> +       ASSERT_EQ(ret, 0);
> +
> +       addrlen = sizeof(addr);
> +       child_fd = accept(server_fd, (struct sockaddr *)&addr, &addrlen);
> +       ASSERT_GT(child_fd, 0);
> +
> +       close(child_fd);
> +       close(client_fd);
> +       close(server_fd);
> +}
> +
> +TEST_F(bind_timewait, 1)
> +{
> +       int fd, ret;
> +
> +       create_timewait_socket(_metadata, self);
> +
> +       fd = socket(AF_INET, SOCK_STREAM, 0);
> +       ASSERT_GT(fd, 0);
> +
> +       ret = bind(fd, (struct sockaddr *)&self->addr, self->addrlen);
> +       ASSERT_EQ(ret, -1);
> +       ASSERT_EQ(errno, EADDRINUSE);
> +
> +       close(fd);
> +}
> +
> +TEST_HARNESS_MAIN
> --
> 2.30.2
>