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

[Pavel Emelyanov]

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?

> +				*end = '/';
> +			}
> +
> +			if (is_dead) {
>  				pr_info("Dumping dead process remap of %d\n", pid);
> -				return dump_dead_process_remap(pid, rpath + 1, plen - 1, ost, lfd, id, nsid);
> +				return dump_dead_process_remap(pid, rpath + 1, link->len - 1, ost, lfd, id, nsid);
>  			}
>  		}
>  
> diff --git a/test/zdtm/live/static/remap_dead_pid.c b/test/zdtm/live/static/remap_dead_pid.c
> index c16832d..a9d5d1a 100644
> --- a/test/zdtm/live/static/remap_dead_pid.c
> +++ b/test/zdtm/live/static/remap_dead_pid.c
> @@ -36,13 +36,23 @@ int main(int argc, char **argv)
>  		while(1)
>  			sleep(10);
>  	} else {
> -		int fd, ret;
> +		int fd, fd2, ret;
>  		char path[PATH_MAX];
>  		pid_t result;
>  
>  		sprintf(path, "/proc/%d/mountinfo", pid);
> -
>  		fd = open(path, O_RDONLY);
> +		if (fd < 0) {
> +			fail("failed to open fd");
> +			return -1;
> +		}
> +
> +		sprintf(path, "/proc/%d", pid);
> +		fd2 = open(path, O_RDONLY);
> +		if (fd2 < 0) {
> +			fail("failed to open fd2");
> +			return -1;
> +		}
>  
>  		/* no matter what, we should kill the child */
>  		kill(pid, SIGKILL);
> @@ -67,6 +77,14 @@ int main(int argc, char **argv)
>  			fail("bad fd after restore");
>  			return -1;
>  		}
> +
> +		ret = fcntl(fd2, F_GETFD);
> +		close(fd2);
> +
> +		if (ret) {
> +			fail("bad fd2 after restore");
> +			return -1;
> +		}
>  	}
>  
>  	pass();
>