[PATCH RFC v4 00/18] coredump, files: exit files on request

[PATCH RFC v4 00/18] coredump, files: exit files on request

[Christian Brauner]

We've had quite a few proposal for exiting files before generating the
coredump (see [1]-[8]). The various proposed solution were quite
unacceptable. So I took some time to look what a _remotely_ acceptable
version of this could look like. Here it is.

The gist is that we allow the coredump socket to request the
thread-group shed the various fdtables closed synchronously before
generating the coredump.

The intricate task is that this requires us to add a primitive to close
fdtables synchronously. Coredump is not enough to justify this but
performance numbers for this look pretty convincing. For tasks exiting
with a large file descriptor table this means getting rid of _a lot_ of
cmpxchg()s and hammering on task->pi_lock. The cost of this was
mentioned in various threads over the years. I found at least recent
comments by Oleg and Neil.

The obvious problematic case are kernel threads. fput() punts their
final __fput() to a workqueue. kthreads never return to userspace to run
task work and some of them may need to finish a umount and so cannot
->release() inline. No path we change is taken by a kthread. They don't
own a descriptor table and kthreadd is created with CLONE_FILES and
every kthread inherits it. So all of them share init_files and init_task
pins that forever. Their exit_files() never drops the last reference.

kernel_execve() refuses PF_KTHREAD outright. Usermodehelpers are spawned
without the kthread flag. close_range() is a syscall. The one indirect
way is a failing copy_process() calling exit_files() on a
child from a kthread parent. A dup_fd() in copy_process() cannot hold
the last reference to any of its files while the source table is alive.

Worker threads such as io_uring workers are user threads without
PF_KTHREAD that share the fdtable. One of them can be the last to put
the fdtable at exit. Today fput() hands their final __fput()s to task
work. The worker runs that itself in exit_task_work() a few lines later.
Now the same thread does the same work in exit_files() instead. vhost
workers are created without a descriptor table at all, so they never
even put an fdtable. Neither kind ever execs or calls close_range().

Now the performance numbers. I measured this on a 64 vCPU KVM guest (AMD
EPYC 9754) on top of vfs-7.4.coredump with and without the series and
the same x86_64_defconfig for both. The benchmark opens N files in a
forked worker, moves them to random positions in the descriptor table so
that the table order has nothing to do with the allocation order, and
times the teardown:

- process exit
- execve() with N close-on-exec descriptors
- close_range() with and without CLOSE_RANGE_UNSHARE

Here are the medians over two boots which agree within 2%. With
/dev/null as the file and 4096, 65536 and 1M descriptors:

                         4096 fds        65536 fds        1M fds
  ----------------------------------------------------------------------
  exit                   1045 -> 895 us  18.9 -> 14.9 ms  429 -> 306 ms
                          -14.4%         -21.2%           -28.7%
  ----------------------------------------------------------------------
  execve, close-on-exec  1393 -> 1190 us 21.4 -> 16.9 ms  471 -> 325 ms
                          -14.6%         -21.0%           -31.1%
  ----------------------------------------------------------------------
  close_range()          1179 -> 988 us  21.9 -> 17.4 ms  484 -> 337 ms
                          -16.1%         -20.6%           -30.3%
  ----------------------------------------------------------------------
  close_range(UNSHARE)   1118 -> 958 us  22.1 -> 17.8 ms  487 -> 333 ms
                          -14.3%         -19.8%           -31.7%

So that's a 16-21% win or 40-150 ns per closed file. This sheds
init_task_work() and task_work_add() per file with its cmpxchg() and the
TIF_NOTIFY_RESUME test-and-set, the list walk with the indirect call
in the path out, and then a second pass over every struct file that
isn't cache-hot anymore by then.

The performance boost holds from 256 to 1M descriptors and is largest
at 1M. Note the thread count doesn't matter since one thread does the
close anyway.

So with eight processes tearing down 65536 descriptors each at the same
time the win is 16-21% (34.9 -> 29.5 ms for exit, 34.4 -> 27.0 ms
for execve). The old code walks the table once to queue the task work
and then walks all 16 MB of struct file a second time from
task_work_run().

So once eight CPUs compete for the cache that second pass gets
very expensive. With 32 or 64 processes at once both kernels are bound
by pushing millions of files through slab and RCU. So then the
difference shrinks to 4-8%. Still though...

Files whose own release dominates the cleanup gain a little less. 13%
for eventfds, 11% for pipes, 6% for AF_UNIX sockets and 7% for distinct
tmpfs inodes. For them a release costs about 2 us per descriptor anyway.

Using function profiling a 65536 descriptor exit spends 4.6 ms in
exit_files() and 11.1 ms in task_work_run() before and now 13.7 ms in
exit_files() and nothing in task_work_run() after.

close(2) is untouched and measures the same. will-it-scale (open1,
open3, dup1, eventfd1, unix1, pipe1, signal1, processes and threads)
is within 2% either way. dup1_threads at 16 tasks and getppid1 at 64
tasks a few percent better which I'd put down to layout.

So, back to coredumps. With COREDUMP_CLOSE_FILES coredumps switch to an
empty file descriptor table before generating the coredump. That
requires a bit of synchronization but I think I got the basics down.

The coredumping thread allocates a new empty file descriptor table. It
then wakes all threads in the thread-group and tells them to switch to
the empty file descriptor table and get rid of the old one. They report
back once they're done.

So that handles most cases where locks would be held for an unreasonable
time until the coredump is generated but since files can be shared
between completely unrelated processes that's not a guarantee and we
can't give one. But it solves the reported issue without resorting to
even grosser hacks.

Link: https://lore.kernel.org/20260618030700.2511668-1-jackzxcui1989@163.com [1]
Link: https://lore.kernel.org/20260618150301.3226517-1-jackzxcui1989@163.com [2]
Link: https://lore.kernel.org/20260619122419.3954581-1-jackzxcui1989@163.com [3]
Link: https://lore.kernel.org/20260624145552.70143-1-jackzxcui1989@163.com [4]
Link: https://lore.kernel.org/20260630075604.52533-1-jackzxcui1989@163.com [5]
Link: https://lore.kernel.org/20260804001703.1340667-1-jackzxcui1989@163.com [6]
Link: https://lore.kernel.org/20260807040124.1706927-1-jackzxcui1989@163.com [7]
Link: https://lore.kernel.org/20260808052732.2589657-1-jackzxcui1989@163.com [8]

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Changes in v4:
- Fix performance number variations. It said variable within 4%. It's
  actually just 2%.
- Make various simplifications and add wait_var_event_state().
- Link to v3: https://patch.msgid.link/20260909-work-coredump-unlock-self-v3-0-04f907687563@kernel.org

Changes in v3:
- Drop the llist. This brings yet another performance boost.
- Link to v2: https://patch.msgid.link/20260902-work-coredump-unlock-self-v2-0-1bece368cbb1@kernel.org

Changes in v2:
- Revamp.
- Link to v1: https://patch.msgid.link/20260824-work-coredump-unlock-self-v1-0-48fd1cca7eda@kernel.org

---
Christian Brauner (18):
      fs: don't open-code file_close_fd() in close_fd()
      fs: add switch_files_struct()
      fs: move unshare_fd() to fs/file.c
      fs: remove unshare_files()
      fs: add filp_close_sync()
      fs: make close_files() synchronous
      fs: make close_range() synchronous
      fs: rename do_close_on_exec() to close_cloexec_files()
      fs: make close_cloexec_files() synchronous
      coredump: drop core_state->dumper
      sched: add wait_var_event_state()
      coredump: replace the startup completion with a thread count
      coredump: factor out coredump_wait_inactive()
      fs: add alloc_files_struct()
      coredump: add COREDUMP_CLOSE_FILES
      coredump: cancel io_uring requests before closing files
      tools: sync coredump.h header
      selftests/coredump: test COREDUMP_CLOSE_FILES

 fs/binfmt_elf.c                                    |   2 +-
 fs/binfmt_elf_fdpic.c                              |   2 +-
 fs/coredump.c                                      |  91 +++-
 fs/exec.c                                          |  13 +-
 fs/file.c                                          | 107 ++--
 fs/internal.h                                      |   1 +
 fs/open.c                                          |  17 +-
 include/linux/fdtable.h                            |   6 +-
 include/linux/sched/signal.h                       |   8 +-
 include/linux/wait_bit.h                           |  26 +
 include/uapi/linux/coredump.h                      |   8 +
 kernel/exit.c                                      |  22 +-
 kernel/fork.c                                      |  48 +-
 tools/include/uapi/linux/coredump.h                |   8 +
 tools/testing/selftests/coredump/Makefile          |   4 +-
 .../selftests/coredump/coredump_close_files_test.c | 592 +++++++++++++++++++++
 .../coredump/coredump_socket_protocol_test.c       |   6 +
 .../selftests/coredump/coredump_test_helpers.c     |   3 +-
 18 files changed, 840 insertions(+), 124 deletions(-)
---
base-commit: a5625efa7a0e77c26e857de147284a21d53fa3ad
change-id: 20260824-work-coredump-unlock-self-63a898912870

[PATCH RFC v4 01/18] fs: don't open-code file_close_fd() in close_fd()

[Christian Brauner]

close_fd() takes the lock, calls file_close_fd_locked() and drops the
lock, which is exactly what file_close_fd() does. Use it.

No functional changes.

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

diff --git a/fs/file.c b/fs/file.c
index 628ca07dc4b1..59673547de90 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -732,16 +732,13 @@ struct file *file_close_fd_locked(struct files_struct *files, unsigned fd)
 
 int close_fd(unsigned fd)
 {
-	struct files_struct *files = current->files;
 	struct file *file;
 
-	spin_lock(&files->file_lock);
-	file = file_close_fd_locked(files, fd);
-	spin_unlock(&files->file_lock);
+	file = file_close_fd(fd);
 	if (!file)
 		return -EBADF;
 
-	return filp_close(file, files);
+	return filp_close(file, current->files);
 }
 EXPORT_SYMBOL(close_fd);
 

-- 
2.53.0

[PATCH RFC v4 02/18] fs: add switch_files_struct()

[Christian Brauner]

Add switch_files_struct() to install another table on a task. It
consumes the reference to the new table and puts the old one. Convert
every place that switches a descriptor table except unshare_files().

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/file.c               | 23 +++++++++++------------
 include/linux/fdtable.h |  1 +
 kernel/fork.c           |  6 ++----
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 59673547de90..345011dad472 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -515,16 +515,18 @@ void put_files_struct(struct files_struct *files)
 	}
 }
 
-void exit_files(struct task_struct *tsk)
+/* Install @files on @tsk, consuming the reference, and put the old table. */
+void switch_files_struct(struct task_struct *tsk, struct files_struct *files)
 {
-	struct files_struct * files = tsk->files;
+	scoped_guard(task_lock, tsk)
+		swap(tsk->files, files);
+	put_files_struct(files);
+}
 
-	if (files) {
-		task_lock(tsk);
-		tsk->files = NULL;
-		task_unlock(tsk);
-		put_files_struct(files);
-	}
+void exit_files(struct task_struct *tsk)
+{
+	if (tsk->files)
+		switch_files_struct(tsk, NULL);
 }
 
 struct files_struct init_files = {
@@ -855,10 +857,7 @@ SYSCALL_DEFINE3(close_range, unsigned int, fd, unsigned int, max_fd,
 		 * We're done closing the files we were supposed to. Time to install
 		 * the new file descriptor table and drop the old one.
 		 */
-		task_lock(me);
-		me->files = cur_fds;
-		task_unlock(me);
-		put_files_struct(fds);
+		switch_files_struct(me, cur_fds);
 	}
 
 	return 0;
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index c45306a9f007..9614c6ecd477 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -100,6 +100,7 @@ static inline bool close_on_exec(unsigned int fd, const struct files_struct *fil
 struct task_struct;
 
 void put_files_struct(struct files_struct *fs);
+void switch_files_struct(struct task_struct *tsk, struct files_struct *files);
 int unshare_files(void);
 struct fd_range {
 	unsigned int from, to;
diff --git a/kernel/fork.c b/kernel/fork.c
index 416758c8a3d4..7c6fe82dbb9f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -3320,10 +3320,8 @@ int ksys_unshare(unsigned long unshare_flags)
 		if (new_fs)
 			new_fs = switch_fs_struct(new_fs);
 
-		if (new_fd) {
-			guard(task_lock)(current);
-			swap(current->files, new_fd);
-		}
+		if (new_fd)
+			switch_files_struct(current, no_free_ptr(new_fd));
 
 		if (new_cred) {
 			/* Install the new user namespace */

-- 
2.53.0

[PATCH RFC v4 03/18] fs: move unshare_fd() to fs/file.c

[Christian Brauner]

Move unshare_fd() where the rest of the descriptor table lifecycle
helpers live.

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/file.c               | 18 ++++++++++++++++++
 include/linux/fdtable.h |  1 +
 kernel/fork.c           | 18 ------------------
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 345011dad472..636a87e527b3 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -471,6 +471,24 @@ struct files_struct *dup_fd(struct files_struct *oldf, struct fd_range *punch_ho
 	return newf;
 }
 
+/*
+ * Unshare file descriptor table if it is being shared
+ */
+int unshare_fd(unsigned long unshare_flags, struct files_struct **new_fdp)
+{
+	struct files_struct *fd = current->files;
+
+	if ((unshare_flags & CLONE_FILES) &&
+	    (fd && atomic_read(&fd->count) > 1)) {
+		fd = dup_fd(fd, NULL);
+		if (IS_ERR(fd))
+			return PTR_ERR(fd);
+		*new_fdp = fd;
+	}
+
+	return 0;
+}
+
 static struct fdtable *close_files(struct files_struct * files)
 {
 	/*
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 9614c6ecd477..4ee1598848bb 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -102,6 +102,7 @@ struct task_struct;
 void put_files_struct(struct files_struct *fs);
 void switch_files_struct(struct task_struct *tsk, struct files_struct *files);
 int unshare_files(void);
+int unshare_fd(unsigned long unshare_flags, struct files_struct **new_fdp);
 struct fd_range {
 	unsigned int from, to;
 };
diff --git a/kernel/fork.c b/kernel/fork.c
index 7c6fe82dbb9f..fddba55bbc14 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -3207,24 +3207,6 @@ static int unshare_fs(unsigned long unshare_flags, struct fs_struct **new_fsp)
 	return 0;
 }
 
-/*
- * Unshare file descriptor table if it is being shared
- */
-static int unshare_fd(unsigned long unshare_flags, struct files_struct **new_fdp)
-{
-	struct files_struct *fd = current->files;
-
-	if ((unshare_flags & CLONE_FILES) &&
-	    (fd && atomic_read(&fd->count) > 1)) {
-		fd = dup_fd(fd, NULL);
-		if (IS_ERR(fd))
-			return PTR_ERR(fd);
-		*new_fdp = fd;
-	}
-
-	return 0;
-}
-
 /*
  * unshare allows a process to 'unshare' part of the process
  * context which was originally shared using clone.  copy_*

-- 
2.53.0

[PATCH RFC v4 04/18] fs: remove unshare_files()

[Christian Brauner]

exec is the only caller left since commit 433967cab51e ("coredump: stop
unsharing the file descriptor table"). All it does is call unshare_fd()
with CLONE_FILES and install the copy. Kill the pointless helper and
open-code it.

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/exec.c               |  5 ++++-
 include/linux/fdtable.h |  1 -
 kernel/fork.c           | 24 ------------------------
 3 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index d3081c8f7c10..977778f44cfc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1124,6 +1124,7 @@ static struct file *bprm_identity_file(const struct linux_binprm *bprm)
 int begin_new_exec(struct linux_binprm * bprm)
 {
 	struct task_struct *me = current;
+	struct files_struct *files = NULL;
 	int retval;
 
 	/* A pending PT_INTERP substitution this format cannot consume. */
@@ -1160,9 +1161,11 @@ int begin_new_exec(struct linux_binprm * bprm)
 	io_uring_task_cancel();
 
 	/* Ensure the files table is not shared. */
-	retval = unshare_files();
+	retval = unshare_fd(CLONE_FILES, &files);
 	if (retval)
 		goto out;
+	if (files)
+		switch_files_struct(me, files);
 
 	/*
 	 * We have to apply CLOEXEC before we change whether the process is
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 4ee1598848bb..666808a1caf5 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -101,7 +101,6 @@ struct task_struct;
 
 void put_files_struct(struct files_struct *fs);
 void switch_files_struct(struct task_struct *tsk, struct files_struct *files);
-int unshare_files(void);
 int unshare_fd(unsigned long unshare_flags, struct files_struct **new_fdp);
 struct fd_range {
 	unsigned int from, to;
diff --git a/kernel/fork.c b/kernel/fork.c
index fddba55bbc14..300cb46bc51e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -3336,30 +3336,6 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
 	return ksys_unshare(unshare_flags);
 }
 
-/*
- *	Helper to unshare the files of the current task.
- *	We don't want to expose copy_files internals to
- *	the exec layer of the kernel.
- */
-
-int unshare_files(void)
-{
-	struct task_struct *task = current;
-	struct files_struct *old, *copy = NULL;
-	int error;
-
-	error = unshare_fd(CLONE_FILES, &copy);
-	if (error || !copy)
-		return error;
-
-	old = task->files;
-	task_lock(task);
-	task->files = copy;
-	task_unlock(task);
-	put_files_struct(old);
-	return 0;
-}
-
 static int sysctl_max_threads(const struct ctl_table *table, int write,
 		       void *buffer, size_t *lenp, loff_t *ppos)
 {

-- 
2.53.0

[PATCH RFC v4 05/18] fs: add filp_close_sync()

[Christian Brauner]

Currently close() already does a synchronous release of the last
reference since the task is about to return to userspace and the
deferral through task work buys nothing. Add a filp_close_sync() helper.
We'll use that in the next patches.

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/internal.h |  1 +
 fs/open.c     | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index c658c8a5ebd5..8812de210d3f 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -198,6 +198,7 @@ extern struct file *do_file_open_root(const struct path *,
 extern struct open_how build_open_how(int flags, umode_t mode);
 extern int build_open_flags(const struct open_how *how, struct open_flags *op);
 struct file *file_close_fd_locked(struct files_struct *files, unsigned fd);
+int filp_close_sync(struct file *filp, fl_owner_t id);
 
 int do_ftruncate(struct file *file, loff_t length, unsigned int flags);
 int chmod_common(const struct path *path, umode_t mode);
diff --git a/fs/open.c b/fs/open.c
index 6b1c14e684a9..998e42ac319a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1537,6 +1537,19 @@ int filp_close(struct file *filp, fl_owner_t id)
 }
 EXPORT_SYMBOL(filp_close);
 
+/* Like filp_close() but the last reference is put right here. */
+int filp_close_sync(struct file *filp, fl_owner_t id)
+{
+	int retval;
+
+	/* Kernel threads must never put their final reference here. */
+	VFS_WARN_ON_ONCE(current->flags & PF_KTHREAD);
+	retval = filp_flush(filp, id);
+	fput_close_sync(filp);
+
+	return retval;
+}
+
 /*
  * Careful here! We test whether the file pointer is NULL before
  * releasing the fd. This ensures that one clone task can't release
@@ -1551,13 +1564,11 @@ SYSCALL_DEFINE1(close, unsigned int, fd)
 	if (!file)
 		return -EBADF;
 
-	retval = filp_flush(file, current->files);
-
 	/*
 	 * We're returning to user space. Don't bother
 	 * with any delayed fput() cases.
 	 */
-	fput_close_sync(file);
+	retval = filp_close_sync(file, current->files);
 
 	if (likely(retval == 0))
 		return 0;

-- 
2.53.0

[PATCH RFC v4 06/18] fs: make close_files() synchronous

[Christian Brauner]

When the last reference to a descriptor table is dropped close_files()
closes every file but punts the actual work to task work. For an exiting
task that task work only runs in exit_task_work().

Before commit 4a9d4b024a31 ("switch fput to task_work_add") fput() was
synchronous everywhere and exit released its files in exit_files().

The deferral made fput() safe from any context. And exit_files()
offloaded to task work as a side-effect. And that has downsides.

Oleg and Neil noticed that some time ago. A task that exits with a big
descriptor table ends up queueing a very large number of files on task
work. That leaves a list for any later task_work_cancel() to search
under ->pi_lock and costs a lot of atomics too.

Let close_files() close right away. Flush and put each file inline the
way close(2) does. The final __fput() runs during the table walk now
instead of from task_work_run() in exit_task_work(). One difference is
the order: task work ran the final __fput()s in reverse and now they run
in table order.

Every put of a dying table is synchronous now:

- exit_files()
- copy_process()
- close_range(CLOSE_RANGE_UNSHARE)
- unshare(2)
- exec

Kernel threads don't own a file descriptor table and exec already splats
were they to exec. kthreadd and every kthread share init_files and
init_task pins that forever.

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

diff --git a/fs/file.c b/fs/file.c
index 636a87e527b3..b2b466dce7fb 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -489,7 +489,7 @@ int unshare_fd(unsigned long unshare_flags, struct files_struct **new_fdp)
 	return 0;
 }
 
-static struct fdtable *close_files(struct files_struct * files)
+static struct fdtable *close_files(struct files_struct *files)
 {
 	/*
 	 * It is safe to dereference the fd table without RCU or
@@ -509,7 +509,7 @@ static struct fdtable *close_files(struct files_struct * files)
 			if (set & 1) {
 				struct file *file = fdt->fd[i];
 				if (file) {
-					filp_close(file, files);
+					filp_close_sync(file, files);
 					cond_resched();
 				}
 			}

-- 
2.53.0

[PATCH RFC v4 07/18] fs: make close_range() synchronous

[Christian Brauner]

__range_close() closes through filp_close() so every file the caller
held the last reference to is punted to task work. That costs one
cmpxchg per file plus a list entry for any later task_work_cancel() to
search under ->pi_lock. close_range(2) exists to close many descriptors
in one go fast. So convert it to the same synchronous treatment as
close(2) and close_files().

Flush and put each file inline while ->file_lock is dropped.
close_range(2) now behaves like close(2).

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

diff --git a/fs/file.c b/fs/file.c
index b2b466dce7fb..178c8cb9da09 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -807,7 +807,7 @@ static inline void __range_close(struct files_struct *files, unsigned int fd,
 		file = file_close_fd_locked(files, fd);
 		if (file) {
 			spin_unlock(&files->file_lock);
-			filp_close(file, files);
+			filp_close_sync(file, files);
 			cond_resched();
 			spin_lock(&files->file_lock);
 			fdt = files_fdtable(files);

-- 
2.53.0

[PATCH RFC v4 08/18] fs: rename do_close_on_exec() to close_cloexec_files()

[Christian Brauner]

Rename the helper and align it with close_files().

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/exec.c               | 2 +-
 fs/file.c               | 2 +-
 include/linux/fdtable.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 977778f44cfc..1d9163155d16 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1179,7 +1179,7 @@ int begin_new_exec(struct linux_binprm * bprm)
 	 * This must happen after the point of no return, and after unsharing
 	 * the FD table.
 	 */
-	do_close_on_exec(me->files);
+	close_cloexec_files(me->files);
 
 	/*
 	 * Must be called _before_ exec_mmap() as bprm->mm is
diff --git a/fs/file.c b/fs/file.c
index 178c8cb9da09..b0490566719c 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -901,7 +901,7 @@ struct file *file_close_fd(unsigned int fd)
 	return file;
 }
 
-void do_close_on_exec(struct files_struct *files)
+void close_cloexec_files(struct files_struct *files)
 {
 	unsigned i;
 	struct fdtable *fdt;
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 666808a1caf5..2965acd120bc 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -106,7 +106,7 @@ struct fd_range {
 	unsigned int from, to;
 };
 struct files_struct *dup_fd(struct files_struct *, struct fd_range *) __latent_entropy;
-void do_close_on_exec(struct files_struct *);
+void close_cloexec_files(struct files_struct *);
 int iterate_fd(struct files_struct *, unsigned,
 		int (*)(const void *, struct file *, unsigned),
 		const void *);

-- 
2.53.0

[PATCH RFC v4 09/18] fs: make close_cloexec_files() synchronous

[Christian Brauner]

Punting file closing to task work during exec slows down exec
significantly when its done with a bunch of file descriptors. We can
do this in-band instead. Flush already runs synchronous. Jann moved
close-on-exec in e780259b54e6 ("exec: do_close_on_exec() before taking
exec_update_lock") outside of exec_update_lock.

The only lock that's still held now is cred_guard_mutex. It's deprecated
and has five takers

(1) exec
(2) ptrace_attach()
(3) seccomp() with SECCOMP_FILTER_FLAG_TSYNC
(4) writes to /proc/<pid>/attr/*
(5) lsm_set_self_attr()

Four of them take the task's own cred_guard_mutex. When
close_cloexec_files() runs, de_thread() ensured that the calling task is
the only one alive in its thread-group. That leaves ptrace() waiting on
cred_guard_mutex of the tracee going through exec. exec already sleeps
under cred_guard_mutex in de_thread() when it reads binary and
interpreter. So while we add wait-time to an attaching ptracer no new
lock dependency is added. vfork() als waits but that's a dup_fd() copy
of the fdtable and rarely holds the last reference. If that's an issue
we can always change that later.

Link: https://lore.kernel.org/CAGudoHEsGP1P+sAWaw_tbh1NesJhSeww8869uzmaqtgk8F43=Q@mail.gmail.com
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/exec.c | 6 +++---
 fs/file.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 1d9163155d16..075a744421e1 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1173,9 +1173,9 @@ int begin_new_exec(struct linux_binprm * bprm)
 	 * trying to access the should-be-closed file descriptors of a process
 	 * undergoing exec(2).
 	 *
-	 * This can block on filesystem ->flush() handlers, including waiting
-	 * for FUSE daemons, so do it before exec_mmap takes the
-	 * exec_update_lock.
+	 * This can block on filesystem ->flush() and ->release() handlers,
+	 * including waiting for FUSE daemons, so do it before exec_mmap
+	 * takes the exec_update_lock.
 	 * This must happen after the point of no return, and after unsharing
 	 * the FD table.
 	 */
diff --git a/fs/file.c b/fs/file.c
index b0490566719c..76e328edf630 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -928,7 +928,7 @@ void close_cloexec_files(struct files_struct *files)
 			rcu_assign_pointer(fdt->fd[fd], NULL);
 			__put_unused_fd(files, fd);
 			spin_unlock(&files->file_lock);
-			filp_close(file, files);
+			filp_close_sync(file, files);
 			cond_resched();
 			spin_lock(&files->file_lock);
 		}

-- 
2.53.0

[PATCH RFC v4 10/18] coredump: drop core_state->dumper

[Christian Brauner]

The core_state->dumper field isn't used anymore. Only its ->next pointer
is. The current task is always the dumping thread and the ->task pointer
is never read. Replace it with a plain pointer to the list of parked
threads.

Historically, core_state->dumper was used. Its ->task pointer was read.
by fill_note_info() started at &core_state->dumper to ensure that the
dumping thread came first in the ELF thread notes. That changed in
commit 4b0e21d64253 ("[elf][regset] simplify thread list handling in
fill_note_info()"). The first iteration was taken out of the loop. So
it's been unused ever since.

No functional changes.

Suggested-by: NeilBrown <neilb@ownmail.net>
Link: https://lore.kernel.org/178900159210.207413.8292125177519817528@noble.neil.brown.name
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/binfmt_elf.c              | 2 +-
 fs/binfmt_elf_fdpic.c        | 2 +-
 fs/coredump.c                | 7 +++----
 include/linux/sched/signal.h | 2 +-
 kernel/exit.c                | 4 ++--
 5 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 6b7ffac5d665..bf7f8f47548d 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1875,7 +1875,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 		return 0;
 
 	info->thread->task = dump_task;
-	for (ct = dump_task->signal->core_state->dumper.next; ct; ct = ct->next) {
+	for (ct = dump_task->signal->core_state->tasks; ct; ct = ct->next) {
 		t = kzalloc_flex(*t, notes, info->thread_notes);
 		if (unlikely(!t))
 			return 0;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 005f0a084483..d3872169f55e 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1504,7 +1504,7 @@ static bool elf_fdpic_core_dump(struct coredump_params *cprm)
 	if (!psinfo)
 		goto end_coredump;
 
-	for (ct = current->signal->core_state->dumper.next;
+	for (ct = current->signal->core_state->tasks;
 					ct; ct = ct->next) {
 		tmp = elf_dump_thread_status(cprm->siginfo->si_signo,
 					     ct->task, &thread_status_size);
diff --git a/fs/coredump.c b/fs/coredump.c
index 9addd2d59b7b..16d3fb54500b 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -524,8 +524,7 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
 	int core_waiters = -EBUSY;
 
 	init_completion(&core_state->startup);
-	core_state->dumper.task = tsk;
-	core_state->dumper.next = NULL;
+	core_state->tasks = NULL;
 
 	core_waiters = zap_threads(tsk, core_state, exit_code);
 	if (core_waiters > 0) {
@@ -538,7 +537,7 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
 		 * all the thread context (extended register state, like
 		 * fpu etc) gets copied to the memory.
 		 */
-		ptr = core_state->dumper.next;
+		ptr = core_state->tasks;
 		while (ptr != NULL) {
 			wait_task_inactive(ptr->task, TASK_ANY);
 			ptr = ptr->next;
@@ -556,7 +555,7 @@ static void coredump_finish(enum coredump_state state)
 	spin_lock_irq(&current->sighand->siglock);
 	if ((state & COREDUMP_STATE_STARTED) && !__fatal_signal_pending(current))
 		current->signal->group_exit_code |= 0x80;
-	next = current->signal->core_state->dumper.next;
+	next = current->signal->core_state->tasks;
 	current->signal->core_state = NULL;
 	spin_unlock_irq(&current->sighand->siglock);
 
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index d45a5476b97d..14b55d00d605 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -80,7 +80,7 @@ struct core_thread {
 
 struct core_state {
 	atomic_t nr_threads;
-	struct core_thread dumper;
+	struct core_thread *tasks;
 	struct completion startup;
 };
 
diff --git a/kernel/exit.c b/kernel/exit.c
index 4e028f157597..3df1fffc6674 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -435,12 +435,12 @@ static void coredump_task_exit(struct task_struct *tsk,
 
 	self.task = tsk;
 	if (self.task->flags & PF_SIGNALED)
-		self.next = xchg(&core_state->dumper.next, &self);
+		self.next = xchg(&core_state->tasks, &self);
 	else
 		self.task = NULL;
 	/*
 	 * Implies mb(), the result of xchg() must be visible
-	 * to core_state->dumper.
+	 * to the dumper.
 	 */
 	if (atomic_dec_and_test(&core_state->nr_threads))
 		complete(&core_state->startup);

-- 
2.53.0

[PATCH RFC v4 11/18] sched: add wait_var_event_state()

[Christian Brauner]

All wait_var_event() sleep in a fixed task state. For coredumps we need
a variant that takes the state from the caller the way
wait_event_state() does. This allows us to continue sleeping with
TASK_FREEZABLE. That's certainly also a useful addition for other places.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 include/linux/wait_bit.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h
index 553d7b23e3ad..af077ed4caf6 100644
--- a/include/linux/wait_bit.h
+++ b/include/linux/wait_bit.h
@@ -432,6 +432,32 @@ do {									\
 	__ret;								\
 })
 
+/**
+ * wait_var_event_state - wait for a variable to be updated and notified
+ * @var: the address of variable being waited on
+ * @condition: the condition to wait for
+ * @state: the task state to sleep in, %TASK_UNINTERRUPTIBLE etc.
+ *
+ * Wait for a @condition to be true, only re-checking when a wake up is
+ * received for the given @var (an arbitrary kernel address which need
+ * not be directly related to the given condition, but usually is).
+ *
+ * Returns 0 if the condition became true, or %-ERESTARTSYS if a signal
+ * arrived which @state allows to interrupt.
+ *
+ * The condition should normally use smp_load_acquire() or a similarly
+ * ordered access to ensure that any changes to memory made before the
+ * condition became true will be visible after the wait completes.
+ */
+#define wait_var_event_state(var, condition, state)					\
+({											\
+	int __ret = 0;									\
+	might_sleep();									\
+	if (!(condition))								\
+		__ret = ___wait_var_event(var, condition, (state), 0, 0, schedule());	\
+	__ret;										\
+})
+
 /**
  * wait_var_event_any_lock - wait for a variable to be updated under a lock
  * @var: the address of the variable being waited on

-- 
2.53.0

[PATCH RFC v4 12/18] coredump: replace the startup completion with a thread count

[Christian Brauner]

coredump_wait() sets core_state->nr_threads to the number of tasks
killed and waits for the last thread to enter coredump_task_exit() to
signal completion. Let's just wait on the count directly. The exiting
tasks can use atomic_dec_and_wake_up() and the dumping task sleeps in
wait_var_event_state().

The dumping task must remain freezable since commit f5d39b020809
("freezer,sched: Rewrite core freezer logic"). So keep the wait
TASK_UNINTERRUPTIBLE|TASK_FREEZABLE.

Drop the completion and rename nr_threads to threads_remaining.

No functional changes.

Suggested-by: NeilBrown <neilb@ownmail.net>
Link: https://lore.kernel.org/178899497961.207413.10554121774377911612@noble.neil.brown.name
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/coredump.c                | 9 +++++----
 include/linux/sched/signal.h | 4 ++--
 kernel/exit.c                | 6 +++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 16d3fb54500b..2e2005c14d93 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -39,6 +39,7 @@
 #include <linux/oom.h>
 #include <linux/compat.h>
 #include <linux/fs.h>
+#include <linux/wait_bit.h>
 #include <linux/path.h>
 #include <linux/timekeeping.h>
 #include <linux/sysctl.h>
@@ -512,7 +513,7 @@ static int zap_threads(struct task_struct *tsk,
 		nr = zap_process(signal, exit_code);
 		clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
 		tsk->flags |= PF_DUMPCORE;
-		atomic_set(&core_state->nr_threads, nr);
+		atomic_set(&core_state->threads_remaining, nr);
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
 	return nr;
@@ -523,15 +524,15 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
 	struct task_struct *tsk = current;
 	int core_waiters = -EBUSY;
 
-	init_completion(&core_state->startup);
 	core_state->tasks = NULL;
 
 	core_waiters = zap_threads(tsk, core_state, exit_code);
 	if (core_waiters > 0) {
 		struct core_thread *ptr;
 
-		wait_for_completion_state(&core_state->startup,
-					  TASK_UNINTERRUPTIBLE|TASK_FREEZABLE);
+		wait_var_event_state(&core_state->threads_remaining,
+				     !atomic_read_acquire(&core_state->threads_remaining),
+				     TASK_UNINTERRUPTIBLE|TASK_FREEZABLE);
 		/*
 		 * Wait for all the threads to become inactive, so that
 		 * all the thread context (extended register state, like
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 14b55d00d605..e039e29cd8c5 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -79,9 +79,9 @@ struct core_thread {
 };
 
 struct core_state {
-	atomic_t nr_threads;
+	/* Threads the dumper still waits for. */
+	atomic_t threads_remaining;
 	struct core_thread *tasks;
-	struct completion startup;
 };
 
 /*
diff --git a/kernel/exit.c b/kernel/exit.c
index 3df1fffc6674..55dbea3b242e 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/capability.h>
 #include <linux/completion.h>
+#include <linux/wait_bit.h>
 #include <linux/personality.h>
 #include <linux/tty.h>
 #include <linux/iocontext.h>
@@ -442,8 +443,7 @@ static void coredump_task_exit(struct task_struct *tsk,
 	 * Implies mb(), the result of xchg() must be visible
 	 * to the dumper.
 	 */
-	if (atomic_dec_and_test(&core_state->nr_threads))
-		complete(&core_state->startup);
+	atomic_dec_and_wake_up(&core_state->threads_remaining);
 
 	for (;;) {
 		set_current_state(TASK_IDLE|TASK_FREEZABLE);
@@ -917,7 +917,7 @@ static void synchronize_group_exit(struct task_struct *tsk, long code)
 	 * Serialize with any possible pending coredump.
 	 * We must hold siglock around checking core_state
 	 * and setting PF_POSTCOREDUMP.  The core-inducing thread
-	 * will increment ->nr_threads for each thread in the
+	 * will increment ->threads_remaining for each thread in the
 	 * group without PF_POSTCOREDUMP set.
 	 */
 	tsk->flags |= PF_POSTCOREDUMP;

-- 
2.53.0

[PATCH RFC v4 13/18] coredump: factor out coredump_wait_inactive()

[Christian Brauner]

Factor out a new coredump_wait_inactive() helper that
COREDUMP_CLOSE_FILES can consume in a bit.

No functional changes.

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

diff --git a/fs/coredump.c b/fs/coredump.c
index 2e2005c14d93..daeca723bdae 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -519,6 +519,22 @@ static int zap_threads(struct task_struct *tsk,
 	return nr;
 }
 
+static void coredump_wait_inactive(struct core_state *core_state)
+{
+	struct core_thread *ptr;
+
+	wait_var_event_state(&core_state->threads_remaining,
+			     !atomic_read_acquire(&core_state->threads_remaining),
+			     TASK_UNINTERRUPTIBLE | TASK_FREEZABLE);
+	/*
+	 * Wait for all the threads to become inactive, so that
+	 * all the thread context (extended register state, like
+	 * fpu etc) gets copied to the memory.
+	 */
+	for (ptr = core_state->tasks; ptr; ptr = ptr->next)
+		wait_task_inactive(ptr->task, TASK_ANY);
+}
+
 static int coredump_wait(int exit_code, struct core_state *core_state)
 {
 	struct task_struct *tsk = current;
@@ -527,23 +543,8 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
 	core_state->tasks = NULL;
 
 	core_waiters = zap_threads(tsk, core_state, exit_code);
-	if (core_waiters > 0) {
-		struct core_thread *ptr;
-
-		wait_var_event_state(&core_state->threads_remaining,
-				     !atomic_read_acquire(&core_state->threads_remaining),
-				     TASK_UNINTERRUPTIBLE|TASK_FREEZABLE);
-		/*
-		 * Wait for all the threads to become inactive, so that
-		 * all the thread context (extended register state, like
-		 * fpu etc) gets copied to the memory.
-		 */
-		ptr = core_state->tasks;
-		while (ptr != NULL) {
-			wait_task_inactive(ptr->task, TASK_ANY);
-			ptr = ptr->next;
-		}
-	}
+	if (core_waiters > 0)
+		coredump_wait_inactive(core_state);
 
 	return core_waiters;
 }

-- 
2.53.0

[PATCH RFC v4 14/18] fs: add alloc_files_struct()

[Christian Brauner]

Add a way to get an empty descriptor table with one reference. Let
dup_fd() share the allocation. The coredump code will use it too in a bit.

No functional changes.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/file.c               | 49 ++++++++++++++++++++++++++++++++++++++-----------
 include/linux/fdtable.h |  1 +
 2 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 76e328edf630..8fa58500b6d1 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -375,21 +375,15 @@ static unsigned int sane_fdtable_size(struct fdtable *fdt, struct fd_range *punc
 	return ALIGN(last + 1, BITS_PER_LONG);
 }
 
-/*
- * Allocate a new descriptor table and copy contents from the passed in
- * instance.  Returns a pointer to cloned table on success, ERR_PTR()
- * on failure.  For 'punch_hole' see sane_fdtable_size().
- */
-struct files_struct *dup_fd(struct files_struct *oldf, struct fd_range *punch_hole)
+/* A table with one reference and the embedded fdtable, nothing copied yet. */
+static struct files_struct *alloc_files(gfp_t gfp)
 {
 	struct files_struct *newf;
-	struct file **old_fds, **new_fds;
-	unsigned int open_files, i;
-	struct fdtable *old_fdt, *new_fdt;
+	struct fdtable *new_fdt;
 
-	newf = kmem_cache_alloc(files_cachep, GFP_KERNEL);
+	newf = kmem_cache_alloc(files_cachep, gfp);
 	if (!newf)
-		return ERR_PTR(-ENOMEM);
+		return NULL;
 
 	atomic_set(&newf->count, 1);
 
@@ -404,6 +398,39 @@ struct files_struct *dup_fd(struct files_struct *oldf, struct fd_range *punch_ho
 	new_fdt->full_fds_bits = newf->full_fds_bits_init;
 	new_fdt->fd = &newf->fd_array[0];
 
+	return newf;
+}
+
+/* An empty descriptor table with one reference. */
+struct files_struct *alloc_files_struct(void)
+{
+	struct files_struct *newf;
+
+	newf = alloc_files(GFP_KERNEL | __GFP_ZERO);
+	if (!newf)
+		return NULL;
+
+	rcu_assign_pointer(newf->fdt, &newf->fdtab);
+	return newf;
+}
+
+/*
+ * Allocate a new descriptor table and copy contents from the passed in
+ * instance.  Returns a pointer to cloned table on success, ERR_PTR()
+ * on failure.  For 'punch_hole' see sane_fdtable_size().
+ */
+struct files_struct *dup_fd(struct files_struct *oldf, struct fd_range *punch_hole)
+{
+	struct files_struct *newf;
+	struct file **old_fds, **new_fds;
+	unsigned int open_files, i;
+	struct fdtable *old_fdt, *new_fdt;
+
+	newf = alloc_files(GFP_KERNEL);
+	if (!newf)
+		return ERR_PTR(-ENOMEM);
+	new_fdt = &newf->fdtab;
+
 	spin_lock(&oldf->file_lock);
 	old_fdt = files_fdtable(oldf);
 	open_files = sane_fdtable_size(old_fdt, punch_hole);
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 2965acd120bc..155ad783f2ce 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -100,6 +100,7 @@ static inline bool close_on_exec(unsigned int fd, const struct files_struct *fil
 struct task_struct;
 
 void put_files_struct(struct files_struct *fs);
+struct files_struct *alloc_files_struct(void);
 void switch_files_struct(struct task_struct *tsk, struct files_struct *files);
 int unshare_fd(unsigned long unshare_flags, struct files_struct **new_fdp);
 struct fd_range {

-- 
2.53.0

[PATCH RFC v4 15/18] coredump: add COREDUMP_CLOSE_FILES

[Christian Brauner]

Add COREDUMP_CLOSE_FILES and allow a coredump server to request that the
thread-group closes all files before creating the coredump.

There have been several attempts to let the dumping process decide
through a new fcntl() flag, a new procfs file or a new coredump_filter
bit that its descriptors go away early. That's just broken imho.

Tools like systemd-coredump walk /proc/<pid>/fd and /proc/<pid>/fdinfo
and some use pidfd_getfd() to preserve files of the crashing process.
Only the coredump server knows whether it still needs the descriptors.

So let the coredump server ask for it. Add a new COREDUMP_CLOSE_FILES
feature bit. If the coredump server raises it the kernel drops the
descriptor tables of the thread group right after the handshake and
before it generates the coredump.

COREDUMP_CLOSE_FILES doesn't work with COREDUMP_REJECT. A rejected task
exits and closes everything right away anyway.

We switch to an empty fdtable instead of simply clearing because
io_uring_files_cancel() runs task work in do_exit() before
exit_signals() sets PF_EXITING and may reissue requests that dereference
current->files.

The files are closed like close(2) would, PF_EXITING isn't set yet so
SO_LINGER sockets linger.

Reported-by: Xin Zhao <jackzxcui1989@163.com>
Link: https://lore.kernel.org/20260618030700.2511668-1-jackzxcui1989@163.com
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/coredump.c                 | 47 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/sched/signal.h  |  2 ++
 include/uapi/linux/coredump.h |  8 ++++++++
 kernel/exit.c                 | 11 ++++++++--
 4 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index daeca723bdae..4ffd801aca01 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -549,6 +549,40 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
 	return core_waiters;
 }
 
+/*
+ * Allocate a new empty fdtable and switch the whole thread-group to it.
+ * Put all the old fdtables freeing up resources and locks before writing the
+ * coredump.
+ */
+static bool coredump_close_files(struct core_state *core_state)
+{
+	struct files_struct *files;
+	struct core_thread *ct;
+
+	files = alloc_files_struct();
+	if (!files)
+		return false;
+
+	for (ct = core_state->tasks; ct; ct = ct->next) {
+		/* Tasks without a table such as vhost workers can be skipped. */
+		if (!ct->task->files)
+			continue;
+		atomic_inc(&core_state->threads_remaining);
+		/* ct->files holds a reference until the thread switches to it. */
+		atomic_inc(&files->count);
+		/* Pairs with the acquire in coredump_task_exit(). */
+		smp_store_release(&ct->files, files);
+		wake_up_process(ct->task);
+	}
+
+	/* Use the dumper's real creds not the overridden ones. */
+	scoped_with_creds(current_real_cred())
+		switch_files_struct(current, files);
+
+	coredump_wait_inactive(core_state);
+	return true;
+}
+
 static void coredump_finish(enum coredump_state state)
 {
 	struct core_thread *curr, *next;
@@ -838,7 +872,8 @@ static bool coredump_sock_request(struct core_name *cn, struct coredump_params *
 		.mask			= COREDUMP_KERNEL | COREDUMP_USERSPACE |
 					  COREDUMP_REJECT | COREDUMP_WAIT |
 					  COREDUMP_RECORDS | COREDUMP_SPARSE |
-					  COREDUMP_MEMORY_TYPES,
+					  COREDUMP_MEMORY_TYPES |
+					  COREDUMP_CLOSE_FILES,
 		.size_ack		= sizeof(struct coredump_ack),
 		.memory_types		= cprm->memory_types,
 		.memory_types_mask	= COREDUMP_MEMORY_ALL,
@@ -906,6 +941,12 @@ static bool coredump_sock_request(struct core_name *cn, struct coredump_params *
 		return false;
 	}
 
+	/* A rejected task exits right away and closes everything anyway. */
+	if ((ack.mask & COREDUMP_CLOSE_FILES) && (ack.mask & COREDUMP_REJECT)) {
+		coredump_sock_mark(cprm->file, COREDUMP_MARK_CONFLICTING);
+		return false;
+	}
+
 	if (ack.mask & COREDUMP_MEMORY_TYPES) {
 		/* The memory types need the whole field. */
 		if (usize < COREDUMP_ACK_SIZE_VER1) {
@@ -1221,6 +1262,10 @@ static void do_coredump(struct core_name *cn, struct coredump_params *cprm,
 	if (cprm->mask & COREDUMP_REJECT)
 		return;
 
+	if ((cprm->mask & COREDUMP_CLOSE_FILES) &&
+	    !coredump_close_files(current->signal->core_state))
+		return;
+
 	if ((cprm->mask & COREDUMP_KERNEL) && !coredump_write(cprm, binfmt))
 		return;
 
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index e039e29cd8c5..70067ccfe2ba 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -76,6 +76,8 @@ struct multiprocess_signals {
 struct core_thread {
 	struct task_struct *task;
 	struct core_thread *next;
+	/* The empty table to switch to, published by the dumping thread. */
+	struct files_struct *files;
 };
 
 struct core_state {
diff --git a/include/uapi/linux/coredump.h b/include/uapi/linux/coredump.h
index 6d0c53b534ea..ec09d7ab0131 100644
--- a/include/uapi/linux/coredump.h
+++ b/include/uapi/linux/coredump.h
@@ -19,6 +19,9 @@
  * @COREDUMP_MEMORY_TYPES: dump the memory types in
  *                          coredump_ack->memory_types instead of the ones
  *                          the task selected; requires COREDUMP_KERNEL
+ * @COREDUMP_CLOSE_FILES: close all file descriptors of the task before the
+ *                        coredump is generated; incompatible with
+ *                        COREDUMP_REJECT
  */
 enum {
 	COREDUMP_KERNEL		= (1ULL << 0),
@@ -28,6 +31,7 @@ enum {
 	COREDUMP_RECORDS	= (1ULL << 4),
 	COREDUMP_SPARSE		= (1ULL << 5),
 	COREDUMP_MEMORY_TYPES	= (1ULL << 6),
+	COREDUMP_CLOSE_FILES	= (1ULL << 7),
 };
 
 /**
@@ -137,6 +141,10 @@ enum {
  * Note that @memory_types must be zero if COREDUMP_MEMORY_TYPES isn't
  * raised. COREDUMP_MEMORY_TYPES requires COREDUMP_KERNEL and an ack of
  * at least COREDUMP_ACK_SIZE_VER1 bytes.
+ *
+ * If COREDUMP_CLOSE_FILES is raised in @mask the kernel closes the file
+ * descriptors of the coredumping task before it generates the coredump.
+ * The task ends up with an empty descriptor table.
  */
 struct coredump_ack {
 	__u32 size;
diff --git a/kernel/exit.c b/kernel/exit.c
index 55dbea3b242e..04f2c8c78879 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -26,6 +26,7 @@
 #include <linux/acct.h>
 #include <linux/tsacct_kern.h>
 #include <linux/file.h>
+#include <linux/fdtable.h>
 #include <linux/freezer.h>
 #include <linux/binfmts.h>
 #include <linux/nsproxy.h>
@@ -432,9 +433,8 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
 static void coredump_task_exit(struct task_struct *tsk,
 			       struct core_state *core_state)
 {
-	struct core_thread self;
+	struct core_thread self = { .task = tsk };
 
-	self.task = tsk;
 	if (self.task->flags & PF_SIGNALED)
 		self.next = xchg(&core_state->tasks, &self);
 	else
@@ -449,6 +449,13 @@ static void coredump_task_exit(struct task_struct *tsk,
 		set_current_state(TASK_IDLE|TASK_FREEZABLE);
 		if (!self.task) /* see coredump_finish() */
 			break;
+		/* Pairs with the release in coredump_close_files(). */
+		if (smp_load_acquire(&self.files)) {
+			__set_current_state(TASK_RUNNING);
+			switch_files_struct(tsk, no_free_ptr(self.files));
+			atomic_dec_and_wake_up(&core_state->threads_remaining);
+			continue;
+		}
 		schedule();
 	}
 	__set_current_state(TASK_RUNNING);

-- 
2.53.0

[PATCH RFC v4 16/18] coredump: cancel io_uring requests before closing files

[Christian Brauner]

COREDUMP_CLOSE_FILES allows userspace to request to drop the descriptor
tables of the thread-group before the coredump is written. io_uring
requests the threads had in flight survive and say a poll-triggered
accept can install a descriptor into whatever table the thread has at
that point.

Since the whole thread-group is going down in a coredump do what exec
does after de_thread() and cancel everything. After that no io_uring
task work can be queued to the thread anymore.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/coredump.c | 5 ++++-
 kernel/exit.c | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 4ffd801aca01..87befc5df961 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -40,6 +40,7 @@
 #include <linux/compat.h>
 #include <linux/fs.h>
 #include <linux/wait_bit.h>
+#include <linux/io_uring.h>
 #include <linux/path.h>
 #include <linux/timekeeping.h>
 #include <linux/sysctl.h>
@@ -576,8 +577,10 @@ static bool coredump_close_files(struct core_state *core_state)
 	}
 
 	/* Use the dumper's real creds not the overridden ones. */
-	scoped_with_creds(current_real_cred())
+	scoped_with_creds(current_real_cred()) {
+		io_uring_task_cancel();
 		switch_files_struct(current, files);
+	}
 
 	coredump_wait_inactive(core_state);
 	return true;
diff --git a/kernel/exit.c b/kernel/exit.c
index 04f2c8c78879..4c94a475b958 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -452,6 +452,7 @@ static void coredump_task_exit(struct task_struct *tsk,
 		/* Pairs with the release in coredump_close_files(). */
 		if (smp_load_acquire(&self.files)) {
 			__set_current_state(TASK_RUNNING);
+			io_uring_task_cancel();
 			switch_files_struct(tsk, no_free_ptr(self.files));
 			atomic_dec_and_wake_up(&core_state->threads_remaining);
 			continue;

-- 
2.53.0

[PATCH RFC v4 17/18] tools: sync coredump.h header

[Christian Brauner]

Sync the headers for the selftests.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 tools/include/uapi/linux/coredump.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/include/uapi/linux/coredump.h b/tools/include/uapi/linux/coredump.h
index 6d0c53b534ea..ec09d7ab0131 100644
--- a/tools/include/uapi/linux/coredump.h
+++ b/tools/include/uapi/linux/coredump.h
@@ -19,6 +19,9 @@
  * @COREDUMP_MEMORY_TYPES: dump the memory types in
  *                          coredump_ack->memory_types instead of the ones
  *                          the task selected; requires COREDUMP_KERNEL
+ * @COREDUMP_CLOSE_FILES: close all file descriptors of the task before the
+ *                        coredump is generated; incompatible with
+ *                        COREDUMP_REJECT
  */
 enum {
 	COREDUMP_KERNEL		= (1ULL << 0),
@@ -28,6 +31,7 @@ enum {
 	COREDUMP_RECORDS	= (1ULL << 4),
 	COREDUMP_SPARSE		= (1ULL << 5),
 	COREDUMP_MEMORY_TYPES	= (1ULL << 6),
+	COREDUMP_CLOSE_FILES	= (1ULL << 7),
 };
 
 /**
@@ -137,6 +141,10 @@ enum {
  * Note that @memory_types must be zero if COREDUMP_MEMORY_TYPES isn't
  * raised. COREDUMP_MEMORY_TYPES requires COREDUMP_KERNEL and an ack of
  * at least COREDUMP_ACK_SIZE_VER1 bytes.
+ *
+ * If COREDUMP_CLOSE_FILES is raised in @mask the kernel closes the file
+ * descriptors of the coredumping task before it generates the coredump.
+ * The task ends up with an empty descriptor table.
  */
 struct coredump_ack {
 	__u32 size;

-- 
2.53.0

[PATCH RFC v4 18/18] selftests/coredump: test COREDUMP_CLOSE_FILES

[Christian Brauner]

Test COREDUMP_CLOSE_FILES.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 tools/testing/selftests/coredump/Makefile          |   4 +-
 .../selftests/coredump/coredump_close_files_test.c | 592 +++++++++++++++++++++
 .../coredump/coredump_socket_protocol_test.c       |   6 +
 .../selftests/coredump/coredump_test_helpers.c     |   3 +-
 4 files changed, 603 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/coredump/Makefile b/tools/testing/selftests/coredump/Makefile
index dece1a31d561..dc8489d40618 100644
--- a/tools/testing/selftests/coredump/Makefile
+++ b/tools/testing/selftests/coredump/Makefile
@@ -3,7 +3,8 @@ 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_close_files_test
 TEST_FILES := stackdump
 
 include ../lib.mk
@@ -11,3 +12,4 @@ 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_close_files_test: coredump_test_helpers.c
diff --git a/tools/testing/selftests/coredump/coredump_close_files_test.c b/tools/testing/selftests/coredump/coredump_close_files_test.c
new file mode 100644
index 000000000000..7a41906b4695
--- /dev/null
+++ b/tools/testing/selftests/coredump/coredump_close_files_test.c
@@ -0,0 +1,592 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <dirent.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <sys/file.h>
+#include <sys/stat.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+
+#include "coredump_test.h"
+
+#define LOCK_FILE "/tmp/coredump.lock"
+
+/* Idle threads the multi-threaded crashing child spawns. */
+#define NUM_CLOSE_THREADS 4
+
+/* How the crashing child locks LOCK_FILE. */
+enum lock_kind {
+	LOCK_KIND_POSIX,
+	LOCK_KIND_OFD,
+	LOCK_KIND_FLOCK,
+};
+
+/* Who else has a handle on the lock when the child crashes. */
+enum lock_share {
+	LOCK_SHARE_NONE,
+	LOCK_SHARE_DUP,		/* a second slot in the child's own table */
+	LOCK_SHARE_FORK,	/* a forked process keeps the fd open */
+	LOCK_SHARE_FILES,	/* a CLONE_FILES process shares the table */
+};
+
+struct close_test {
+	enum lock_kind kind;
+	enum lock_share share;
+	bool threads;
+	bool close;		/* ack with COREDUMP_CLOSE_FILES */
+	bool userspace;		/* COREDUMP_USERSPACE instead of COREDUMP_KERNEL */
+	bool released;		/* the lock is gone once the kernel is past the close */
+};
+
+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);
+}
+
+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(LOCK_FILE);
+	unlink("/tmp/coredump.socket");
+
+	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);
+}
+
+/* Write-lock @fd the way @kind says. */
+static int take_lock(int fd, enum lock_kind kind)
+{
+	struct flock fl = {
+		.l_type = F_WRLCK,
+		.l_whence = SEEK_SET,
+	};
+
+	switch (kind) {
+	case LOCK_KIND_POSIX:
+		return fcntl(fd, F_SETLK, &fl);
+	case LOCK_KIND_OFD:
+		return fcntl(fd, F_OFD_SETLK, &fl);
+	case LOCK_KIND_FLOCK:
+		return flock(fd, LOCK_EX);
+	}
+
+	return -1;
+}
+
+/* Does anyone else hold a write lock on @fd? 1 if so, 0 if not, -1 on error. */
+static int lock_held(int fd, enum lock_kind kind)
+{
+	struct flock fl = {
+		.l_type = F_WRLCK,
+		.l_whence = SEEK_SET,
+	};
+
+	if (kind == LOCK_KIND_FLOCK) {
+		if (flock(fd, LOCK_EX | LOCK_NB) == 0) {
+			flock(fd, LOCK_UN);
+			return 0;
+		}
+		return errno == EWOULDBLOCK ? 1 : -1;
+	}
+
+	/* F_GETLK reports conflicting OFD locks too. */
+	if (fcntl(fd, F_GETLK, &fl) < 0)
+		return -1;
+	return fl.l_type != F_UNLCK;
+}
+
+/* Number of entries in /proc/@pid/fd, the lowest one in @first. */
+static int count_fds(pid_t pid, int *first)
+{
+	char path[64];
+	struct dirent *de;
+	DIR *dir;
+	int nr = 0;
+
+	snprintf(path, sizeof(path), "/proc/%d/fd", pid);
+	dir = opendir(path);
+	if (!dir)
+		return -1;
+
+	*first = -1;
+	while ((de = readdir(dir))) {
+		int fd;
+
+		if (de->d_name[0] == '.')
+			continue;
+		fd = atoi(de->d_name);
+		if (*first < 0 || fd < *first)
+			*first = fd;
+		nr++;
+	}
+	closedir(dir);
+	return nr;
+}
+
+/*
+ * Block until the test hangs up @fd_release, keeping every inherited fd
+ * open, then report through @fd_result whether @fd is still open.
+ */
+static void hold_until_released(int fd, int fd_release, int fd_result)
+{
+	char c;
+
+	read_nointr(fd_release, &c, 1);
+	c = fcntl(fd, F_GETFD) < 0 ? 'C' : 'O';
+	write_nointr(fd_result, &c, 1);
+	_exit(EXIT_SUCCESS);
+}
+
+/* Lock LOCK_FILE, share it as requested, then crash. */
+static void crashing_child_locked(const struct close_test *t, int fd_release,
+				  int fd_result)
+{
+	pthread_t thread;
+	int fd, pidfd, i;
+	pid_t pid;
+
+	fd = open(LOCK_FILE, O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		_exit(EXIT_FAILURE);
+
+	if (take_lock(fd, t->kind))
+		_exit(EXIT_FAILURE);
+
+	switch (t->share) {
+	case LOCK_SHARE_NONE:
+		break;
+	case LOCK_SHARE_DUP:
+		if (dup(fd) < 0)
+			_exit(EXIT_FAILURE);
+		break;
+	case LOCK_SHARE_FORK:
+		pid = fork();
+		if (pid < 0)
+			_exit(EXIT_FAILURE);
+		if (pid == 0)
+			hold_until_released(fd, fd_release, fd_result);
+		break;
+	case LOCK_SHARE_FILES:
+		pid = create_child(&pidfd, CLONE_FILES);
+		if (pid < 0)
+			_exit(EXIT_FAILURE);
+		if (pid == 0)
+			hold_until_released(fd, fd_release, fd_result);
+		break;
+	}
+
+	if (t->threads)
+		for (i = 0; i < NUM_CLOSE_THREADS; i++)
+			pthread_create(&thread, NULL, do_nothing, NULL);
+
+	/* crash on purpose */
+	*(volatile int *)NULL = 0;
+}
+
+/*
+ * Serve one coredump and look at the task on the way. Before the ack the
+ * lock is held and the descriptors are there. Once the kernel is past the
+ * point where it closes them, which is before the first byte of the dump
+ * or before the hangup in userspace mode, they are gone if we asked for
+ * it and the lock is in the expected state.
+ */
+static int close_server(const struct close_test *t, int fd_ipc)
+{
+	struct coredump_req req = {};
+	struct pidfd_info info = {};
+	int fd_server = -1, fd_coredump = -1, fd_peer_pidfd = -1, fd_lock = -1;
+	int exit_code = EXIT_FAILURE;
+	int fd, first_fd, nr_fds;
+	__u64 mask;
+	ssize_t bytes;
+	char c;
+
+	fd_lock = open(LOCK_FILE, O_RDWR | O_CLOEXEC);
+	if (fd_lock < 0) {
+		fprintf(stderr, "%s: open lock file failed: %m\n", __func__);
+		goto out;
+	}
+
+	fd_server = create_and_listen_unix_socket("/tmp/coredump.socket");
+	if (fd_server < 0) {
+		fprintf(stderr, "%s: create_and_listen_unix_socket failed: %m\n", __func__);
+		goto out;
+	}
+
+	if (write_nointr(fd_ipc, "1", 1) < 0) {
+		fprintf(stderr, "%s: write_nointr to ipc socket failed: %m\n", __func__);
+		goto out;
+	}
+	close(fd_ipc);
+
+	fd_coredump = accept4(fd_server, NULL, NULL, SOCK_CLOEXEC);
+	if (fd_coredump < 0) {
+		fprintf(stderr, "%s: accept4 failed: %m\n", __func__);
+		goto out;
+	}
+
+	fd_peer_pidfd = get_peer_pidfd(fd_coredump);
+	if (fd_peer_pidfd < 0) {
+		fprintf(stderr, "%s: get_peer_pidfd failed\n", __func__);
+		goto out;
+	}
+
+	if (!get_pidfd_info(fd_peer_pidfd, &info)) {
+		fprintf(stderr, "%s: get_pidfd_info failed\n", __func__);
+		goto out;
+	}
+
+	if (!read_coredump_req(fd_coredump, &req)) {
+		fprintf(stderr, "%s: read_coredump_req failed\n", __func__);
+		goto out;
+	}
+
+	if (!check_coredump_req(&req)) {
+		fprintf(stderr, "%s: check_coredump_req failed\n", __func__);
+		goto out;
+	}
+
+	/* The task waits for our answer with everything still in place. */
+	if (lock_held(fd_lock, t->kind) != 1) {
+		fprintf(stderr, "%s: lock not held during the handshake\n", __func__);
+		goto out;
+	}
+
+	nr_fds = count_fds(info.pid, &first_fd);
+	if (nr_fds <= 0) {
+		fprintf(stderr, "%s: no descriptors during the handshake\n", __func__);
+		goto out;
+	}
+
+	fd = sys_pidfd_getfd(fd_peer_pidfd, first_fd, 0);
+	if (fd < 0) {
+		fprintf(stderr, "%s: pidfd_getfd during the handshake failed: %m\n", __func__);
+		goto out;
+	}
+	close(fd);
+
+	mask = COREDUMP_WAIT;
+	mask |= t->userspace ? COREDUMP_USERSPACE : COREDUMP_KERNEL;
+	if (t->close)
+		mask |= COREDUMP_CLOSE_FILES;
+
+	if (!send_coredump_ack(fd_coredump, &req, mask, 0)) {
+		fprintf(stderr, "%s: send_coredump_ack failed\n", __func__);
+		goto out;
+	}
+
+	if (!read_marker(fd_coredump, COREDUMP_MARK_REQACK)) {
+		fprintf(stderr, "%s: read_marker COREDUMP_MARK_REQACK failed\n", __func__);
+		goto out;
+	}
+
+	bytes = read_nointr(fd_coredump, &c, 1);
+	if (bytes != (t->userspace ? 0 : 1)) {
+		fprintf(stderr, "%s: read after the ack returned %zd: %m\n", __func__, bytes);
+		goto out;
+	}
+
+	if (lock_held(fd_lock, t->kind) != !t->released) {
+		fprintf(stderr, "%s: lock %s while the coredump is generated\n",
+			__func__, t->released ? "still held" : "released");
+		goto out;
+	}
+
+	nr_fds = count_fds(info.pid, &first_fd);
+	if (nr_fds < 0 || !nr_fds != t->close) {
+		fprintf(stderr, "%s: %d descriptors while the coredump is generated\n",
+			__func__, nr_fds);
+		goto out;
+	}
+
+	fd = sys_pidfd_getfd(fd_peer_pidfd, first_fd, 0);
+	if (t->close) {
+		if (fd >= 0 || errno != EBADF) {
+			fprintf(stderr, "%s: pidfd_getfd after the close returned %d: %m\n",
+				__func__, fd);
+			goto out;
+		}
+	} else {
+		if (fd < 0) {
+			fprintf(stderr, "%s: pidfd_getfd during the coredump failed: %m\n",
+				__func__);
+			goto out;
+		}
+		close(fd);
+	}
+
+	/* COREDUMP_WAIT keeps the task around until we hang up. */
+	if (!get_pidfd_info(fd_peer_pidfd, &info)) {
+		fprintf(stderr, "%s: get_pidfd_info failed\n", __func__);
+		goto out;
+	}
+
+	if (info.mask & PIDFD_INFO_EXIT) {
+		fprintf(stderr, "%s: task exited before the coredump finished\n", __func__);
+		goto out;
+	}
+
+	for (;;) {
+		char buffer[4096];
+
+		bytes = read_nointr(fd_coredump, buffer, sizeof(buffer));
+		if (bytes < 0) {
+			fprintf(stderr, "%s: read from coredump socket failed: %m\n", __func__);
+			goto out;
+		}
+
+		if (bytes == 0)
+			break;
+	}
+
+	exit_code = EXIT_SUCCESS;
+out:
+	if (fd_lock >= 0)
+		close(fd_lock);
+	if (fd_peer_pidfd >= 0)
+		close(fd_peer_pidfd);
+	if (fd_coredump >= 0)
+		close(fd_coredump);
+	if (fd_server >= 0)
+		close(fd_server);
+	return exit_code;
+}
+
+static void run_close_test(struct __test_metadata *const _metadata,
+			   FIXTURE_DATA(coredump) *self,
+			   const struct close_test *t)
+{
+	int fd, status, ipc_sockets[2], release_pipe[2], result_pipe[2];
+	pid_t pid, pid_coredump_server;
+	char c;
+
+	ASSERT_TRUE(set_core_pattern("@@/tmp/coredump.socket"));
+
+	fd = open(LOCK_FILE, O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0644);
+	ASSERT_GE(fd, 0);
+	EXPECT_EQ(close(fd), 0);
+
+	ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, ipc_sockets), 0);
+
+	pid_coredump_server = fork();
+	ASSERT_GE(pid_coredump_server, 0);
+	if (pid_coredump_server == 0) {
+		close(ipc_sockets[0]);
+		_exit(close_server(t, ipc_sockets[1]));
+	}
+	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);
+
+	/* Only the crashing child and what it spawns see these pipes. */
+	ASSERT_EQ(pipe2(release_pipe, O_CLOEXEC), 0);
+	ASSERT_EQ(pipe2(result_pipe, O_CLOEXEC), 0);
+
+	pid = fork();
+	ASSERT_GE(pid, 0);
+	if (pid == 0) {
+		close(release_pipe[1]);
+		close(result_pipe[0]);
+		crashing_child_locked(t, release_pipe[0], result_pipe[1]);
+	}
+	EXPECT_EQ(close(release_pipe[0]), 0);
+	EXPECT_EQ(close(result_pipe[1]), 0);
+
+	waitpid(pid, &status, 0);
+	ASSERT_TRUE(WIFSIGNALED(status));
+	ASSERT_TRUE(WCOREDUMP(status));
+
+	wait_and_check_coredump_server(pid_coredump_server, _metadata, self);
+
+	/* Let the process holding the shared lock go. */
+	EXPECT_EQ(close(release_pipe[1]), 0);
+
+	/* The crashing child is gone but what it shared with is untouched. */
+	if (t->share == LOCK_SHARE_FORK || t->share == LOCK_SHARE_FILES) {
+		ASSERT_EQ(read_nointr(result_pipe[0], &c, 1), 1);
+		ASSERT_EQ(c, 'O');
+	}
+	EXPECT_EQ(close(result_pipe[0]), 0);
+}
+
+TEST_F(coredump, close_files_posix)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_POSIX,
+		.share = LOCK_SHARE_NONE,
+		.threads = true,
+		.close = true,
+		.released = true,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_ofd)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_OFD,
+		.share = LOCK_SHARE_NONE,
+		.close = true,
+		.released = true,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_flock)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_FLOCK,
+		.share = LOCK_SHARE_NONE,
+		.threads = true,
+		.close = true,
+		.released = true,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_flock_dup)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_FLOCK,
+		.share = LOCK_SHARE_DUP,
+		.close = true,
+		.released = true,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_posix_fork)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_POSIX,
+		.share = LOCK_SHARE_FORK,
+		.close = true,
+		.released = true,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_flock_fork)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_FLOCK,
+		.share = LOCK_SHARE_FORK,
+		.close = true,
+		.released = false,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_ofd_fork)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_OFD,
+		.share = LOCK_SHARE_FORK,
+		.close = true,
+		.released = false,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_posix_shared_table)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_POSIX,
+		.share = LOCK_SHARE_FILES,
+		.close = true,
+		.released = false,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_userspace)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_FLOCK,
+		.share = LOCK_SHARE_NONE,
+		.close = true,
+		.userspace = true,
+		.released = true,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_F(coredump, close_files_not_requested)
+{
+	const struct close_test t = {
+		.kind = LOCK_KIND_FLOCK,
+		.share = LOCK_SHARE_NONE,
+		.threads = true,
+		.close = false,
+		.released = false,
+	};
+
+	run_close_test(_metadata, self, &t);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/coredump/coredump_socket_protocol_test.c b/tools/testing/selftests/coredump/coredump_socket_protocol_test.c
index f5c9bad87546..beb167698cfd 100644
--- a/tools/testing/selftests/coredump/coredump_socket_protocol_test.c
+++ b/tools/testing/selftests/coredump/coredump_socket_protocol_test.c
@@ -2377,6 +2377,12 @@ TEST_F(coredump, socket_request_memory_types_without_kernel)
 	check_conflicting_ack(_metadata, self, COREDUMP_USERSPACE | COREDUMP_MEMORY_TYPES);
 }
 
+/* A rejected task closes everything on its way out anyway. */
+TEST_F(coredump, socket_request_close_files_reject)
+{
+	check_conflicting_ack(_metadata, self, COREDUMP_REJECT | COREDUMP_CLOSE_FILES);
+}
+
 /*
  * A server built with the first structs reads the request it knows,
  * discards the rest and acks with the ack it knows. It raises nothing
diff --git a/tools/testing/selftests/coredump/coredump_test_helpers.c b/tools/testing/selftests/coredump/coredump_test_helpers.c
index 4e36e3e4fb78..65132a0deced 100644
--- a/tools/testing/selftests/coredump/coredump_test_helpers.c
+++ b/tools/testing/selftests/coredump/coredump_test_helpers.c
@@ -1612,7 +1612,8 @@ bool send_coredump_ack(int fd, const struct coredump_req *req,
 #define TEST_REQ_MASK_ALL					\
 	(COREDUMP_KERNEL | COREDUMP_USERSPACE |			\
 	 COREDUMP_REJECT | COREDUMP_WAIT |			\
-	 COREDUMP_RECORDS | COREDUMP_SPARSE | COREDUMP_MEMORY_TYPES)
+	 COREDUMP_RECORDS | COREDUMP_SPARSE | COREDUMP_MEMORY_TYPES | \
+	 COREDUMP_CLOSE_FILES)
 
 bool check_coredump_req(const struct coredump_req *req)
 {

-- 
2.53.0

Re: [PATCH RFC v4 00/18] coredump, files: exit files on request

[NeilBrown]

On Fri, 11 Sep 2026, Christian Brauner wrote:
> We've had quite a few proposal for exiting files before generating the
> coredump (see [1]-[8]). The various proposed solution were quite
> unacceptable. So I took some time to look what a _remotely_ acceptable
> version of this could look like. Here it is.
> 
> The gist is that we allow the coredump socket to request the
> thread-group shed the various fdtables closed synchronously before
> generating the coredump.
> 
> The intricate task is that this requires us to add a primitive to close
> fdtables synchronously. Coredump is not enough to justify this but
> performance numbers for this look pretty convincing. For tasks exiting
> with a large file descriptor table this means getting rid of _a lot_ of
> cmpxchg()s and hammering on task->pi_lock. The cost of this was
> mentioned in various threads over the years. I found at least recent
> comments by Oleg and Neil.
> 
> The obvious problematic case are kernel threads. fput() punts their
> final __fput() to a workqueue. kthreads never return to userspace to run
> task work and some of them may need to finish a umount and so cannot
> ->release() inline. No path we change is taken by a kthread. They don't
> own a descriptor table and kthreadd is created with CLONE_FILES and
> every kthread inherits it. So all of them share init_files and init_task
> pins that forever. Their exit_files() never drops the last reference.
> 
> kernel_execve() refuses PF_KTHREAD outright. Usermodehelpers are spawned
> without the kthread flag. close_range() is a syscall. The one indirect
> way is a failing copy_process() calling exit_files() on a
> child from a kthread parent. A dup_fd() in copy_process() cannot hold
> the last reference to any of its files while the source table is alive.
> 
> Worker threads such as io_uring workers are user threads without
> PF_KTHREAD that share the fdtable. One of them can be the last to put
> the fdtable at exit. Today fput() hands their final __fput()s to task
> work. The worker runs that itself in exit_task_work() a few lines later.
> Now the same thread does the same work in exit_files() instead. vhost
> workers are created without a descriptor table at all, so they never
> even put an fdtable. Neither kind ever execs or calls close_range().
> 
> Now the performance numbers. I measured this on a 64 vCPU KVM guest (AMD
> EPYC 9754) on top of vfs-7.4.coredump with and without the series and
> the same x86_64_defconfig for both. The benchmark opens N files in a
> forked worker, moves them to random positions in the descriptor table so
> that the table order has nothing to do with the allocation order, and
> times the teardown:
> 
> - process exit
> - execve() with N close-on-exec descriptors
> - close_range() with and without CLOSE_RANGE_UNSHARE
> 
> Here are the medians over two boots which agree within 2%. With
> /dev/null as the file and 4096, 65536 and 1M descriptors:
> 
>                          4096 fds        65536 fds        1M fds
>   ----------------------------------------------------------------------
>   exit                   1045 -> 895 us  18.9 -> 14.9 ms  429 -> 306 ms
>                           -14.4%         -21.2%           -28.7%
>   ----------------------------------------------------------------------
>   execve, close-on-exec  1393 -> 1190 us 21.4 -> 16.9 ms  471 -> 325 ms
>                           -14.6%         -21.0%           -31.1%
>   ----------------------------------------------------------------------
>   close_range()          1179 -> 988 us  21.9 -> 17.4 ms  484 -> 337 ms
>                           -16.1%         -20.6%           -30.3%
>   ----------------------------------------------------------------------
>   close_range(UNSHARE)   1118 -> 958 us  22.1 -> 17.8 ms  487 -> 333 ms
>                           -14.3%         -19.8%           -31.7%
> 
> So that's a 16-21% win or 40-150 ns per closed file. This sheds
> init_task_work() and task_work_add() per file with its cmpxchg() and the
> TIF_NOTIFY_RESUME test-and-set, the list walk with the indirect call
> in the path out, and then a second pass over every struct file that
> isn't cache-hot anymore by then.
> 
> The performance boost holds from 256 to 1M descriptors and is largest
> at 1M. Note the thread count doesn't matter since one thread does the
> close anyway.
> 
> So with eight processes tearing down 65536 descriptors each at the same
> time the win is 16-21% (34.9 -> 29.5 ms for exit, 34.4 -> 27.0 ms
> for execve). The old code walks the table once to queue the task work
> and then walks all 16 MB of struct file a second time from
> task_work_run().
> 
> So once eight CPUs compete for the cache that second pass gets
> very expensive. With 32 or 64 processes at once both kernels are bound
> by pushing millions of files through slab and RCU. So then the
> difference shrinks to 4-8%. Still though...
> 
> Files whose own release dominates the cleanup gain a little less. 13%
> for eventfds, 11% for pipes, 6% for AF_UNIX sockets and 7% for distinct
> tmpfs inodes. For them a release costs about 2 us per descriptor anyway.
> 
> Using function profiling a 65536 descriptor exit spends 4.6 ms in
> exit_files() and 11.1 ms in task_work_run() before and now 13.7 ms in
> exit_files() and nothing in task_work_run() after.
> 
> close(2) is untouched and measures the same. will-it-scale (open1,
> open3, dup1, eventfd1, unix1, pipe1, signal1, processes and threads)
> is within 2% either way. dup1_threads at 16 tasks and getppid1 at 64
> tasks a few percent better which I'd put down to layout.
> 
> So, back to coredumps. With COREDUMP_CLOSE_FILES coredumps switch to an
> empty file descriptor table before generating the coredump. That
> requires a bit of synchronization but I think I got the basics down.
> 
> The coredumping thread allocates a new empty file descriptor table. It
> then wakes all threads in the thread-group and tells them to switch to
> the empty file descriptor table and get rid of the old one. They report
> back once they're done.
> 
> So that handles most cases where locks would be held for an unreasonable
> time until the coredump is generated but since files can be shared
> between completely unrelated processes that's not a guarantee and we
> can't give one. But it solves the reported issue without resorting to
> even grosser hacks.
> 
> Link: https://lore.kernel.org/20260618030700.2511668-1-jackzxcui1989@163.com [1]
> Link: https://lore.kernel.org/20260618150301.3226517-1-jackzxcui1989@163.com [2]
> Link: https://lore.kernel.org/20260619122419.3954581-1-jackzxcui1989@163.com [3]
> Link: https://lore.kernel.org/20260624145552.70143-1-jackzxcui1989@163.com [4]
> Link: https://lore.kernel.org/20260630075604.52533-1-jackzxcui1989@163.com [5]
> Link: https://lore.kernel.org/20260804001703.1340667-1-jackzxcui1989@163.com [6]
> Link: https://lore.kernel.org/20260807040124.1706927-1-jackzxcui1989@163.com [7]
> Link: https://lore.kernel.org/20260808052732.2589657-1-jackzxcui1989@163.com [8]
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> ---
> Changes in v4:
> - Fix performance number variations. It said variable within 4%. It's
>   actually just 2%.
> - Make various simplifications and add wait_var_event_state().
> - Link to v3: https://patch.msgid.link/20260909-work-coredump-unlock-self-v3-0-04f907687563@kernel.org

There are some more nice improvement here in v4.
I'm happy to offer a
  Reviewed-by: NeilBrown <neil@brown.name>
for 1 thru 14.  I don't think I have the background to properly assess
the last 4, though I see nothing that might be problematic.

Thanks,
NeilBrown


> 
> Changes in v3:
> - Drop the llist. This brings yet another performance boost.
> - Link to v2: https://patch.msgid.link/20260902-work-coredump-unlock-self-v2-0-1bece368cbb1@kernel.org
> 
> Changes in v2:
> - Revamp.
> - Link to v1: https://patch.msgid.link/20260824-work-coredump-unlock-self-v1-0-48fd1cca7eda@kernel.org
> 
> ---
> Christian Brauner (18):
>       fs: don't open-code file_close_fd() in close_fd()
>       fs: add switch_files_struct()
>       fs: move unshare_fd() to fs/file.c
>       fs: remove unshare_files()
>       fs: add filp_close_sync()
>       fs: make close_files() synchronous
>       fs: make close_range() synchronous
>       fs: rename do_close_on_exec() to close_cloexec_files()
>       fs: make close_cloexec_files() synchronous
>       coredump: drop core_state->dumper
>       sched: add wait_var_event_state()
>       coredump: replace the startup completion with a thread count
>       coredump: factor out coredump_wait_inactive()
>       fs: add alloc_files_struct()
>       coredump: add COREDUMP_CLOSE_FILES
>       coredump: cancel io_uring requests before closing files
>       tools: sync coredump.h header
>       selftests/coredump: test COREDUMP_CLOSE_FILES
> 
>  fs/binfmt_elf.c                                    |   2 +-
>  fs/binfmt_elf_fdpic.c                              |   2 +-
>  fs/coredump.c                                      |  91 +++-
>  fs/exec.c                                          |  13 +-
>  fs/file.c                                          | 107 ++--
>  fs/internal.h                                      |   1 +
>  fs/open.c                                          |  17 +-
>  include/linux/fdtable.h                            |   6 +-
>  include/linux/sched/signal.h                       |   8 +-
>  include/linux/wait_bit.h                           |  26 +
>  include/uapi/linux/coredump.h                      |   8 +
>  kernel/exit.c                                      |  22 +-
>  kernel/fork.c                                      |  48 +-
>  tools/include/uapi/linux/coredump.h                |   8 +
>  tools/testing/selftests/coredump/Makefile          |   4 +-
>  .../selftests/coredump/coredump_close_files_test.c | 592 +++++++++++++++++++++
>  .../coredump/coredump_socket_protocol_test.c       |   6 +
>  .../selftests/coredump/coredump_test_helpers.c     |   3 +-
>  18 files changed, 840 insertions(+), 124 deletions(-)
> ---
> base-commit: a5625efa7a0e77c26e857de147284a21d53fa3ad
> change-id: 20260824-work-coredump-unlock-self-63a898912870
> 
> 

Re: [PATCH RFC v4 10/18] coredump: drop core_state->dumper

[Oleg Nesterov]

On 09/10, Christian Brauner wrote:
>
> The core_state->dumper field isn't used anymore. Only its ->next pointer
> is. The current task is always the dumping thread and the ->task pointer
> is never read. Replace it with a plain pointer to the list of parked
> threads.

...

> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -1504,7 +1504,7 @@ static bool elf_fdpic_core_dump(struct coredump_params *cprm)
>  	if (!psinfo)
>  		goto end_coredump;
>
> -	for (ct = current->signal->core_state->dumper.next;
> +	for (ct = current->signal->core_state->tasks;
>  					ct; ct = ct->next) {

(purely cosmetic, but the extra line looks strange)

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH RFC v4 11/18] sched: add wait_var_event_state()

[Oleg Nesterov]

On 09/10, Christian Brauner wrote:
>
> All wait_var_event() sleep in a fixed task state. For coredumps we need
> a variant that takes the state from the caller the way
> wait_event_state() does. This allows us to continue sleeping with
> TASK_FREEZABLE. That's certainly also a useful addition for other places.
>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> ---
>  include/linux/wait_bit.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH RFC v4 12/18] coredump: replace the startup completion with a thread count

[Oleg Nesterov]

On 09/10, Christian Brauner wrote:
>
> coredump_wait() sets core_state->nr_threads to the number of tasks
> killed and waits for the last thread to enter coredump_task_exit() to
> signal completion. Let's just wait on the count directly. The exiting
> tasks can use atomic_dec_and_wake_up() and the dumping task sleeps in
> wait_var_event_state().
>
> The dumping task must remain freezable since commit f5d39b020809
> ("freezer,sched: Rewrite core freezer logic"). So keep the wait
> TASK_UNINTERRUPTIBLE|TASK_FREEZABLE.
>
> Drop the completion and rename nr_threads to threads_remaining.

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH RFC v4 13/18] coredump: factor out coredump_wait_inactive()

[Oleg Nesterov]

On 09/10, Christian Brauner wrote:
>
> Factor out a new coredump_wait_inactive() helper that
> COREDUMP_CLOSE_FILES can consume in a bit.
>
> No functional changes.
>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> ---
>  fs/coredump.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH RFC v4 09/18] fs: make close_cloexec_files() synchronous

[Oleg Nesterov]

On 09/10, Christian Brauner wrote:
>
> Punting file closing to task work during exec slows down exec
> significantly when its done with a bunch of file descriptors. We can
> do this in-band instead. Flush already runs synchronous. Jann moved
> close-on-exec in e780259b54e6 ("exec: do_close_on_exec() before taking
> exec_update_lock") outside of exec_update_lock.
>
> The only lock that's still held now is cred_guard_mutex. It's deprecated
> and has five takers
>
> (1) exec
> (2) ptrace_attach()
> (3) seccomp() with SECCOMP_FILTER_FLAG_TSYNC
> (4) writes to /proc/<pid>/attr/*
> (5) lsm_set_self_attr()
>
> Four of them take the task's own cred_guard_mutex. When
> close_cloexec_files() runs, de_thread() ensured that the calling task is
> the only one alive in its thread-group. That leaves ptrace() waiting on
> cred_guard_mutex of the tracee going through exec. exec already sleeps
> under cred_guard_mutex in de_thread() when it reads binary and
> interpreter. So while we add wait-time to an attaching ptracer no new
> lock dependency is added.

Plus ptrace has other issues with cred_guard_mutex, ptrace_attach() may
deadlock ;) So I agree this is not a problem.

I like 1-9 and believe they are correct with the additional fix

	[PATCH v3 08/17] exit: hang up the tty before closing the files
	https://lore.kernel.org/all/20260921-work-coredump-fixes-v3-8-8e4adb1619e6@kernel.org/

feel free to add

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH RFC v4 15/18] coredump: add COREDUMP_CLOSE_FILES

[Oleg Nesterov]

On 09/10, Christian Brauner wrote:
>
> Add COREDUMP_CLOSE_FILES and allow a coredump server to request that the
> thread-group closes all files before creating the coredump.

Perhaps the changelog could explain the motivation for this feature...

The patch looks correct, but let me ask some question.

> +static bool coredump_close_files(struct core_state *core_state)
> +{
> +	struct files_struct *files;
> +	struct core_thread *ct;
> +
> +	files = alloc_files_struct();
> +	if (!files)
> +		return false;
> +
> +	for (ct = core_state->tasks; ct; ct = ct->next) {
> +		/* Tasks without a table such as vhost workers can be skipped. */
> +		if (!ct->task->files)
> +			continue;
> +		atomic_inc(&core_state->threads_remaining);
> +		/* ct->files holds a reference until the thread switches to it. */
> +		atomic_inc(&files->count);
> +		/* Pairs with the acquire in coredump_task_exit(). */
> +		smp_store_release(&ct->files, files);
> +		wake_up_process(ct->task);

I don't understand this smp_store_release() and smp_load_acquire() in
coredump_task_exit().

Both wake_up_process() and set_current_state() have the necessary barriers,
see the comment above set_current_state(). So it seems that we can use the
plain store/load.

But what I really can't understand is why coredump_close_files() can't just
do switch_files_struct() itself. It could do override_creds(ct->task->cred)
if this is one of the reasons.

Could you explain this please?

Oleg.

Re: [PATCH RFC v4 15/18] coredump: add COREDUMP_CLOSE_FILES

[Christian Brauner]

On Thu, Sep 24, 2026 at 04:47:45PM +0200, Oleg Nesterov wrote:
> On 09/10, Christian Brauner wrote:
> >
> > Add COREDUMP_CLOSE_FILES and allow a coredump server to request that the
> > thread-group closes all files before creating the coredump.
> 
> Perhaps the changelog could explain the motivation for this feature...
> 
> The patch looks correct, but let me ask some question.
> 
> > +static bool coredump_close_files(struct core_state *core_state)
> > +{
> > +	struct files_struct *files;
> > +	struct core_thread *ct;
> > +
> > +	files = alloc_files_struct();
> > +	if (!files)
> > +		return false;
> > +
> > +	for (ct = core_state->tasks; ct; ct = ct->next) {
> > +		/* Tasks without a table such as vhost workers can be skipped. */
> > +		if (!ct->task->files)
> > +			continue;
> > +		atomic_inc(&core_state->threads_remaining);
> > +		/* ct->files holds a reference until the thread switches to it. */
> > +		atomic_inc(&files->count);
> > +		/* Pairs with the acquire in coredump_task_exit(). */
> > +		smp_store_release(&ct->files, files);
> > +		wake_up_process(ct->task);
> 
> I don't understand this smp_store_release() and smp_load_acquire() in
> coredump_task_exit().
> 
> Both wake_up_process() and set_current_state() have the necessary barriers,
> see the comment above set_current_state(). So it seems that we can use the
> plain store/load.

Oh, I wasn't clear on that.

> But what I really can't understand is why coredump_close_files() can't just
> do switch_files_struct() itself. It could do override_creds(ct->task->cred)
> if this is one of the reasons.

It can, it just felt cleaner to let the individual tasks do it. But I
have deferred that part of the patch series until we got the synchronous
close in.

Re: [PATCH RFC v4 15/18] coredump: add COREDUMP_CLOSE_FILES

[Oleg Nesterov]

On 09/25, Christian Brauner wrote:
>
> On Thu, Sep 24, 2026 at 04:47:45PM +0200, Oleg Nesterov wrote:
> >
> > I don't understand this smp_store_release() and smp_load_acquire() in
> > coredump_task_exit().
> >
> > Both wake_up_process() and set_current_state() have the necessary barriers,
> > see the comment above set_current_state(). So it seems that we can use the
> > plain store/load.
>
> Oh, I wasn't clear on that.
>
> > But what I really can't understand is why coredump_close_files() can't just
> > do switch_files_struct() itself. It could do override_creds(ct->task->cred)
> > if this is one of the reasons.
>
> It can, it just felt cleaner to let the individual tasks do it. But I
> have deferred that part of the patch series until we got the synchronous
> close in.

OK, we can return to this later.

As I said, the patch looks correct to me, so FWIW

Reviewed-by: Oleg Nesterov <oleg@redhat.com>