[PATCH 1/2] coredump: fix race condition between connect and putting pidfs dentry

[PATCH 1/2] coredump: fix race condition between connect and putting pidfs dentry

[Laura Brehm]

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)

[PATCH 2/2] coredump: fix PIDFD_INFO_COREDUMP ioctl check

[Laura Brehm]

In Commit 1d8db6fd698de1f73b1a7d72aea578fdd18d9a87 ("pidfs,
coredump: add PIDFD_INFO_COREDUMP"), the following code was added:

    if (mask & PIDFD_INFO_COREDUMP) {
        kinfo.mask |= PIDFD_INFO_COREDUMP;
        kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
    }
    [...]
    if (!(kinfo.mask & PIDFD_INFO_COREDUMP)) {
        task_lock(task);
        if (task->mm)
            kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
        task_unlock(task);
    }

The second bit in particular looks off to me - the condition in essence
checks whether PIDFD_INFO_COREDUMP was **not** requested, and if so
fetches the coredump_mask in kinfo, since it's checking !(kinfo.mask &
PIDFD_INFO_COREDUMP), which is unconditionally set in the earlier hunk.

I'm tempted to assume the idea in the second hunk was to calculate the
coredump mask if one was requested but fetched in the first hunk, in
which case the check should be
    if ((kinfo.mask & PIDFD_INFO_COREDUMP) && !(kinfo.coredump_mask))
which might be more legibly written as
    if ((mask & PIDFD_INFO_COREDUMP) && !(kinfo.coredump_mask))

This could also instead be achieved by changing the first hunk to be:

    if (mask & PIDFD_INFO_COREDUMP) {
	kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
	if (kinfo.coredump_mask)
	    kinfo.mask |= PIDFD_INFO_COREDUMP;
    }

and the second hunk to:

    if ((mask & PIDFD_INFO_COREDUMP) && !(kinfo.mask & PIDFD_INFO_COREDUMP)) {
	task_lock(task);
        if (task->mm) {
	    kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
            kinfo.mask |= PIDFD_INFO_COREDUMP;
        }
        task_unlock(task);
    }

However, when looking at this, the supposition that the second hunk
means to cover cases where the coredump info was requested but the first
hunk failed to get it starts getting doubtful, so apologies if I'm
completely off-base.

This patch addresses the issue by fixing the check in the second hunk.

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

diff --git a/fs/pidfs.c b/fs/pidfs.c
index 69919be1c9d8..4625e097e3a0 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -319,7 +319,7 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 	if (!c)
 		return -ESRCH;
 
-	if (!(kinfo.mask & PIDFD_INFO_COREDUMP)) {
+	if ((kinfo.mask & PIDFD_INFO_COREDUMP) && !(kinfo.coredump_mask)) {
 		task_lock(task);
 		if (task->mm)
 			kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 1/2] coredump: fix race condition between connect and putting pidfs dentry

[Christian Brauner]

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?

Re: (subset) [PATCH 2/2] coredump: fix PIDFD_INFO_COREDUMP ioctl check

[Christian Brauner]

On Thu, 03 Jul 2025 14:02:44 +0200, Laura Brehm wrote:
> In Commit 1d8db6fd698de1f73b1a7d72aea578fdd18d9a87 ("pidfs,
> coredump: add PIDFD_INFO_COREDUMP"), the following code was added:
> 
>     if (mask & PIDFD_INFO_COREDUMP) {
>         kinfo.mask |= PIDFD_INFO_COREDUMP;
>         kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
>     }
>     [...]
>     if (!(kinfo.mask & PIDFD_INFO_COREDUMP)) {
>         task_lock(task);
>         if (task->mm)
>             kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
>         task_unlock(task);
>     }
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[2/2] coredump: fix PIDFD_INFO_COREDUMP ioctl check
      https://git.kernel.org/vfs/vfs/c/8c0bcafc722c

Re: [PATCH 2/2] coredump: fix PIDFD_INFO_COREDUMP ioctl check

[Christian Brauner]

On Thu, Jul 03, 2025 at 02:02:44PM +0200, Laura Brehm wrote:
> In Commit 1d8db6fd698de1f73b1a7d72aea578fdd18d9a87 ("pidfs,
> coredump: add PIDFD_INFO_COREDUMP"), the following code was added:
> 
>     if (mask & PIDFD_INFO_COREDUMP) {
>         kinfo.mask |= PIDFD_INFO_COREDUMP;
>         kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
>     }
>     [...]
>     if (!(kinfo.mask & PIDFD_INFO_COREDUMP)) {
>         task_lock(task);
>         if (task->mm)
>             kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
>         task_unlock(task);
>     }
> 
> The second bit in particular looks off to me - the condition in essence
> checks whether PIDFD_INFO_COREDUMP was **not** requested, and if so
> fetches the coredump_mask in kinfo, since it's checking !(kinfo.mask &
> PIDFD_INFO_COREDUMP), which is unconditionally set in the earlier hunk.
> 
> I'm tempted to assume the idea in the second hunk was to calculate the
> coredump mask if one was requested but fetched in the first hunk, in
> which case the check should be
>     if ((kinfo.mask & PIDFD_INFO_COREDUMP) && !(kinfo.coredump_mask))
> which might be more legibly written as
>     if ((mask & PIDFD_INFO_COREDUMP) && !(kinfo.coredump_mask))
> 
> This could also instead be achieved by changing the first hunk to be:
> 
>     if (mask & PIDFD_INFO_COREDUMP) {
> 	kinfo.coredump_mask = READ_ONCE(pidfs_i(inode)->__pei.coredump_mask);
> 	if (kinfo.coredump_mask)
> 	    kinfo.mask |= PIDFD_INFO_COREDUMP;
>     }
> 
> and the second hunk to:
> 
>     if ((mask & PIDFD_INFO_COREDUMP) && !(kinfo.mask & PIDFD_INFO_COREDUMP)) {
> 	task_lock(task);
>         if (task->mm) {
> 	    kinfo.coredump_mask = pidfs_coredump_mask(task->mm->flags);
>             kinfo.mask |= PIDFD_INFO_COREDUMP;
>         }
>         task_unlock(task);
>     }
> 
> However, when looking at this, the supposition that the second hunk
> means to cover cases where the coredump info was requested but the first
> hunk failed to get it starts getting doubtful, so apologies if I'm
> completely off-base.
> 
> This patch addresses the issue by fixing the check in the second hunk.
> 
> Signed-off-by: Laura Brehm <laurabrehm@hey.com>
> Cc: brauner@kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> ---

Yes, that looks correct to me. Thanks for the fix!

Re: [PATCH 1/2] coredump: fix race condition between connect and

[Laura Brehm]

> 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!