[CRIU] [PATCH] remap: fix dead pid remap of /proc/<pid>

[Tycho Andersen]

On Wed, Jan 13, 2016 at 04:05:28PM +0300, Pavel Emelyanov wrote:
> On 01/06/2016 06:57 PM, Tycho Andersen wrote:
> > It turns out we can't just test for /proc/<pid>, because the kernel appends
> > (deleted), since the directory is actually deleted (vs. something like
> > /proc/1/mountinfo, where the file still exists in the unlinked directory,
> > so there is no (deleted)). See comment for details.
> > 
> > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > ---
> >  files-reg.c                            | 34 +++++++++++++++++++++++++---------
> >  test/zdtm/live/static/remap_dead_pid.c | 22 ++++++++++++++++++++--
> >  2 files changed, 45 insertions(+), 11 deletions(-)
> > 
> > diff --git a/files-reg.c b/files-reg.c
> > index 0f89be5..43872f0 100644
> > --- a/files-reg.c
> > +++ b/files-reg.c
> > @@ -887,16 +887,32 @@ static int check_path_remap(struct fd_link *link, const struct fd_parms *parms,
> >  			return -1;
> >  		pid = strtol(start + 1, &end, 10);
> >  
> > -		/* if we didn't find another /, this path something
> > -		 * like ./proc/kmsg, which we shouldn't mess with. */
> > -		if (*end == '/') {
> > -			*end = 0;
> > -			ret = access(rpath, F_OK);
> > -			*end = '/';
> > -
> > -			if (ret) {
> > +		/* If strtol didn't convert anything, then we are looking at
> > +		 * something like /proc/kmsg, which we shouldn't mess with.
> > +		 * Anything under /proc/<pid> (including that directory itself)
> > +		 * can be c/r'd with a dead pid remap, so let's allow all such
> > +		 * cases.
> > +		 */
> > +		if (pid != 0) {
> > +			bool is_dead = strip_deleted(link);
> > +
> > +			/* /proc/<pid> will be "/proc/1 (deleted)" when it is
> > +			 * dead, but a path like /proc/1/mountinfo won't have
> > +			 * the suffix, since it isn't actually deleted (still
> > +			 * exists, but the parent dir is deleted). So, if we
> > +			 * have a path like /proc/1/mountinfo, test if /proc/1
> > +			 * exists instead, since this is what CRIU will need to
> > +			 * open on restore.
> > +			 */
> > +			if (!is_dead) {
> 
> If the is_dead is 0 here we never go dumping the pid remap, which seems to
> be breaking the behavior on /proc/pid/xxx files.
> 
> > +				*end = 0;
> > +				ret = access(rpath, F_OK);
> 
> Shouldn't this be is_dead = access() instead?

Oh, yes. Doesn't that fix the case you noted above, too?

Also, I'll fix the test so that it doesn't use the same pid for both
cases, so we'll see this if it breaks in the future :)

Tycho