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

[Christian Brauner]

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.