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!
