linux-kernel - Re: [PATCH 3/3] coredump: hand a pidfd to the usermode coredump helper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250414124843.GB28345@redhat.com>
Date: Mon, 14 Apr 2025 14:48:44 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Christian Brauner <brauner@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, Luca Boccassi <luca.boccassi@...il.com>,
	Lennart Poettering <lennart@...ttering.net>,
	Daan De Meyer <daan.j.demeyer@...il.com>,
	Mike Yuan <me@...dnzj.com>,
	Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] coredump: hand a pidfd to the usermode coredump
 helper

On 04/14, Christian Brauner wrote:
>
> +			case 'F': {
> +				struct file *pidfs_file __free(fput) = NULL;
> +
> +				/*
> +				 * Install a pidfd only makes sense if
> +				 * we actually spawn a usermode helper.
> +				 */
> +				if (!ispipe)
> +					break;
> +
> +				/*
> +				 * We already created a pidfs_file but the user
> +				 * specified F multiple times. Just print the
> +				 * number multiple times.
> +				 */
> +				if (!cprm->pidfs_file) {
> +					/*
> +					 * Create a pidfs file for the
> +					 * coredumping thread that we can
> +					 * install into the usermode helper's
> +					 * file descriptor table later.
> +					 *
> +					 * Note that we'll install a pidfd for
> +					 * the thread-group leader. We know that
> +					 * task linkage hasn't been removed yet
> +					 * and even if this @current isn't the
> +					 * actual thread-group leader we know
> +					 * that the thread-group leader cannot
> +					 * be reaped until @current has exited.
> +					 */
> +					pidfs_file = pidfs_alloc_file(task_tgid(current), 0);
> +					if (IS_ERR(pidfs_file))
> +						return PTR_ERR(pidfs_file);
> +				}
> +
> +				 /*
> +				 * Usermode helpers are childen of
> +				 * either system_unbound_wq or of
> +				 * kthreadd. So we know that we're
> +				 * starting off with a clean file
> +				 * descriptor table. Thus, we should
> +				 * always be able to use file descriptor
> +				 * number 3.
> +				 */
> +				err = cn_printf(cn, "%d", COREDUMP_PIDFD_NUMBER);
> +				if (err)
> +					return err;
> +
> +				cprm->pidfs_file = no_free_ptr(pidfs_file);
> +				break;
> +			}

So the new case 'F' differs from other case's in that it doesn't do
"break" but returns the error... this is a bit inconsistent.

Note also that if you do cn_printf() before pidfs_alloc_file(), then you
can avoid __free(fput) and no_free_ptr().

But this is minor. Can't we simplify this patch?

Rather than add the new pidfs_file member into coredump_params, we can
add "struct pid *pid". format_corename() will simply do

	case 'F':
		if (ispipe) {
			// no need to do get_pid()
			cprm->pid = task_tgid(current);
			err = cn_printf(...);
		}
		break;

and umh_pipe_setup() can itself do pidfs_alloc_file(cp->pid) if it is
not NULL.

This way do_coredump() doesn't need any changes.

No?

Oleg.