On Mon, Apr 14, 2025 at 02:48:44PM +0200, Oleg Nesterov wrote:
> 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.
Yeah, though that already happens right at the top.
>
> Note also that if you do cn_printf() before pidfs_alloc_file(), then you
> can avoid __free(fput) and no_free_ptr().
Seemed inconsitent to me to print first even though we didn't succeed
but I agree that it doesn't matter.
>
> But this is minor. Can't we simplify this patch?
Let's hear it...
>
> 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.
Sure, same result but works for me. I'll send v2 in a bit.
>
> This way do_coredump() doesn't need any changes.
>
> No?
Yep, sounds good.
