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.
