Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()

[Christian Brauner]

On Tue, Jan 08, 2019 at 11:20:23AM -0700, Tycho Andersen wrote:
> On Tue, Jan 08, 2019 at 12:17:42PM -0600, Serge E. Hallyn wrote:
> > On Tue, Jan 08, 2019 at 10:58:43AM -0700, Tycho Andersen wrote:
> > > On Tue, Jan 08, 2019 at 11:54:15AM -0600, Serge E. Hallyn wrote:
> > > > On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> > > > > On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > > > > > +			/*
> > > > > > +			 * Stop the child so we can inspect whether we have
> > > > > > +			 * recycled pid PID_RECYCLE.
> > > > > > +			 */
> > > > > > +			close(pipe_fds[0]);
> > > > > > +			ret = kill(recycled_pid, SIGSTOP);
> > > > > > +			close(pipe_fds[1]);
> > > > > > +			if (ret) {
> > > > > > +				(void)wait_for_pid(recycled_pid);
> > > > > > +				_exit(PIDFD_ERROR);
> > > > > > +			}
> > > > > 
> > > > > Sorry for being late to the party, but I wonder if this whole thing
> > > > > couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
> > > > 
> > > > no, bc it's not namespaced :)
> > > 
> > > Huh? It looks like it is...
> > > 
> > > static int pid_ns_ctl_handler(struct ctl_table *table, int write,
> > >                 void __user *buffer, size_t *lenp, loff_t *ppos)
> > > {
> > >         struct pid_namespace *pid_ns = task_active_pid_ns(current);
> > >         struct ctl_table tmp = *table;
> > >         int ret, next;
> > > 
> > >         if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> > >                 return -EPERM;
> > > 
> > >         ...
> > 
> > Oh - hah, but that's ns_last_pid.  You'd want pid_max.  And that one
> > is not namespaced.
> 
> Perhaps I'm misunderstanding, but isn't the point of all this code to
> get the same pid again? So can't we just fork(), kill(), then set
> ns_last_pid to pid-1, and fork() again to re-use?

Maybe. It's just a selftest that works reliably as it is so unless
there's a technical issue with the patch I'm not going to do another
version just because of that unless people feel super strongly about
this.
Another advantage is that the code we have right now works even when
CONFIG_CHECKPOINT_RESTORE is not selected.