In Commit 1d8db6fd698de1f73b1a7d72aea578fdd18d9a87 ("pidfs, coredump:
add PIDFD_INFO_COREDUMP"), the coredump handling logic puts the pidfs
entry right after `connect`, and states:

    Make sure to only put our reference after connect() took
    its own reference keeping the pidfs entry alive ...

However, `connect` does not seem to take a reference to the pidfs
entry, just the pid struct (please correct me if I'm wrong here).
Since the expectation is that the coredump server makes a
PIDFD_GET_INFO ioctl to get the coredump info - see Commit
a3b4ca60f93ff3e8b41fffbf63bb02ef3b169c5e ("coredump: add coredump
socket"):

    The pidfd for the crashing task will contain information how the
    task coredumps. The PIDFD_GET_INFO ioctl gained a new flag
    PIDFD_INFO_COREDUMP which can be used to retreive the coredump
    information.

    If the coredump gets a new coredump client connection the kernel
    guarantees that PIDFD_INFO_COREDUMP information is available.

This seems to result in the coredump server racing with the kernel to
get the pidfd before the kernel puts the pidfs entry, and if it loses
it won't be able to retrieve the coredump information.

This patch simply moves the `pidfs_put_pid` call to after the kernel is
done handing off the coredump to the coredump server.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
Cc: brauner@kernel.org
Cc: linux-fsdevel@vger.kernel.org
---
 fs/coredump.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index f217ebf2b3b6..a379758d9ca9 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -898,12 +898,6 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		retval = kernel_connect(socket, (struct sockaddr *)(&addr),
 					addr_len, O_NONBLOCK | SOCK_COREDUMP);
 
-		/*
-		 * ... Make sure to only put our reference after connect() took
-		 * its own reference keeping the pidfs entry alive ...
-		 */
-		pidfs_put_pid(cprm.pid);
-
 		if (retval) {
 			if (retval == -EAGAIN)
 				coredump_report_failure("Coredump socket %s receive queue full", addr.sun_path);
@@ -1002,6 +996,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 close_fail:
 	if (cprm.file)
 		filp_close(cprm.file, NULL);
+	if (cn.core_type == COREDUMP_SOCK)
+		pidfs_put_pid(cprm.pid);
 fail_dropcount:
 	if (cn.core_type == COREDUMP_PIPE)
 		atomic_dec(&core_dump_count);
-- 
2.39.5 (Apple Git-154)

On Thu, Jul 03, 2025 at 02:02:43PM +0200, Laura Brehm wrote:
> In Commit 1d8db6fd698de1f73b1a7d72aea578fdd18d9a87 ("pidfs, coredump:
> add PIDFD_INFO_COREDUMP"), the coredump handling logic puts the pidfs
> entry right after `connect`, and states:
> 
>     Make sure to only put our reference after connect() took
>     its own reference keeping the pidfs entry alive ...
> 
> However, `connect` does not seem to take a reference to the pidfs
> entry, just the pid struct (please correct me if I'm wrong here).

kernel_connect()
-> sock->ops->connect::unix_stream_connect()
   -> prepare_peercred()
      -> pidfs_register_pid()

> Since the expectation is that the coredump server makes a
> PIDFD_GET_INFO ioctl to get the coredump info - see Commit
> a3b4ca60f93ff3e8b41fffbf63bb02ef3b169c5e ("coredump: add coredump
> socket"):
> 
>     The pidfd for the crashing task will contain information how the
>     task coredumps. The PIDFD_GET_INFO ioctl gained a new flag
>     PIDFD_INFO_COREDUMP which can be used to retreive the coredump
>     information.
> 
>     If the coredump gets a new coredump client connection the kernel
>     guarantees that PIDFD_INFO_COREDUMP information is available.
> 
> This seems to result in the coredump server racing with the kernel to
> get the pidfd before the kernel puts the pidfs entry, and if it loses
> it won't be able to retrieve the coredump information.

Honestly curious: is that something you actually observed or that you
think may happen or that an some coding assistant thinks might happen?

> kernel_connect()
> -> sock->ops->connect::unix_stream_connect()
>   -> prepare_peercred()
>      -> pidfs_register_pid()

Ah, thank you! I initially ran into this while working from an older
tree that had had the coredump-socket series cherry-picked into it,
but it was missing Commit fd0a109a0f6b7524543d17520da92a44a9f5343c
("net, pidfs: prepare for handing out pidfds for reaped 
sk->sk_peer_pid").

My tree instead had:
    static void init_peercred(struct sock *sk)
    {
	    sk->sk_peer_pid = get_pid(task_tgid(current));
	    sk->sk_peer_cred = get_current_cred();
    }

I switched over to the main tree when preparing patches, but missed
that the issue was not present there. 

> Honestly curious: is that something you actually observed or that you
> think may happen or that an some coding assistant thinks might happen?

No coding assistants (not a fan), but I understand the question. I
maintain some other large projects and we get a few inane patches
too. I usually try my best to avoid making patches such as these
without some amount of double checking if I'm addressing a real issue,
but I did run into the issue I described (about half the time,
depending on how fast the coredump server ran) in my tree, and I
forgot to repro after switching trees.

Apologies for the inconvenience, and for the understanding/quick
replies!