[PATCH 0/4] Stop TIF_NOTIFY_SIGNAL from interrupting work that can't be restarted

[PATCH 0/4] Stop TIF_NOTIFY_SIGNAL from interrupting work that can't be restarted

[Christian Brauner]

Ok, so I was looking into things and as usual got
side-tracked so here we are. Oleg, save me please.

IF_NOTIFY_SIGNAL is used to kick a task in uninterruptible sleep to
return to userspace and run task work and then go back to sleep. This
mechanism works well but breaks coredumps. dump_interrupted() only
allows fatal signals to interrupt a coredump and the whole regular write
path going to actual filesystems is impervious to TIF_NOTIFY_SIGNAL as
well.

The core is that you can have quite deep callchains that end up calling
signal_pending() in both the pipe and the socket codepaths so it's like
we can just pass a flag through somehow.

For coredumps its very annoying because it means io_uring is - depending
on how much outstanding work you have - incompatible with generating
non-truncated coredumps. A process with too many file backed mappings
and io_uring requests in flight ends up losing most of the coredump.

While zap_threads() has cleared TIF_SIGPENDING for a long time, just
clearing TIF_NOTIFY_SIGNAL isn't going to work because the next
completion will just set it again.

The fun part also is that io_uring isn't actually the only case:

(1) io_uring

(2) klp_send_signals()

(3) bpf_task_work_schedule_signal()

(4) landlock's tsync

And technically, kthread_stop() and the printk kunit test set the bit
raw. So no simple way of just fixing this in one subsystem.

So, a fix for this issue has the following constraints:

(i) The places where a write is aborted are deep callchains that we
    can't reasonably parameterize. For example, anon_pipe_write(),
    unix_stream_sendmsg(), unix_stream_read_generic(),
    sk_stream_wait_memory(), or a bare wait_event_interruptible() in
    wait_for_dump_helpers(). All of them are shared with regular
    syscalls that must stay interruptible.

    IOW, the state has to be per-task and ambient.

    An LLM would call this "load bearing"...

(ii) There are multiple ways TIF_NOTIFY_SIGNAL can get raised and they
     can get set from irq context against any task. As said above we
     have at least io_uring paths (poll task_work, msg_ring, tctx exit,
     io-wq via __set_notify_signal()), bpf_task_work_schedule_signal(),
     klp_send_signals(), landlock tsync, plus kthread_stop() and the
     printk kunit stuff that set the bit raw.

     So fixing this up in the individual subsystems is doomed to fail or
     require constant audits in case some new variant shows up.

(iii) The coredump task is exiting and can't restart the work.

So here's some stuff that was considered but I think it not really
feasible:

(a) Check for task_is_coredumping(). That will end up forcing
    task_work_add() users to know about coredumps. This seems like a
    layering violation.

    I have a patch for this as well but it's ugly. It races with the
    dump staarting unless the bit setter takes a lock in the io_uring
    completion. I know someone that will disagree with this approach. ;)

(b) Oleg's old suggestion iirc. Just clear the bit at coredump entry.
    That doesn't work because io_uring poll completions just raise it
    again from irq context. So you also need synchronization with the
    setter of which there are quite a few.

(c) Let the setting task defer setting the bit if the task is flagged
    and then raise it again at exit.

(d) Take TIF_NOTIFY_SIGNAL out of singal_pending() and make it opt-in at
    specific points. That breaks io_uring quite badly and forces a
    tree-wide audit.

So the amount of patches for this issue over the years is impressive. So
let me add one to the pile for the lolz.

Add PF_NO_NOTIFY_SIGNAL and helpers to raise/restore it. This is the
same approach as memalloc_nofs_save(). signal_pending() will not report
a fake pending signal via TIF_NOTIFY_SIGNAL if inside a
PF_NO_NOTIFY_SIGNAL critical section. Obviously you can't
fork()/clone3() in such a section.

Fix coredumps and smb. Fwiw, I think there's a few other potential users
of the helpers that are left out of this series.

|         |    notify_signal_pipe     |   notify_signal_socket    |
|---------|---------------------------|---------------------------|
| fix     | ok                        | ok                        |
|---------|---------------------------|---------------------------|
| unfixed | 357324 of ≥ 5111808 bytes | 467336 of ≥ 5111808 bytes |

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (4):
      signal: allow taks to temporarily block TIF_NOTIFY_SIGNAL
      coredump: prevent TIF_NOTIFY_SIGNAL from interrupting coredumps
      selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump
      smb: prevent TIF_NOTIFY_SIGNAL from interrupting

 fs/coredump.c                                      |   4 +
 fs/smb/client/transport.c                          |  13 +-
 include/linux/sched.h                              |   2 +-
 include/linux/sched/signal.h                       |  21 +-
 tools/testing/selftests/coredump/Makefile          |   7 +-
 .../selftests/coredump/coredump_notify_signal.h    |  29 ++
 .../coredump/coredump_notify_signal_helper.c       |  46 +++
 .../coredump/coredump_notify_signal_test.c         | 245 ++++++++++++++++
 tools/testing/selftests/coredump/coredump_test.h   |   1 +
 .../selftests/coredump/coredump_test_helpers.c     | 318 +++++++++++++++++++++
 10 files changed, 674 insertions(+), 12 deletions(-)
---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260817-work-tif_notify_signal-6ab080d33693

[PATCH 1/4] signal: allow taks to temporarily block TIF_NOTIFY_SIGNAL

[Christian Brauner]

TIF_NOTIFY_SIGNAL is used to kick a task in uninterruptible sleep to
return to userspace and run task work and then go back to sleep. This
mechanism works well but breaks e.g., coredumps. dump_interrupted() only
allows fatal signals to interrupt a coredump and the whole regular write
path going to actual filesystems is impervious to TIF_NOTIFY_SIGNAL as
well.

Add PF_NO_NOTIFY_SIGNAL and helpers to raise and restore it. This is the
same approach as memalloc_nofs_save(). signal_pending() will not report
a fake pending signal via TIF_NOTIFY_SIGNAL if inside a
PF_NO_NOTIFY_SIGNAL section.

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 include/linux/sched.h        |  2 +-
 include/linux/sched/signal.h | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 373bcc0598d1..fd72ca4ee92f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1796,7 +1796,7 @@ extern struct pid *cad_pid;
 						 * I am cleaning dirty pages from some other bdi. */
 #define PF_KTHREAD		0x00200000	/* I am a kernel thread */
 #define PF_RANDOMIZE		0x00400000	/* Randomize virtual address space */
-#define PF__HOLE__00800000	0x00800000
+#define PF_NO_NOTIFY_SIGNAL	0x00800000	/* see no_notify_signal_save() */
 #define PF__HOLE__01000000	0x01000000
 #define PF__HOLE__02000000	0x02000000
 #define PF_NO_SETAFFINITY	0x04000000	/* Userland is not allowed to meddle with cpus_mask */
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 584ae88b435e..3182de4ff948 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -384,14 +384,31 @@ static inline int task_sigpending(struct task_struct *p)
 	return unlikely(test_tsk_thread_flag(p,TIF_SIGPENDING));
 }
 
+/* Prevent TIF_NOTIFY_SIGNAL from interrupting this task. */
+static inline unsigned int no_notify_signal_save(void)
+{
+	unsigned int flags = current->flags & PF_NO_NOTIFY_SIGNAL;
+
+	current->flags |= PF_NO_NOTIFY_SIGNAL;
+	return flags;
+}
+
+/* Restore TIF_NOTIFY_SIGNAL. */
+static inline void no_notify_signal_restore(unsigned int flags)
+{
+	current_restore_flags(flags, PF_NO_NOTIFY_SIGNAL);
+}
+
 static inline int signal_pending(struct task_struct *p)
 {
 	/*
 	 * TIF_NOTIFY_SIGNAL isn't really a signal, but it requires the same
 	 * behavior in terms of ensuring that we break out of wait loops
-	 * so that notify signal callbacks can be processed.
+	 * so that notify signal callbacks can be processed. Not for a task
+	 * that asked not to be interrupted by it, see no_notify_signal_save().
 	 */
-	if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
+	if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)) &&
+	    likely(!(READ_ONCE(p->flags) & PF_NO_NOTIFY_SIGNAL)))
 		return 1;
 	return task_sigpending(p);
 }

-- 
2.53.0

[PATCH 2/4] coredump: prevent TIF_NOTIFY_SIGNAL from interrupting coredumps

[Christian Brauner]

TIF_NOTIFY_SIGNAL is used to kick a task in uninterruptible sleep to
return to userspace and run task work and then go back to sleep. This
mechanism works well but breaks coredumps. dump_interrupted() only
allows fatal signals to interrupt a coredump and the whole regular write
path going to actual filesystems is impervious to TIF_NOTIFY_SIGNAL as
well.

However, both the usermodehelper pipe and the coredump socket will bail
early on TIF_NOTIFY_SIGNAL. This affects the following codepaths:

- coredump_sock_recv() -> unix_stream_read_generic()
  The request/ack handshake is abandoned before any coredump data is
  sent.

- anon_pipe_write() returning -ERESTARTSYS
  Once the pipe is full this truncates the coredump.

- unix_stream_sendmsg() returning -ERESTARTSYS
  Once the send buffer is full this truncates the coredump.

- coredump_sock_wait() -> __kernel_read()
  This reports a failure that didn't happen.

- wait_for_dump_helpers() -> wait_event_interruptible()
  This stops waiting for the coredump helpers.

Such truncation is entirely invisible to userspace and all uapi bits
still indicate that a successful coredump happened. A crashing process
with a bunch of file backed mappings and io_uring thrown in loses most
of the coredump data. If the NT_FILE note goes past PAGE_SIZE mappings
it's gonzo.

TIF_NOTIFY_SIGNAL is sent by io_uring for the common case. And it uses
poll without sleeping so a completion callback runs task_work_add() from
interrupt context against the task that submitted the request. This is
the task that is running the coredump. Since that task hasn't set
work_exited (it hasn't exited yet after all) TIF_NOTIFY_SIGNAL keeps
reappearing.

A coredumping task doesn't return to userspace. The task work is run at
exit. So interrupting it doesn't buy anything and just loses the
coredump which is quite valuable.

Note that this isn't specific to io_uring. There's also
klp_send_signals(), bpf_task_work_schedule_signal(), landlock's tsync
and then  technically, kthread_stop() and the printk kunit test set the
bit raw.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/coredump.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/coredump.c b/fs/coredump.c
index e68a76ff92a3..edb5a1a61d84 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -1165,6 +1165,7 @@ void vfs_coredump(const kernel_siginfo_t *siginfo)
 	struct core_name cn;
 	const struct mm_struct *mm = current->mm;
 	const struct linux_binfmt *binfmt = mm->binfmt;
+	unsigned int notify_flags;
 	int argc = 0;
 	struct coredump_params cprm = {
 		.siginfo = siginfo,
@@ -1196,9 +1197,12 @@ void vfs_coredump(const kernel_siginfo_t *siginfo)
 	if (coredump_wait(siginfo->si_signo, &core_state) < 0)
 		return;
 
+	/* Task work must not cut the dump short, see signal_pending(). */
+	notify_flags = no_notify_signal_save();
 	scoped_with_creds(cred)
 		do_coredump(&cn, &cprm, &argv, &argc, binfmt);
 	coredump_cleanup(&cn, &cprm);
+	no_notify_signal_restore(notify_flags);
 	return;
 }
 

-- 
2.53.0

[PATCH 3/4] selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump

[Christian Brauner]

Add a test that verifies that a coredump cannot be cut short by
TIF_NOTIFY_SIGNAL through io_uring running task work for uninterruptible
tasks.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 tools/testing/selftests/coredump/Makefile          |   7 +-
 .../selftests/coredump/coredump_notify_signal.h    |  29 ++
 .../coredump/coredump_notify_signal_helper.c       |  46 +++
 .../coredump/coredump_notify_signal_test.c         | 245 ++++++++++++++++
 tools/testing/selftests/coredump/coredump_test.h   |   1 +
 .../selftests/coredump/coredump_test_helpers.c     | 318 +++++++++++++++++++++
 6 files changed, 645 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/coredump/Makefile b/tools/testing/selftests/coredump/Makefile
index dece1a31d561..728f3c342eb9 100644
--- a/tools/testing/selftests/coredump/Makefile
+++ b/tools/testing/selftests/coredump/Makefile
@@ -3,7 +3,10 @@ CFLAGS += -Wall -O0 -g $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
 
 TEST_GEN_PROGS := stackdump_test \
 		  coredump_socket_test \
-		  coredump_socket_protocol_test
+		  coredump_socket_protocol_test \
+		  coredump_notify_signal_test
+# Spawned by the kernel as the |helper, not a test of its own.
+TEST_GEN_FILES := coredump_notify_signal_helper
 TEST_FILES := stackdump
 
 include ../lib.mk
@@ -11,3 +14,5 @@ include ../lib.mk
 $(OUTPUT)/stackdump_test: coredump_test_helpers.c
 $(OUTPUT)/coredump_socket_test: coredump_test_helpers.c
 $(OUTPUT)/coredump_socket_protocol_test: coredump_test_helpers.c
+$(OUTPUT)/coredump_notify_signal_test: coredump_test_helpers.c
+$(OUTPUT)/coredump_notify_signal_helper: coredump_test_helpers.c
diff --git a/tools/testing/selftests/coredump/coredump_notify_signal.h b/tools/testing/selftests/coredump/coredump_notify_signal.h
new file mode 100644
index 000000000000..9a2892de8192
--- /dev/null
+++ b/tools/testing/selftests/coredump/coredump_notify_signal.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __COREDUMP_NOTIFY_SIGNAL_H
+#define __COREDUMP_NOTIFY_SIGNAL_H
+
+#include <stdbool.h>
+#include <sys/types.h>
+
+/*
+ * Define a bunch of constants we need. We create a situation where the
+ * file note note blows pasts 200K. That's way beyond the default 64K
+ * pipe ring and past the ~36K an af_unix skb holds. So we force a write
+ * to come back short.
+ */
+#define NOTIFY_SIGNAL_MAP_COUNT		4000
+#define NOTIFY_SIGNAL_ANON_BYTES	(4UL << 20)
+#define NOTIFY_SIGNAL_STALL_US		200000
+#define NOTIFY_SIGNAL_MAPFILE		"/tmp/coredump.notify_signal.mapfile"
+#define NOTIFY_SIGNAL_TRIGGER		"/tmp/coredump.notify_signal.trigger"
+#define NOTIFY_SIGNAL_CORE_FILE		"/tmp/coredump.notify_signal.core"
+#define NOTIFY_SIGNAL_CORE_TMPFILE	"/tmp/coredump.notify_signal.core.tmp"
+#define NOTIFY_SIGNAL_SOCKET		"/tmp/coredump.notify_signal.socket"
+
+void crashing_child_notify_signal(void);
+bool coredump_io_uring_available(void);
+ssize_t recv_coredump_notify_signal(int fd, int fd_out, bool arm);
+long long coredump_expected_size(const char *path, long long *tail);
+
+#endif /* __COREDUMP_NOTIFY_SIGNAL_H */
diff --git a/tools/testing/selftests/coredump/coredump_notify_signal_helper.c b/tools/testing/selftests/coredump/coredump_notify_signal_helper.c
new file mode 100644
index 000000000000..849f5c1ea736
--- /dev/null
+++ b/tools/testing/selftests/coredump/coredump_notify_signal_helper.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * The |helper half of coredump_notify_signal_test. The kernel spawns this
+ * with the coredump on stdin, so it cannot be part of the test binary.
+ * It saves the dump and, once the notes have started, trips the fifo the
+ * crashing task is polling so TIF_NOTIFY_SIGNAL is raised while the note
+ * write is in flight.
+ */
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "coredump_notify_signal.h"
+
+int main(int argc, char *argv[])
+{
+	int fd_core_file;
+	ssize_t ret;
+
+	fd_core_file = open(NOTIFY_SIGNAL_CORE_TMPFILE,
+			    O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0600);
+	if (fd_core_file < 0) {
+		fprintf(stderr, "%s: open failed: %m\n", argv[0]);
+		return EXIT_FAILURE;
+	}
+
+	ret = recv_coredump_notify_signal(STDIN_FILENO, fd_core_file, true);
+	close(fd_core_file);
+	if (ret < 0)
+		goto err;
+
+	/* The test polls for this name, so only create it once it is whole. */
+	if (rename(NOTIFY_SIGNAL_CORE_TMPFILE, NOTIFY_SIGNAL_CORE_FILE)) {
+		fprintf(stderr, "%s: rename failed: %m\n", argv[0]);
+		goto err;
+	}
+
+	return EXIT_SUCCESS;
+
+err:
+	unlink(NOTIFY_SIGNAL_CORE_TMPFILE);
+	return EXIT_FAILURE;
+}
diff --git a/tools/testing/selftests/coredump/coredump_notify_signal_test.c b/tools/testing/selftests/coredump/coredump_notify_signal_test.c
new file mode 100644
index 000000000000..e63c06a81562
--- /dev/null
+++ b/tools/testing/selftests/coredump/coredump_notify_signal_test.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * A coredump is meant to be interrupted by SIGKILL and by the freezer and
+ * by nothing else. dump_interrupted() says so, but the blocking waits
+ * underneath it test signal_pending(), which is also true for
+ * TIF_NOTIFY_SIGNAL. A crashing task that has an io_uring completion land
+ * on it mid-dump therefore keeps dumping while every wait it enters bails
+ * out at once, and the dump is silently cut short. Nothing reports it:
+ * binfmt_elf sets has_dumped before it writes anything, so WCOREDUMP()
+ * says the dump worked.
+ *
+ * A coredump note is the only dump_emit() that exceeds what the transport
+ * takes in one go, so it is the one write that is certain to block. Both
+ * tests crash a child holding enough file backed mappings for its NT_FILE
+ * note to run past that, arm an io_uring poll on it, and trip the poll
+ * while the note is being written. What comes out has to be the whole
+ * dump.
+ */
+
+#include <fcntl.h>
+#include <limits.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/un.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "coredump_test.h"
+
+FIXTURE_SETUP(coredump)
+{
+	FILE *file;
+	int ret;
+
+	self->pid_coredump_server = -ESRCH;
+	self->fd_tmpfs_detached = -1;
+	file = fopen("/proc/sys/kernel/core_pattern", "r");
+	ASSERT_NE(NULL, file);
+
+	ret = fread(self->original_core_pattern, 1,
+		    sizeof(self->original_core_pattern), file);
+	ASSERT_TRUE(ret || feof(file));
+	ASSERT_LT(ret, sizeof(self->original_core_pattern));
+
+	self->original_core_pattern[ret] = '\0';
+	self->fd_tmpfs_detached = create_detached_tmpfs();
+	ASSERT_GE(self->fd_tmpfs_detached, 0);
+
+	ret = fclose(file);
+	ASSERT_EQ(0, ret);
+
+	unlink(NOTIFY_SIGNAL_TRIGGER);
+	ASSERT_EQ(mkfifo(NOTIFY_SIGNAL_TRIGGER, 0600), 0);
+}
+
+FIXTURE_TEARDOWN(coredump)
+{
+	const char *reason;
+	FILE *file;
+	int ret, status;
+
+	if (self->pid_coredump_server > 0) {
+		kill(self->pid_coredump_server, SIGTERM);
+		waitpid(self->pid_coredump_server, &status, 0);
+	}
+	unlink(NOTIFY_SIGNAL_CORE_FILE);
+	unlink(NOTIFY_SIGNAL_CORE_TMPFILE);
+	unlink(NOTIFY_SIGNAL_SOCKET);
+	unlink(NOTIFY_SIGNAL_TRIGGER);
+	unlink(NOTIFY_SIGNAL_MAPFILE);
+
+	file = fopen("/proc/sys/kernel/core_pattern", "w");
+	if (!file) {
+		reason = "Unable to open core_pattern";
+		goto fail;
+	}
+
+	ret = fprintf(file, "%s", self->original_core_pattern);
+	if (ret < 0) {
+		reason = "Unable to write to core_pattern";
+		goto fail;
+	}
+
+	ret = fclose(file);
+	if (ret) {
+		reason = "Unable to close core_pattern";
+		goto fail;
+	}
+
+	if (self->fd_tmpfs_detached >= 0) {
+		ret = close(self->fd_tmpfs_detached);
+		if (ret < 0) {
+			reason = "Unable to close detached tmpfs";
+			goto fail;
+		}
+		self->fd_tmpfs_detached = -1;
+	}
+
+	return;
+fail:
+	/* This should never happen */
+	fprintf(stderr, "Failed to cleanup coredump test: %s\n", reason);
+}
+
+/*
+ * Check that what the helper or the server saved is the whole dump. Only
+ * a hole at the very end can be missing, see coredump_expected_size().
+ */
+static void check_whole_coredump(struct __test_metadata *const _metadata)
+{
+	long long expected, tail;
+	struct stat st;
+
+	expected = coredump_expected_size(NOTIFY_SIGNAL_CORE_FILE, &tail);
+	ASSERT_GT(expected, 0);
+	ASSERT_EQ(stat(NOTIFY_SIGNAL_CORE_FILE, &st), 0);
+	ASSERT_LE((long long)st.st_size, expected);
+	ASSERT_GE((long long)st.st_size, expected - tail);
+}
+
+/*
+ * The dump goes to a |helper, so the reader is a separate program the
+ * kernel spawns. It saves what it received to NOTIFY_SIGNAL_CORE_FILE.
+ */
+TEST_F(coredump, notify_signal_pipe)
+{
+	char pattern[PATH_MAX], helper[PATH_MAX], *p;
+	struct stat st;
+	int status, i;
+	pid_t pid;
+	ssize_t n;
+
+	if (!coredump_io_uring_available())
+		SKIP(return, "io_uring not available");
+
+	n = readlink("/proc/self/exe", helper, sizeof(helper) - 1);
+	ASSERT_GT(n, 0);
+	helper[n] = '\0';
+	p = strstr(helper, "coredump_notify_signal_test");
+	ASSERT_NE(p, NULL);
+	ASSERT_LE((size_t)(p - helper) + sizeof("coredump_notify_signal_helper"),
+		  sizeof(helper));
+	strcpy(p, "coredump_notify_signal_helper");
+	if (access(helper, X_OK))
+		SKIP(return, "coredump_notify_signal_helper not built");
+
+	ASSERT_LT(snprintf(pattern, sizeof(pattern), "|%s", helper),
+		  (int)sizeof(pattern));
+	ASSERT_TRUE(set_core_pattern(pattern));
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+	if (pid == 0)
+		crashing_child_notify_signal();
+
+	ASSERT_EQ(waitpid(pid, &status, 0), pid);
+	ASSERT_TRUE(WIFSIGNALED(status));
+
+	/* The kernel does not wait for the helper, so poll for it. */
+	for (i = 0; i < 100; i++) {
+		if (!stat(NOTIFY_SIGNAL_CORE_FILE, &st) && st.st_size)
+			break;
+		usleep(100000);
+	}
+
+	check_whole_coredump(_metadata);
+}
+
+/* The same thing with the dump going to a coredump socket. */
+TEST_F(coredump, notify_signal_socket)
+{
+	pid_t pid, pid_coredump_server;
+	int ipc_sockets[2], status;
+	char pattern[PATH_MAX];
+	char c;
+
+	if (!coredump_io_uring_available())
+		SKIP(return, "io_uring not available");
+
+	ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
+			     ipc_sockets), 0);
+	ASSERT_LT(snprintf(pattern, sizeof(pattern), "@%s",
+			   NOTIFY_SIGNAL_SOCKET), (int)sizeof(pattern));
+	ASSERT_TRUE(set_core_pattern(pattern));
+
+	pid_coredump_server = fork();
+	ASSERT_GE(pid_coredump_server, 0);
+	if (pid_coredump_server == 0) {
+		int fd_server = -1, fd_coredump = -1, fd_core_file = -1;
+		int exit_code = EXIT_FAILURE;
+
+		close(ipc_sockets[0]);
+
+		fd_server = create_and_listen_unix_socket(NOTIFY_SIGNAL_SOCKET);
+		if (fd_server < 0)
+			goto out;
+		if (write_nointr(ipc_sockets[1], "1", 1) < 0)
+			goto out;
+		close(ipc_sockets[1]);
+
+		fd_coredump = accept4(fd_server, NULL, NULL, SOCK_CLOEXEC);
+		if (fd_coredump < 0)
+			goto out;
+
+		fd_core_file = open(NOTIFY_SIGNAL_CORE_FILE,
+				    O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC,
+				    0600);
+		if (fd_core_file < 0)
+			goto out;
+
+		if (recv_coredump_notify_signal(fd_coredump, fd_core_file,
+						true) < 0)
+			goto out;
+
+		exit_code = EXIT_SUCCESS;
+out:
+		if (fd_core_file >= 0)
+			close(fd_core_file);
+		if (fd_coredump >= 0)
+			close(fd_coredump);
+		if (fd_server >= 0)
+			close(fd_server);
+		_exit(exit_code);
+	}
+	self->pid_coredump_server = pid_coredump_server;
+
+	EXPECT_EQ(close(ipc_sockets[1]), 0);
+	ASSERT_EQ(read_nointr(ipc_sockets[0], &c, 1), 1);
+	EXPECT_EQ(close(ipc_sockets[0]), 0);
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+	if (pid == 0)
+		crashing_child_notify_signal();
+
+	ASSERT_EQ(waitpid(pid, &status, 0), pid);
+	ASSERT_TRUE(WIFSIGNALED(status));
+
+	wait_and_check_coredump_server(pid_coredump_server, _metadata, self);
+
+	check_whole_coredump(_metadata);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/coredump/coredump_test.h b/tools/testing/selftests/coredump/coredump_test.h
index ed47f01fa53c..06f997ba47fd 100644
--- a/tools/testing/selftests/coredump/coredump_test.h
+++ b/tools/testing/selftests/coredump/coredump_test.h
@@ -9,6 +9,7 @@
 
 #include "../kselftest_harness.h"
 #include "../pidfd/pidfd.h"
+#include "coredump_notify_signal.h"
 
 #ifndef PAGE_SIZE
 #define PAGE_SIZE 4096
diff --git a/tools/testing/selftests/coredump/coredump_test_helpers.c b/tools/testing/selftests/coredump/coredump_test_helpers.c
index 2a20faf9cb0a..20fcc5b644c3 100644
--- a/tools/testing/selftests/coredump/coredump_test_helpers.c
+++ b/tools/testing/selftests/coredump/coredump_test_helpers.c
@@ -1,11 +1,18 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <assert.h>
+#include <elf.h>
+#include <endian.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
+#include <link.h>
+#include <linux/stddef.h>
+#include <linux/io_uring.h>
+#include <linux/swab.h>
 #include <linux/coredump.h>
 #include <linux/fs.h>
+#include <poll.h>
 #include <pthread.h>
 #include <stdbool.h>
 #include <stdio.h>
@@ -13,7 +20,9 @@
 #include <string.h>
 #include <sys/epoll.h>
 #include <sys/ioctl.h>
+#include <sys/mman.h>
 #include <sys/socket.h>
+#include <sys/syscall.h>
 #include <sys/types.h>
 #include <sys/un.h>
 #include <sys/wait.h>
@@ -21,6 +30,7 @@
 
 #include "../filesystems/wrappers.h"
 #include "../pidfd/pidfd.h"
+#include "coredump_notify_signal.h"
 
 /* Forward declarations to avoid including harness header */
 struct __test_metadata;
@@ -381,3 +391,311 @@ void process_coredump_worker(int fd_coredump, int fd_peer_pidfd, int fd_core_fil
 		close(fd_coredump);
 	_exit(exit_code);
 }
+
+/*
+ * TIF_NOTIFY_SIGNAL coredump helpers.
+ *
+ * __dump_emit() takes anything short of a full write as the end of the
+ * dump, so every emit that blocks on a full transport can lose the rest
+ * of it. The NT_FILE note is the one emit that is certain to block,
+ * because it is the only one larger than the transport, so these helpers
+ * build a note large enough for that and then raise TIF_NOTIFY_SIGNAL on
+ * the dumping task while that write is in flight.
+ */
+
+static int io_uring_setup_raw(unsigned int entries, struct io_uring_params *p)
+{
+	return syscall(__NR_io_uring_setup, entries, p);
+}
+
+/* io_uring reads poll32_events back through swahw32() on big endian. */
+static __u32 notify_poll_mask(__u32 events)
+{
+#if defined(__BYTE_ORDER) && __BYTE_ORDER == __BIG_ENDIAN
+	return __swahw32(events);
+#else
+	return events;
+#endif
+}
+
+bool coredump_io_uring_available(void)
+{
+	struct io_uring_params p = {};
+	int fd;
+
+	fd = io_uring_setup_raw(1, &p);
+	if (fd < 0)
+		return false;
+	close(fd);
+	return true;
+}
+
+/*
+ * Arm a poll on @fd. io_uring leaves ctx->notify_method at TWA_SIGNAL
+ * unless the ring asks for SQPOLL or COOP_TASKRUN, so the completion runs
+ * set_notify_signal() against the task that submitted it. That is us, and
+ * we are about to become the coredumping task.
+ */
+static int arm_poll_notify(int trigger_fd)
+{
+	struct io_uring_params p = {};
+	unsigned int *sq_tail, *sq_array;
+	struct io_uring_sqe *sqes;
+	size_t sqring_sz;
+	void *sq;
+	int ring;
+
+	ring = io_uring_setup_raw(8, &p);
+	if (ring < 0)
+		return -1;
+
+	sqring_sz = p.sq_off.array + p.sq_entries * sizeof(unsigned int);
+	sq = mmap(NULL, sqring_sz, PROT_READ | PROT_WRITE,
+		  MAP_SHARED | MAP_POPULATE, ring, IORING_OFF_SQ_RING);
+	if (sq == MAP_FAILED)
+		return -1;
+
+	sqes = mmap(NULL, p.sq_entries * sizeof(*sqes), PROT_READ | PROT_WRITE,
+		    MAP_SHARED | MAP_POPULATE, ring, IORING_OFF_SQES);
+	if (sqes == MAP_FAILED)
+		return -1;
+
+	sq_tail = (unsigned int *)((char *)sq + p.sq_off.tail);
+	sq_array = (unsigned int *)((char *)sq + p.sq_off.array);
+
+	memset(&sqes[0], 0, sizeof(sqes[0]));
+	sqes[0].opcode = IORING_OP_POLL_ADD;
+	sqes[0].fd = trigger_fd;
+	sqes[0].poll32_events = notify_poll_mask(POLLIN);
+
+	sq_array[0] = 0;
+	__atomic_store_n(sq_tail, 1, __ATOMIC_RELEASE);
+
+	if (syscall(__NR_io_uring_enter, ring, 1, 0, 0, NULL, 0) < 0)
+		return -1;
+
+	/* Deliberately leaked, we are about to crash. */
+	return 0;
+}
+
+/*
+ * Adjacent mappings with identical flags and contiguous file offsets are
+ * merged into one VMA, which would collapse NT_FILE back to nothing, so
+ * alternate the protection to keep every mapping an entry of its own.
+ * Not PROT_EXEC, /tmp is often mounted noexec. Nothing is ever written
+ * through these so they get no anon_vma and stay out of the dump itself.
+ */
+static int make_file_mappings(void)
+{
+	long pgsz = sysconf(_SC_PAGESIZE);
+	int fd, i;
+
+	fd = open(NOTIFY_SIGNAL_MAPFILE,
+		  O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0600);
+	if (fd < 0)
+		return 0;
+	if (ftruncate(fd, (off_t)NOTIFY_SIGNAL_MAP_COUNT * pgsz)) {
+		close(fd);
+		return 0;
+	}
+
+	for (i = 0; i < NOTIFY_SIGNAL_MAP_COUNT; i++) {
+		int prot = (i & 1) ? PROT_READ : (PROT_READ | PROT_WRITE);
+
+		if (mmap(NULL, pgsz, prot, MAP_PRIVATE, fd,
+			 (off_t)i * pgsz) == MAP_FAILED)
+			break;
+	}
+	close(fd);
+	return i;
+}
+
+void crashing_child_notify_signal(void)
+{
+	long pgsz = sysconf(_SC_PAGESIZE);
+	unsigned char *p;
+	int trigger_fd;
+	unsigned long off;
+
+	/* Open the read side first so the reader's open() cannot block. */
+	trigger_fd = open(NOTIFY_SIGNAL_TRIGGER, O_RDONLY | O_NONBLOCK | O_CLOEXEC);
+
+	/* Exit rather than crash: a dump without the poll armed proves nothing. */
+	if (trigger_fd < 0)
+		_exit(EXIT_FAILURE);
+
+	if (make_file_mappings() < NOTIFY_SIGNAL_MAP_COUNT)
+		_exit(EXIT_FAILURE);
+
+	p = mmap(NULL, NOTIFY_SIGNAL_ANON_BYTES, PROT_READ | PROT_WRITE,
+		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (p == MAP_FAILED)
+		_exit(EXIT_FAILURE);
+	for (off = 0; off < NOTIFY_SIGNAL_ANON_BYTES; off += pgsz)
+		p[off] = 1;
+
+	if (arm_poll_notify(trigger_fd))
+		_exit(EXIT_FAILURE);
+
+	/* crash on purpose */
+	*(volatile int *)NULL = 0;
+	_exit(EXIT_FAILURE);
+}
+
+static int pull_trigger(void)
+{
+	int fd;
+
+	fd = open(NOTIFY_SIGNAL_TRIGGER, O_WRONLY | O_NONBLOCK | O_CLOEXEC);
+	if (fd < 0) {
+		fprintf(stderr, "%s: open failed: %m\n", __func__);
+		return -1;
+	}
+	if (write(fd, "x", 1) != 1) {
+		fprintf(stderr, "%s: write failed: %m\n", __func__);
+		close(fd);
+		return -1;
+	}
+	close(fd);
+	return 0;
+}
+
+/*
+ * phdr[0] is the PT_NOTE entry: elf_core_dump() emits it right after the
+ * ELF header. Its p_offset is where the notes begin.
+ */
+static long long note_offset(const unsigned char *hdr)
+{
+	ElfW(Phdr) ph;
+	ElfW(Ehdr) eh;
+
+	memcpy(&eh, hdr, sizeof(eh));
+	if (memcmp(eh.e_ident, ELFMAG, SELFMAG) || eh.e_type != ET_CORE)
+		return -1;
+	memcpy(&ph, hdr + sizeof(eh), sizeof(ph));
+	if (ph.p_type != PT_NOTE)
+		return -1;
+	return (long long)ph.p_offset;
+}
+
+/*
+ * Drain a coredump off @fd, counting what arrives. Once the notes have
+ * started the kernel is inside the one big note write, so poke the fifo
+ * the crashing task is polling and stop reading, which keeps the
+ * transport full and the write blocked with a partial count when the
+ * wakeup lands. Then carry on to end of file.
+ *
+ * What arrives is written to @fd_out when that is not negative.
+ * Returns the number of bytes received, or -1. Failing to trip the fifo
+ * is an error too: a dump that was never interrupted proves nothing.
+ */
+ssize_t recv_coredump_notify_signal(int fd, int fd_out, bool arm)
+{
+	unsigned char hdr[sizeof(ElfW(Ehdr)) + sizeof(ElfW(Phdr))];
+	static char buf[64 << 10];
+	long pgsz = sysconf(_SC_PAGESIZE);
+	long long note_off = 0;
+	size_t hdrlen = 0;
+	ssize_t total = 0;
+	bool armed = false;
+
+	for (;;) {
+		ssize_t n = read(fd, buf, sizeof(buf));
+
+		if (n < 0) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+		if (n == 0)
+			break;
+		if (fd_out >= 0 && write(fd_out, buf, n) != n)
+			return -1;
+
+		if (hdrlen < sizeof(hdr)) {
+			size_t want = sizeof(hdr) - hdrlen;
+
+			if (want > (size_t)n)
+				want = (size_t)n;
+			memcpy(hdr + hdrlen, buf, want);
+			hdrlen += want;
+			if (hdrlen == sizeof(hdr))
+				note_off = note_offset(hdr);
+		}
+
+		total += n;
+
+		if (arm && !armed && note_off > 0 &&
+		    total > note_off + (long long)pgsz) {
+			if (pull_trigger())
+				return -1;
+			armed = true;
+			usleep(NOTIFY_SIGNAL_STALL_US);
+			continue;
+		}
+	}
+
+	if (arm && !armed)
+		return -1;
+
+	return total;
+}
+
+/*
+ * How large the dump was meant to be. The ELF header and the program
+ * headers are the first thing emitted, so even a truncated dump says how
+ * far it should have run: the end is max(p_offset + p_filesz).
+ *
+ * A hole at the end of the last segment is the one thing that never
+ * arrives. dump_skip() only accumulates cprm->to_skip, elf_core_dump()
+ * has no flush after the last dump_user_range() and a pipe cannot seek,
+ * so those bytes are dropped. On x86_64 the last segment is the gate vma
+ * and vsyscall=xonly leaves no page behind it, so a whole dump is one
+ * page short of what the headers promise. @tail returns the size of that
+ * last segment, which bounds what may go missing this way.
+ */
+long long coredump_expected_size(const char *path, long long *tail)
+{
+	ElfW(Phdr) *phdr = NULL;
+	long long expected = 0;
+	unsigned int nphdr, i;
+	ElfW(Ehdr) eh;
+	int fd;
+
+	*tail = 0;
+
+	fd = open(path, O_RDONLY | O_CLOEXEC);
+	if (fd < 0)
+		return -1;
+	if (read(fd, &eh, sizeof(eh)) != sizeof(eh))
+		goto err;
+	if (memcmp(eh.e_ident, ELFMAG, SELFMAG) || eh.e_type != ET_CORE)
+		goto err;
+	if (!eh.e_phnum || eh.e_phentsize != sizeof(*phdr))
+		goto err;
+
+	nphdr = eh.e_phnum;
+	phdr = calloc(nphdr, sizeof(*phdr));
+	if (!phdr)
+		goto err;
+	if (pread(fd, phdr, (size_t)nphdr * sizeof(*phdr), (off_t)eh.e_phoff) !=
+	    (ssize_t)((size_t)nphdr * sizeof(*phdr)))
+		goto err;
+
+	for (i = 0; i < nphdr; i++) {
+		long long end = (long long)phdr[i].p_offset +
+				(long long)phdr[i].p_filesz;
+		if (end > expected) {
+			expected = end;
+			*tail = (long long)phdr[i].p_filesz;
+		}
+	}
+
+	free(phdr);
+	close(fd);
+	return expected;
+err:
+	free(phdr);
+	close(fd);
+	return -1;
+}

-- 
2.53.0

Re: [PATCH 3/4] selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing cleanup of stale temporary files in FIXTURE_SETUP can bypass the polling synchronization in the notify_signal_pipe test, leading to false test successes.
- [Medium] The test suite uses predictable, hardcoded paths in /tmp/ and opens them with O_CREAT | O_TRUNC without O_EXCL, creating a vulnerability to symlink truncation attacks.
- [Medium] The test globally changes /proc/sys/kernel/core_pattern to point to a test-specific helper. If the test terminates abnormally, this configuration is never restored.
--

--- Patch [3]: [PATCH 3/4] selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump ---
commit c11ee8bbfd73149ce9b69d88e45aeab900d75512
Author: Christian Brauner <brauner@kernel.org>

selftests/coredump: test that TIF_NOTIFY_SIGNAL doesn't truncate a coredump

Add a test that verifies that a coredump cannot be cut short by
TIF_NOTIFY_SIGNAL through io_uring running task work for uninterruptible
tasks.

> diff --git a/tools/testing/selftests/coredump/coredump_notify_signal_helper.c b/tools/testing/selftests/coredump/coredump_notify_signal_helper.c
> new file mode 100644
> index 0000000000000..849f5c1ea7365
> --- /dev/null
> +++ b/tools/testing/selftests/coredump/coredump_notify_signal_helper.c
> @@ -0,0 +1,46 @@
[ ... ]
> +int main(int argc, char *argv[])
> +{
> +	int fd_core_file;
> +	ssize_t ret;
> +
> +	fd_core_file = open(NOTIFY_SIGNAL_CORE_TMPFILE,
> +			    O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0600);

[Severity: Medium]
Could this predictable hardcoded path in /tmp/ be vulnerable to a symlink
truncation attack? The helper is spawned by the kernel (typically as root)
and opens this path without O_EXCL. If an unprivileged user pre-creates this
path as a symlink, it might truncate important system files.

> +	if (fd_core_file < 0) {
> +		fprintf(stderr, "%s: open failed: %m\n", argv[0]);
> +		return EXIT_FAILURE;
> +	}
[ ... ]
> diff --git a/tools/testing/selftests/coredump/coredump_notify_signal_test.c b/tools/testing/selftests/coredump/coredump_notify_signal_test.c
> new file mode 100644
> index 0000000000000..e63c06a81562f
> --- /dev/null
> +++ b/tools/testing/selftests/coredump/coredump_notify_signal_test.c
> @@ -0,0 +1,245 @@
[ ... ]
> +FIXTURE_SETUP(coredump)
> +{
> +	FILE *file;
> +	int ret;
[ ... ]
> +	self->fd_tmpfs_detached = create_detached_tmpfs();
> +	ASSERT_GE(self->fd_tmpfs_detached, 0);
> +
> +	ret = fclose(file);
> +	ASSERT_EQ(0, ret);
> +
> +	unlink(NOTIFY_SIGNAL_TRIGGER);
> +	ASSERT_EQ(mkfifo(NOTIFY_SIGNAL_TRIGGER, 0600), 0);
> +}

[Severity: Medium]
Should FIXTURE_SETUP() also unlink NOTIFY_SIGNAL_CORE_FILE? If a previous
test run fails or is interrupted before teardown, a stale file could be left
in /tmp/. The polling loop in notify_signal_pipe() would immediately break
when it sees this stale file instead of waiting for the new coredump, leading
to a false test success.

[ ... ]
> +TEST_F(coredump, notify_signal_pipe)
> +{
> +	char pattern[PATH_MAX], helper[PATH_MAX], *p;
> +	struct stat st;
[ ... ]
> +	ASSERT_LT(snprintf(pattern, sizeof(pattern), "|%s", helper),
> +		  (int)sizeof(pattern));
> +	ASSERT_TRUE(set_core_pattern(pattern));

[Severity: Medium]
What happens if the test terminates abnormally (e.g. via SIGKILL) after
modifying core_pattern here? FIXTURE_TEARDOWN() would not run, leaving the
global /proc/sys/kernel/core_pattern permanently pointing to the test's
temporary helper binary. Could this break core dumps for the entire system?

[ ... ]
> diff --git a/tools/testing/selftests/coredump/coredump_test_helpers.c b/tools/testing/selftests/coredump/coredump_test_helpers.c
> index 2a20faf9cb0ad..20fcc5b644c32 100644
> --- a/tools/testing/selftests/coredump/coredump_test_helpers.c
> +++ b/tools/testing/selftests/coredump/coredump_test_helpers.c
> @@ -1,11 +1,18 @@
[ ... ]
> +static int make_file_mappings(void)
> +{
> +	long pgsz = sysconf(_SC_PAGESIZE);
> +	int fd, i;
> +
> +	fd = open(NOTIFY_SIGNAL_MAPFILE,
> +		  O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0600);

[Severity: Medium]
Is this another potential symlink truncation vulnerability? Like the helper
above, this opens a predictable, hardcoded path in /tmp/ with O_CREAT | O_TRUNC
but without O_EXCL.

> +	if (fd < 0)
> +		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-work-tif_notify_signal-v1-0-1ee1fcc5b3ff@kernel.org?part=3

[PATCH 4/4] smb: prevent TIF_NOTIFY_SIGNAL from interrupting

[Christian Brauner]

smb_send_kvec() blocks every signal during a send:

	sigfillset(&mask);
	sigprocmask(SIG_BLOCK, &mask, &oldmask);

An incomplete or partial sent would cause the connection to become out
of sync. That in turn would cause the session to be torn down and
reconnected.

In 00be6f26a2a7 ("smb: client: transport: avoid reconnects triggered
by pending task work") smb hand-rolled its own solution. Have it use the
new no_notify_signal_save() critical section instead.

This stops io_uring from cancelling a send in flight but that's just
like it is today and it is bounded by sk_sndtimeo at around 15s.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/smb/client/transport.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index fdf4e50c27ce..05dc20916c27 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -22,7 +22,6 @@
 #include <linux/mempool.h>
 #include <linux/sched/signal.h>
 #include <linux/task_io_accounting_ops.h>
-#include <linux/task_work.h>
 #include "cifsglob.h"
 #include "cifsproto.h"
 #include "cifs_debug.h"
@@ -142,6 +141,7 @@ int
 smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
 	      size_t *sent)
 {
+	unsigned int notify_flags;
 	int rc = 0;
 	int retries = 0;
 	struct socket *ssocket = server->ssocket;
@@ -172,15 +172,12 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
 		 * after the retries we will kill the socket and
 		 * reconnect which may clear the network problem.
 		 *
-		 * Even if regular signals are masked, EINTR might be
-		 * propagated from sk_stream_wait_memory() to here when
-		 * TIF_NOTIFY_SIGNAL is used for task work. For example,
-		 * certain io_uring completions will use that. Treat
-		 * having EINTR with pending task work the same as EAGAIN
-		 * to avoid unnecessary reconnects.
+		 * Task work must not abort the send, see signal_pending().
 		 */
+		notify_flags = no_notify_signal_save();
 		rc = sock_sendmsg(ssocket, smb_msg);
-		if (rc == -EAGAIN || unlikely(rc == -EINTR && task_work_pending(current))) {
+		no_notify_signal_restore(notify_flags);
+		if (rc == -EAGAIN) {
 			retries++;
 			if (retries >= 14 ||
 			    (!server->noblocksnd && (retries > 2))) {

-- 
2.53.0

Re: [PATCH 4/4] smb: prevent TIF_NOTIFY_SIGNAL from interrupting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Partial send of the RFC1002 marker is not accounted for in `total_len` on error, leading to missed connection teardown and TCP stream corruption.
--

--- Patch [4]: [PATCH 4/4] smb: prevent TIF_NOTIFY_SIGNAL from interrupting ---
commit 0a99d5c0436dadc5ec220717067bbc5bd7313e1f
Author: Christian Brauner <brauner@kernel.org>
smb: prevent TIF_NOTIFY_SIGNAL from interrupting

This commit replaces a hand-rolled signal masking solution in
smb_send_kvec() with the new no_notify_signal_save() critical section
to prevent io_uring from cancelling a send in flight.

> diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
> index fdf4e50c27ceb..05dc20916c276 100644
> --- a/fs/smb/client/transport.c
> +++ b/fs/smb/client/transport.c

[ ... ]

> @@ -172,15 +172,12 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
>  		 * after the retries we will kill the socket and
>  		 * reconnect which may clear the network problem.
>  		 *
> -		 * Even if regular signals are masked, EINTR might be
> -		 * propagated from sk_stream_wait_memory() to here when
> -		 * TIF_NOTIFY_SIGNAL is used for task work. For example,
> -		 * certain io_uring completions will use that. Treat
> -		 * having EINTR with pending task work the same as EAGAIN
> -		 * to avoid unnecessary reconnects.
> +		 * Task work must not abort the send, see signal_pending().
>  		 */
> +		notify_flags = no_notify_signal_save();
>  		rc = sock_sendmsg(ssocket, smb_msg);
> -		if (rc == -EAGAIN || unlikely(rc == -EINTR && task_work_pending(current))) {
> +		no_notify_signal_restore(notify_flags);
> +		if (rc == -EAGAIN) {

[Severity: High]
This is a pre-existing issue, but does returning -EAGAIN here cause a
protocol desync if we partially sent the RFC1002 marker?

Looking at __smb_send_rqst() which calls smb_send_kvec() to send the
4-byte marker:

fs/smb/client/transport.c:__smb_send_rqst() {
        ...
        rc = smb_send_kvec(server, &smb_msg, &sent);
        if (rc < 0)
                goto unmask;

        total_len += sent;
        ...
}

If smb_send_kvec() successfully sends 1 to 3 bytes of the marker, but
then blocks and eventually returns -EAGAIN due to the 15-second retry
timeout, the code jumps to unmask without updating total_len.

Later in the function, the reconnect logic checks total_len:

fs/smb/client/transport.c:__smb_send_rqst() {
        ...
        if ((total_len > 0) && (total_len != send_length)) {
                cifs_dbg(FYI, "partial send (wanted=%u sent=%zu): terminating session\n",
                         send_length, total_len);
                cifs_signal_cifsd_for_reconnect(server, false);
                ...
        }
}

Since total_len remains 0, this check fails and the connection is not
terminated. Will this leave stray bytes in the TCP stream and cause the
server to misinterpret all subsequent requests?

>  			retries++;
>  			if (retries >= 14 ||
>  			    (!server->noblocksnd && (retries > 2))) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-work-tif_notify_signal-v1-0-1ee1fcc5b3ff@kernel.org?part=4