[CRIU] [PATCH] Make restored processes inherit file descriptors from criu

[Pavel Emelyanov]

Hi, Saied.

I really hope you didn't plan this for 1.4. It's in several days
and the repo is in feature freeze.

> diff --git a/files-reg.c b/files-reg.c
> index 20b50a0..bfd1896 100644
> --- a/files-reg.c
> +++ b/files-reg.c
> @@ -962,6 +962,14 @@ int open_path(struct file_desc *d,
>  	}
>  
>  	mntns_root = mntns_get_root_by_mnt_id(rfi->rfe->mnt_id);
> +
> +pr_debug(">>> open_path(): rfi->path=(%s)\n", rfi->path);
> +	tmp = inherit_fd_lookup_id(rfi->path, true);

I've found no symmetric check on dump for pathed files. There are
two checks for pipes -- on dump and on restore -- but only one
here.

BTW, maybe we can pull the inherited fd logic higher the stack?
Like this.

On dump in the do_dump_gen_file() before we dive into the ops->dump()
callback we 
 * check that we have some inherited-fds in the game,
 * get p->link path (it will be the path file files and should be
   the pipe[ID] for pipes, if for pipes it's empty there's the
   read_fd_link() helper out there)
 * lookup the inherit-fd list
 * dump the fd as inherited. BTW, shouldn't we put the inherited
   mark on the fdinfo_entry rather than on the file entry?

On restore we can hook into the open_fd() routine. Instead of
doing just d->ops->open we
 * check for external-fd being requested
 * get the respective fd from the current fdtable (or from the
   service-socket-queue as I suggested in another thread)

What do you think?

> +	if (tmp >= 0) {
> +		pr_info("File %s will be restored from fd %d\n", rfi->path, tmp);
> +		return tmp;
> +	}
> +
>  	tmp = open_cb(mntns_root, rfi, arg);
>  	if (tmp < 0) {
>  		pr_perror("Can't open file %s", rfi->path);


> +int inherit_fd_add(char *optarg)
> +{
> +	char *cp;
> +	int n;
> +	int fd;
> +	struct inherit_fd *inh;
> +
> +	/* beding debug support */
> +	fd = -1;
> +	n = sscanf(optarg, "debug[%d]:", &fd);
> +	if (n == 1) {
> +		/*
> +		 * If the argument has the format debug[%d]:%s, it means
> +		 * just write out the string after colon to the file
> +		 * descriptor %d.  This can be used to leave a restore
> +		 * marker in the output stream of the process.
> +		 */
> +		if (fd < 0) {
> +			pr_msg("Invalid fd number %d in argument %s\n",
> +				fd, optarg);
> +			return -1;
> +		}
> +		cp = strchr(optarg, ':') + 1;
> +		n = strlen(cp);
> +		if (write(fd, cp, n) != n) {
> +			pr_msg("Cannot write debug message %s to fd %d\n",
> +				cp, fd);
> +			return -1;
> +		}
> +		return 0;
> +	}
> +	/* end debug support */
> +
> +	fd = -1;
> +	n = sscanf(optarg, "fd[%d]:", &fd);
> +	if (n == 1) {
> +		/* argument is in fd[%d]:%s format */
> +		if (fd < 0) {
> +			pr_msg("Invalid fd number %d in argument %s\n", fd, optarg);
> +			return -1;
> +		}
> +		cp = strchr(optarg, ':');
> +		*cp++ = '\0';
> +	} else {
> +		/* argument is in %s format */
> +		cp = optarg;

But in this case inh_fd would be -1 and the validate routine would just
fail. What's the point in such an entry?

> +	}
> +
> +	if ((inh = xmalloc(sizeof *inh)) == NULL)
> +		return -1;
> +	inh->inh_id = cp;
> +	inh->inh_fd = fd;
> +	list_add_tail(&inh->l, &opts.inherit_fds);
> +	return 0;
> +}


> @@ -304,6 +316,18 @@ static int open_pipe(struct file_desc *d)
>  
>  	pr_info("\t\tCreating pipe pipe_id=%#x id=%#x\n", pi->pe->pipe_id, pi->pe->id);
>  
> +	pipe_name = pipe_id_string(pi->pe->pipe_id);
> +	tmp = inherit_fd_lookup_id(pipe_name, true);
> +	if (tmp >= 0) {
> +		pr_info("Pipe %s will be restored from inherit fd %d\n",
> +			pipe_name, tmp);
> +		return tmp;
> +	}
> +	if (pi->pe->flags == INHERIT_FD) {

Should it rather be inverted? Like

if (pi->pe->flags == INHERIT_FD) {
	tmp = inherit_fd_lookup_id();
	...
}

> +		pr_err("No inherit fd for pipe %s\n", pipe_name);
> +		return -1;
> +	}
> +
>  	if (!pi->create)
>  		return recv_pipe_fd(pi);
>  
> @@ -497,13 +521,18 @@ static int dump_one_pipe(int lfd, u32 id, const struct fd_parms *p)
>  
>  	pe.id		= id;
>  	pe.pipe_id	= pipe_id(p);
> -	pe.flags	= p->flags;
>  	pe.fown		= (FownEntry *)&p->fown;
> +	if (inherit_fd_lookup_id(pipe_id_string(pe.pipe_id), false) < 0)
> +		pe.flags = p->flags;
> +	else {
> +		pe.flags = INHERIT_FD;

I think it's better to introduce a new optional filed in the image. Reading
explicit "inherited = True" in the criu show output would be less confusing :)

> +		pr_info("Pipe %d marked as inherit fd\n", pe.pipe_id);
> +	}
>  
>  	if (pb_write_one(img_from_set(glob_imgset, CR_FD_PIPES), &pe, PB_PIPE))
>  		return -1;
>  
> -	return dump_one_pipe_data(&pd_pipes, lfd, p);
> +	return pe.flags == INHERIT_FD ? 0 : dump_one_pipe_data(&pd_pipes, lfd, p);
>  }
>  


> @@ -93,6 +94,14 @@ void pr_vma(unsigned int loglevel, const struct vma_area *vma_area)
>  int close_safe(int *fd)
>  {
>  	int ret = 0;
> +
> +	/*
> +	 * Don't close files descriptors that criu's caller
> +	 * set up to be inherited by the restored process.
> +	 */
> +	if (inherit_fd_lookup_fd(*fd))
> +		return 0;
> +

Just for the record -- I agree with Andrey, that this place is to generic
for inherited fd check.

>  	if (*fd > -1) {
>  		ret = close(*fd);
>  		if (!ret)
> 

Thanks,
Pavel