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@xxxxxxx>
> Cc: brauner@xxxxxxxxxx
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> ---

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