Message-ID: <CAAVpQUAT8CVwQfSXq+P78kgPVy8gyD9thEgBcAz45Jpxh=1smw@mail.gmail.com>
Date: Fri, 31 Oct 2025 12:41:22 -0700
From: Kuniyuki Iwashima <kuniyu@...gle.com>
To: Sunday Adelodun <adelodunolaoluwa@...oo.com>
Cc: "=David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Shuah Khan <shuah@...nel.org>, Shuah Khan <skhan@...uxfoundation.org>, 
	David Hunter <david.hunter.linux@...il.com>, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v2] selftests: af_unix: Add tests for ECONNRESET and EOF semantics

On Thu, Oct 30, 2025 at 6:45 AM Sunday Adelodun
<adelodunolaoluwa@...oo.com> wrote:
>
> On 10/28/25 19:28, Kuniyuki Iwashima wrote:
> > On Sat, Oct 25, 2025 at 12:03 PM Sunday Adelodun
> > <adelodunolaoluwa@...oo.com> wrote:
> >> Add selftests to verify and document Linux’s intended behaviour for
> >> UNIX domain sockets (SOCK_STREAM and SOCK_DGRAM) when a peer closes.
> >> The tests cover:
> >>
> >>    1. EOF returned when a SOCK_STREAM peer closes normally.
> >>    2. ECONNRESET returned when a SOCK_STREAM peer closes with unread data.
> >>    3. SOCK_DGRAM sockets not returning ECONNRESET on peer close.
> >>
> >> This follows up on review feedback suggesting a selftest to clarify
> >> Linux’s semantics.
> >>
> >> Suggested-by: Kuniyuki Iwashima <kuniyu@...gle.com>
> >> Signed-off-by: Sunday Adelodun <adelodunolaoluwa@...oo.com>
> >> ---
> >> Changelog:
> >>
> >> Changes made from v1:
> >>
> >> - Patch prefix updated to selftest: af_unix:.
> >>
> >> - All mentions of “UNIX” changed to AF_UNIX.
> >>
> >> - Removed BSD references from comments.
> >>
> >> - Shared setup refactored using FIXTURE_VARIANT().
> >>
> >> - Cleanup moved to FIXTURE_TEARDOWN() to always run.
> >>
> >> - Tests consolidated to reduce duplication: EOF, ECONNRESET, SOCK_DGRAM peer close.
> >>
> >> - Corrected ASSERT usage and initialization style.
> >>
> >> - Makefile updated for new directory af_unix.
> >>
> >>   tools/testing/selftests/net/af_unix/Makefile  |   1 +
> >>   .../selftests/net/af_unix/unix_connreset.c    | 161 ++++++++++++++++++
> >>   2 files changed, 162 insertions(+)
> >>   create mode 100644 tools/testing/selftests/net/af_unix/unix_connreset.c
> >>
> >> diff --git a/tools/testing/selftests/net/af_unix/Makefile b/tools/testing/selftests/net/af_unix/Makefile
> >> index de805cbbdf69..5826a8372451 100644
> >> --- a/tools/testing/selftests/net/af_unix/Makefile
> >> +++ b/tools/testing/selftests/net/af_unix/Makefile
> >> @@ -7,6 +7,7 @@ TEST_GEN_PROGS := \
> >>          scm_pidfd \
> >>          scm_rights \
> >>          unix_connect \
> >> +       unix_connreset \
> >>   # end of TEST_GEN_PROGS
> >>
> >>   include ../../lib.mk
> >> diff --git a/tools/testing/selftests/net/af_unix/unix_connreset.c b/tools/testing/selftests/net/af_unix/unix_connreset.c
> >> new file mode 100644
> >> index 000000000000..c65ec997d77d
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/net/af_unix/unix_connreset.c
> >> @@ -0,0 +1,161 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Selftest for AF_UNIX socket close and ECONNRESET behaviour.
> >> + *
> >> + * This test verifies that:
> >> + *  1. SOCK_STREAM sockets return EOF when peer closes normally.
> >> + *  2. SOCK_STREAM sockets return ECONNRESET if peer closes with unread data.
> >> + *  3. SOCK_DGRAM sockets do not return ECONNRESET when peer closes.
> >> + *
> >> + * These tests document the intended Linux behaviour.
> >> + *
> >> + */
> >> +
> >> +#define _GNU_SOURCE
> >> +#include <stdlib.h>
> >> +#include <string.h>
> >> +#include <fcntl.h>
> >> +#include <unistd.h>
> >> +#include <errno.h>
> >> +#include <sys/socket.h>
> >> +#include <sys/un.h>
> >> +#include "../../kselftest_harness.h"
> >> +
> >> +#define SOCK_PATH "/tmp/af_unix_connreset.sock"
> >> +
> >> +static void remove_socket_file(void)
> >> +{
> >> +       unlink(SOCK_PATH);
> >> +}
> >> +
> >> +FIXTURE(unix_sock)
> >> +{
> >> +       int server;
> >> +       int client;
> >> +       int child;
> >> +};
> >> +
> >> +FIXTURE_VARIANT(unix_sock)
> >> +{
> >> +       int socket_type;
> >> +       const char *name;
> >> +};
> >> +
> >> +/* Define variants: stream and datagram */
> >> +FIXTURE_VARIANT_ADD(unix_sock, stream) {
> >> +       .socket_type = SOCK_STREAM,
> >> +       .name = "SOCK_STREAM",
> >> +};
> >> +
> >> +FIXTURE_VARIANT_ADD(unix_sock, dgram) {
> >> +       .socket_type = SOCK_DGRAM,
> >> +       .name = "SOCK_DGRAM",
> >> +};
> > Let's add coverage for SOCK_SEQPACKET,
> > which needs listen() / connect() but other semantics
> > are similar to SOCK_DGRAM.
>
> I will add it through:
> if (variant->socket_type == SOCK_STREAM ||
>                 variant->socket_type == SOCK_SEQPACKET)
>
>
> in both the setup and teardown fixtures with a little bit of modification
>
> where necessary (especially in the setup fixture).
>
> And also the fixture_variant_add macro.
>
> >> +
> >> +FIXTURE_SETUP(unix_sock)
> >> +{
> >> +       struct sockaddr_un addr = {};
> >> +       int err;
> >> +
> >> +       addr.sun_family = AF_UNIX;
> >> +       strcpy(addr.sun_path, SOCK_PATH);
> >> +
> >> +       self->server = socket(AF_UNIX, variant->socket_type, 0);
> >> +       ASSERT_LT(-1, self->server);
> >> +
> >> +       err = bind(self->server, (struct sockaddr *)&addr, sizeof(addr));
> >> +       ASSERT_EQ(0, err);
> >> +
> >> +       if (variant->socket_type == SOCK_STREAM) {
> >> +               err = listen(self->server, 1);
> >> +               ASSERT_EQ(0, err);
> >> +
> >> +               self->client = socket(AF_UNIX, SOCK_STREAM, 0);
> >> +               ASSERT_LT(-1, self->client);
> >> +
> >> +               err = connect(self->client, (struct sockaddr *)&addr, sizeof(addr));
> >> +               ASSERT_EQ(0, err);
> >> +
> >> +               self->child = accept(self->server, NULL, NULL);
> >> +               ASSERT_LT(-1, self->child);
> >> +       } else {
> >> +               /* Datagram: bind and connect only */
> >> +               self->client = socket(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0);
> >> +               ASSERT_LT(-1, self->client);
> >> +
> >> +               err = connect(self->client, (struct sockaddr *)&addr, sizeof(addr));
> >> +               ASSERT_EQ(0, err);
> >> +       }
> >> +}
> >> +
> >> +FIXTURE_TEARDOWN(unix_sock)
> >> +{
> >> +       if (variant->socket_type == SOCK_STREAM)
> >> +               close(self->child);
> >> +
> >> +       close(self->client);
> >> +       close(self->server);
> >> +       remove_socket_file();
> >> +}
> >> +
> >> +/* Test 1: peer closes normally */
> >> +TEST_F(unix_sock, eof)
> >> +{
> >> +       char buf[16] = {};
> >> +       ssize_t n;
> >> +
> >> +       if (variant->socket_type != SOCK_STREAM)
> >> +               SKIP(return, "This test only applies to SOCK_STREAM");
> > Instead of skipping, let's define final ASSERT() results
> > for each type.
> >
> > Same for other 2 tests.
>
> can I use a switch statement in all the tests? say, for example

switch() is completely fine, but I guess "if" will be shorter :)

>
> test1:
>
> ...
>
> switch (variant->socket_type) {
>
> case SOCK_STREAM:
>
> case SOCK_SEQPACKET:
>
>          ASSERT_EQ(0, n);

You need break; here.

>
> case SOCK_DGRAM:
>
>          ASSERT(-1, n);
>
>          ASSERT_EQ(EAGAIN, errno);
>
>          break;

And also please make sure the compiler will not complain
without default: depending on inherited build options.

>
> }
>
> ...
>
> test2:
>
> ...
>
> switch (variant->socket_type) {
>
> case SOCK_STREAM:
>
> case SOCK_SEQPACKET:
>
>          ASSERT_EQ(-1, n);
>
>          ASSERT_EQ(ECONNRESET, errno);
>
>          break;
>
> case SOCK_DGRAM:
>
>          ASSERT(-1, n);
>
>          ASSERT_EQ(EAGAIN, errno);
>
>          break;
>
> }
> ...
>
> test 3:
>
> ...
>
> switch (variant->socket_type) {
>
> case SOCK_STREAM:
>
> case SOCK_SEQPACKET:
>
>          ASSERT_EQ(-1, n);
>
>          ASSERT_EQ(ECONNRESET, errno);
>
>          break;
>
> case SOCK_DGRAM:
>
>          ASSERT(-1, n);
>
>          ASSERT_EQ(EAGAIN, errno);
>
>          break;
>
> }
>
> ...
>
> if not these, could you kindly shed more light to what you meant
>
> >
> >
> >> +
> >> +       /* Peer closes normally */
> >> +       close(self->child);
> >> +
> >> +       n = recv(self->client, buf, sizeof(buf), 0);
> >> +       TH_LOG("%s: recv=%zd errno=%d (%s)", variant->name, n, errno, strerror(errno));
> >> +       if (n == -1)
> >> +               ASSERT_EQ(ECONNRESET, errno);
> > ... otherwise, we don't see an error here
> >
> >> +
> >> +       if (n != -1)
> >> +               ASSERT_EQ(0, n);
> > and this can be checked unconditionally.
> did you mean I should remove the if (n != -1) ASSERT_EQ(0, n); part?

If SOCK_DGRAM does not reuse this test, yes.

The point is we do not want to miss future regression by
preparing both if (n == -1) case and if  (n == 0) case, one
of which should never happen at this point.

Thanks!

Powered by blists - more mailing lists