| 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
| ||
|
Message-ID: <3c01e222-8e58-46ce-9ce7-b536bfb11028@yahoo.com>
Date: Wed, 12 Nov 2025 21:32:59 +0100
From: Sunday Adelodun <adelodunolaoluwa@...oo.com>
To: Kuniyuki Iwashima <kuniyu@...gle.com>
Cc: davem@...emloft.net, david.hunter.linux@...il.com, edumazet@...gle.com,
horms@...nel.org, kuba@...nel.org,
linux-kernel-mentees@...ts.linuxfoundation.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
netdev@...r.kernel.org, pabeni@...hat.com, shuah@...nel.org,
skhan@...uxfoundation.org
Subject: Re: [PATCH v3] selftests: af_unix: Add tests for ECONNRESET and EOF
semantics
On 11/7/25 08:05, Kuniyuki Iwashima wrote:
> From: Sunday Adelodun <adelodunolaoluwa@...oo.com>
> Date: Tue, 4 Nov 2025 11:42:47 +0100
>> On 11/4/25 01:30, Kuniyuki Iwashima wrote:
>>> On Mon, Nov 3, 2025 at 4:08 PM Sunday Adelodun
>>> <adelodunolaoluwa@...oo.com> wrote:
>>>> On 11/2/25 08:32, Kuniyuki Iwashima wrote:
>>>>> On Sat, Nov 1, 2025 at 10:23 AM 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 verify that:
>>>>>>
>>>>>> 1. SOCK_STREAM returns EOF when the peer closes normally.
>>>>>> 2. SOCK_STREAM returns ECONNRESET if the peer closes with unread data.
>>>>>> 3. SOCK_SEQPACKET returns EOF when the peer closes normally.
>>>>>> 4. SOCK_SEQPACKET returns ECONNRESET if the peer closes with unread data.
>>>>>> 5. SOCK_DGRAM does not return ECONNRESET when the peer closes.
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> tools/testing/selftests/net/af_unix/Makefile | 1 +
>>>>>> .../selftests/net/af_unix/unix_connreset.c | 179 ++++++++++++++++++
>>>>>> 2 files changed, 180 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 \
>>>>> patchwork caught this test is not added to .gitignore.
>>>>> https://patchwork.kernel.org/project/netdevbpf/patch/20251101172230.10179-1-adelodunolaoluwa@yahoo.com/
>>>>>
>>>>> Could you add it to this file ?
>>>>>
>>>>> tools/testing/selftests/net/.gitignore
>>>> Oh, thank you for this. will add it
>>>>>> # 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..6f43435d96e2
>>>>>> --- /dev/null
>>>>>> +++ b/tools/testing/selftests/net/af_unix/unix_connreset.c
>>>>>> @@ -0,0 +1,179 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * Selftest for AF_UNIX socket close and ECONNRESET behaviour.
>>>>>> + *
>>>>>> + * This test verifies:
>>>>>> + * 1. SOCK_STREAM returns EOF when the peer closes normally.
>>>>>> + * 2. SOCK_STREAM returns ECONNRESET if peer closes with unread data.
>>>>>> + * 3. SOCK_SEQPACKET returns EOF when the peer closes normally.
>>>>>> + * 4. SOCK_SEQPACKET returns ECONNRESET if the peer closes with unread data.
>>>>>> + * 5. SOCK_DGRAM does not return ECONNRESET when the 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 */
>>>>> nit: outdated, maybe simply remove ?
>>>> oh..skipped me.
>>>> will do so.
>>>>>> +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",
>>>>>> +};
>>>>>> +
>>>>>> +FIXTURE_VARIANT_ADD(unix_sock, seqpacket) {
>>>>>> + .socket_type = SOCK_SEQPACKET,
>>>>>> + .name = "SOCK_SEQPACKET",
>>>>>> +};
>>>>>> +
>>>>>> +FIXTURE_SETUP(unix_sock)
>>>>>> +{
>>>>>> + struct sockaddr_un addr = {};
>>>>>> + int err;
>>>>>> +
>>>>>> + addr.sun_family = AF_UNIX;
>>>>>> + strcpy(addr.sun_path, SOCK_PATH);
>>>>>> + remove_socket_file();
>>>>>> +
>>>>>> + 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 ||
>>>>>> + variant->socket_type == SOCK_SEQPACKET) {
>>>>> patchwork caught mis-alignment here and other places.
>>>>>
>>>>> I'm using this for emacs, and other editors will have a similar config.
>>>>>
>>>>> (setq-default c-default-style "linux")
>>>>>
>>>>> You can check if lines are aligned properly by
>>>>>
>>>>> $ git show --format=email | ./scripts/checkpatch.pl
>>>>>
>>>>>
>>>>>> + err = listen(self->server, 1);
>>>>>> + ASSERT_EQ(0, err);
>>>>>> +
>>>>>> + self->client = socket(AF_UNIX, variant->socket_type, 0);
>>>>> Could you add SOCK_NONBLOCK here too ?
>>>> This is noted
>>>>>> + 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 ||
>>>>>> + variant->socket_type == SOCK_SEQPACKET)
>>>>>> + 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;
>>>>>> +
>>>>>> + /* Peer closes normally */
>>>>>> + if (variant->socket_type == SOCK_STREAM ||
>>>>>> + variant->socket_type == SOCK_SEQPACKET)
>>>>>> + close(self->child);
>>>>>> + else
>>>>>> + close(self->server);
>>>>>> +
>>>>>> + n = recv(self->client, buf, sizeof(buf), 0);
>>>>>> + TH_LOG("%s: recv=%zd errno=%d (%s)", variant->name, n, errno, strerror(errno));
>>>>> errno is undefined if not set, and same for strerror(errno).
>>>>>
>>>>> Also, if ASSERT_XXX() below fails, the same information
>>>>> (test case, errno) is logged. So, TH_LOG() seems unnecessary.
>>>>>
>>>>> Maybe try modifying the condition below and see how the
>>>>> assertion is logged.
>>>> Oh..thank you. Didn't it through that way.
>>>> I understand.
>>>> I will remove the TH_LOG()'s
>>>>>> + if (variant->socket_type == SOCK_STREAM ||
>>>>>> + variant->socket_type == SOCK_SEQPACKET) {
>>>>>> + ASSERT_EQ(0, n);
>>>>>> + } else {
>>>>>> + ASSERT_EQ(-1, n);
>>>>>> + ASSERT_EQ(EAGAIN, errno);
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> +/* Test 2: peer closes with unread data */
>>>>>> +TEST_F(unix_sock, reset_unread)
>>>>>> +{
>>>>>> + char buf[16] = {};
>>>>>> + ssize_t n;
>>>>>> +
>>>>>> + /* Send data that will remain unread by client */
>>>>>> + send(self->client, "hello", 5, 0);
>>>>>> + 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 (variant->socket_type == SOCK_STREAM ||
>>>>>> + variant->socket_type == SOCK_SEQPACKET) {
>>>>>> + ASSERT_EQ(-1, n);
>>>>>> + ASSERT_EQ(ECONNRESET, errno);
>>>>>> + } else {
>>>>>> + ASSERT_EQ(-1, n);
>>>>>> + ASSERT_EQ(EAGAIN, errno);
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> +/* Test 3: SOCK_DGRAM peer close */
>>>>>> Now Test 2 and Test 3 look identical ;)
>>>> seems so, but the only difference is:
>>>>
>>>> close(self->child); is used in Test 2, while
>>>> close(self->server); is used in Test 3.
>>>> Maybe I should find a way to collapse Tests 2 and 3 (if statement might
>>>> work)
>>>>
>>>> I am just afraid the tests to run will reduce to 6 from 9 and we will have 6
>>>> cases passed as against 7 as before.
>>>>
>>>> What do you think?
>>> The name of Test 2 is a bit confusing, which is not true
>>> for SOCK_DGRAM. So, I'd use "if" to change which fd
>>> to close() depending on the socket type.
>>>
>>> Also, close(self->server) does nothing for SOCK_STREAM
>>> and SOCK_SEQPACKET after accept(). Rather, that close()
>>> should have the same effect if self->child is not accept()ed.
>>> (In this case, Skip() for SOCK_DGRAM makes sense)
>>>
>>> I think covering that scenario would be nicer.
>>>
>>> If interested, you can check the test coverage with this patch.
>>> https://lore.kernel.org/linux-kselftest/20251028024339.2028774-1-kuniyu@google.com/
>>>
>>> Thanks!
>> Thank you!
>>
>> kindly check these if any conforms to what it should be:
>>
>> TEST_F(unix_sock, reset_unread_behavior)
>> {
>> char buf[16] = {};
>> ssize_t n;
>>
>> /* Send data that will remain unread by client */
>> send(self->client, "hello", 5, 0);
>>
>> if (variant->socket_type == SOCK_DGRAM) {
>> close(self->server);
>> }
>> else {
>> if (!self->child)
>> SKIP(return);
>>
>> close(self->child);
>> }
>>
>> n = recv(self->client, buf, sizeof(buf), 0);
>>
>> ASSERT_EQ(-1, n);
>>
>> if (variant->socket_type == SOCK_STREAM ||
>> variant->socket_type == SOCK_SEQPACKET)
>> do { ASSERT_EQ(ECONNRESET, errno); } while (0);
>> else
>> ASSERT_EQ(EAGAIN, errno);
>> }
>>
>> OR
>>
>> TEST_F(unix_sock, reset_unread_behavior)
>> {
>> char buf[16] = {};
>> ssize_t n;
>>
>> /* Send data that will remain unread by client */
>> send(self->client, "hello", 5, 0);
>>
>> if (variant->socket_type == SOCK_DGRAM) {
>> close(self->server);
>> }
>> else {
>> if (self->child)
>> close(self->child);
>> else
>> close(self->server);
>> }
>>
>> n = recv(self->client, buf, sizeof(buf), 0);
>>
>> ASSERT_EQ(-1, n);
>>
>> if (variant->socket_type == SOCK_STREAM ||
>> variant->socket_type == SOCK_SEQPACKET)
>> do { ASSERT_EQ(ECONNRESET, errno); } while (0);
>> else
>> ASSERT_EQ(EAGAIN, errno);
>> }
>>
>> OR
>>
>> is there a better way to handle this?
> Sorry for late!
>
> What I had in mind is to move accept() in FIXTURE_SETUP() to
> Test 1 & 2 (then, only listen() is conditional in FIXTURE_SETUP())
> and rewrite Test 3 to cover the last ECONNRESET case caused by
> close()ing un-accept()ed sockets:
>
> TEST_F(unix_sock, reset_closed_embryo)
> {
> if (variant->socket_type == SOCK_DGRAM)
> SKIP(return, "This test only applies to SOCK_STREAM and SOCK_SEQPACKET");
>
> close(self->server);
>
> n = recv(self->client, ...);
> ASSERT_EQ(-1, n);
> ASSERT_EQ(ECONNRESET, errno);
> }
>
>
>> I ran the KCOV_OUTPUT command using the first *TEST_F above* as the Test
>> 2 and got the output below:
>> *$ KCOV_OUTPUT=kcov KCOV_SLOTS=8192
>> ./tools/testing/selftests/net/af_unix/unix_connreset*
> You should be able to see line-by-line coverage by decoding
> files under kcov with addr2line or vock/report.py.
>
> Thanks!
>
So sorry for the late reply.
I have noted all and I will send v4 shortly
Thanks once again
Powered by blists - more mailing lists