Message-ID: <939c78fc-ea90-4e5d-a9b1-e750dd6b4e25@yahoo.com>
Date: Fri, 31 Oct 2025 21:01:56 +0100
From: Sunday Adelodun <adelodunolaoluwa@...oo.com>
To: Kuniyuki Iwashima <kuniyu@...gle.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 10/31/25 20:41, Kuniyuki Iwashima wrote:
> 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 :)
I will go for if then.
>
>> test1:
>>
>> ...
>>
>> switch (variant->socket_type) {
>>
>> case SOCK_STREAM:
>>
>> case SOCK_SEQPACKET:
>>
>>           ASSERT_EQ(0, n);
> You need break; here.
Thank you. It was an omission
>
>> 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.
I will look into this
>
>> }
>>
>> ...
>>
>> 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!
>
Thank you for these.
I will work on them and send v3 shortly.

Thanks once again.

Powered by blists - more mailing lists