[CRIU] [PATCH 4/5] files: save inherited file descriptors in fdstore
Adrian Reber
adrian at lisas.de
Fri Sep 21 10:05:56 MSK 2018
Hello Andrei,
I guess this is a fix for my inherit FD problem report via RPC, right.
This also fixes it correctly for me. Thanks. So:
Tested-by: Adrian Reber <areber at redhat.com>
But it does not apply cleanly on my criu-dev checkout, because of
commit 14740c77267c0f3868ab7f14a2fa74546741b474
Author: Andrei Vagin <avagin at virtuozzo.com>
Date: Thu Sep 13 02:41:14 2018 +0300
criu: set RLIMIT_NOFILE to maximum only on restore
Reviewed-by: Kirill Tkhai <ktkhai at virtuozzo.com>
Signed-off-by: Andrei Vagin <avagin at virtuozzo.com>
The first hunk of files.c does not apply correctly due to a missing
include '#include "kerndat.h"' in you patch.
Adrian
On Thu, Sep 20, 2018 at 10:22:37PM +0000, Andrey Vagin wrote:
> In this case:
> * don't need to care whether they intercept with restored file
> descriptors.
> * they don't copied into each restored process
> * don't need to close them one by one
>
> Signed-off-by: Andrei Vagin <avagin at virtuozzo.com>
> ---
> criu/cr-restore.c | 9 +--
> criu/files-reg.c | 4 ++
> criu/files.c | 164 +++++++--------------------------------------------
> criu/include/files.h | 3 +-
> criu/util.c | 4 --
> 5 files changed, 30 insertions(+), 154 deletions(-)
>
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index 45ed9c63a..c949f9b0f 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -885,9 +885,6 @@ static int restore_one_alive_task(int pid, CoreEntry *core)
> if (collect_zombie_pids(ta) < 0)
> return -1;
>
> - if (inherit_fd_fini() < 0)
> - return -1;
> -
> if (prepare_proc_misc(pid, core->tc))
> return -1;
>
> @@ -1020,9 +1017,6 @@ static int restore_one_zombie(CoreEntry *core)
> if (prepare_fds(current))
> return -1;
>
> - if (inherit_fd_fini() < 0)
> - return -1;
> -
> if (lazy_pages_setup_zombie(vpid(current)))
> return -1;
>
> @@ -2379,6 +2373,9 @@ int cr_restore_tasks(void)
> if (fdstore_init())
> goto err;
>
> + if (inherit_fd_move_to_fdstore())
> + goto err;
> +
> if (crtools_prepare_shared() < 0)
> goto err;
>
> diff --git a/criu/files-reg.c b/criu/files-reg.c
> index e3ed01f55..2404c89bc 100644
> --- a/criu/files-reg.c
> +++ b/criu/files-reg.c
> @@ -1624,6 +1624,7 @@ int open_path(struct file_desc *d,
> struct reg_file_info *rfi;
> char *orig_path = NULL;
> char path[PATH_MAX];
> + int inh_fd = -1;
>
> if (inherited_fd(d, &tmp))
> return tmp;
> @@ -1633,6 +1634,7 @@ int open_path(struct file_desc *d,
> if (rfi->rfe->ext) {
> tmp = inherit_fd_lookup_id(rfi->rfe->name);
> if (tmp >= 0) {
> + inh_fd = tmp;
> mntns_root = open_pid_proc(PROC_SELF);
> snprintf(path, sizeof(path), "fd/%d", tmp);
> orig_path = rfi->path;
> @@ -1691,8 +1693,10 @@ ext:
> tmp = open_cb(mntns_root, rfi, arg);
> if (tmp < 0) {
> pr_perror("Can't open file %s", rfi->path);
> + close_safe(&inh_fd);
> return -1;
> }
> + close_safe(&inh_fd);
>
> if ((rfi->rfe->has_size || rfi->rfe->has_mode) &&
> !rfi->size_mode_checked) {
> diff --git a/criu/files.c b/criu/files.c
> index e59388f28..4bd6468b4 100644
> --- a/criu/files.c
> +++ b/criu/files.c
> @@ -44,6 +44,7 @@
> #include "autofs.h"
> #include "parasite.h"
> #include "parasite-syscall.h"
> +#include "fdstore.h"
>
> #include "protobuf.h"
> #include "util.h"
> @@ -1048,10 +1049,6 @@ static int send_fd_to_self(int fd, struct fdinfo_list_entry *fle)
> if (fd == dfd)
> return 0;
>
> - /* make sure we won't clash with an inherit fd */
> - if (inherit_fd_resolve_clash(dfd) < 0)
> - return -1;
> -
> BUG_ON(dfd == get_service_fd(TRANSPORT_FD_OFF));
>
> pr_info("\t\t\tGoing to dup %d into %d\n", fd, dfd);
> @@ -1239,8 +1236,6 @@ splice:
> return ret;
> }
>
> -static struct inherit_fd *inherit_fd_lookup_fd(int fd, const char *caller);
> -
> int close_old_fds(void)
> {
> DIR *dir;
> @@ -1261,8 +1256,7 @@ int close_old_fds(void)
> return -1;
> }
>
> - if ((!is_any_service_fd(fd)) && (dirfd(dir) != fd) &&
> - !inherit_fd_lookup_fd(fd, __FUNCTION__))
> + if ((!is_any_service_fd(fd)) && (dirfd(dir) != fd))
> close_safe(&fd);
> }
>
> @@ -1486,49 +1480,11 @@ struct inherit_fd {
> struct list_head inh_list;
> char *inh_id; /* file identifier */
> int inh_fd; /* criu's descriptor to inherit */
> - dev_t inh_dev;
> - ino_t inh_ino;
> - mode_t inh_mode;
> - dev_t inh_rdev;
> + int inh_fd_id;
> };
>
> int inh_fd_max = -1;
>
> -/*
> - * Return 1 if inherit fd has been closed or reused, 0 otherwise.
> - *
> - * Some parts of the file restore engine can close an inherit fd
> - * explicitly by close() or implicitly by dup2() to reuse that descriptor.
> - * In some specific functions (for example, send_fd_to_self()), we
> - * check for clashes at the beginning of the function and, therefore,
> - * these specific functions will not reuse an inherit fd. However, to
> - * avoid adding a ton of clash detect and resolve code everywhere we close()
> - * and/or dup2(), we just make sure that when we're dup()ing or close()ing
> - * our inherit fd we're still dealing with the same fd that we inherited.
> - */
> -static int inherit_fd_reused(struct inherit_fd *inh)
> -{
> - struct stat sbuf;
> -
> - if (fstat(inh->inh_fd, &sbuf) == -1) {
> - if (errno == EBADF) {
> - pr_debug("Inherit fd %s -> %d has been closed\n",
> - inh->inh_id, inh->inh_fd);
> - return 1;
> - }
> - pr_perror("Can't fstat inherit fd %d", inh->inh_fd);
> - return -1;
> - }
> -
> - if (inh->inh_dev != sbuf.st_dev || inh->inh_ino != sbuf.st_ino ||
> - inh->inh_mode != sbuf.st_mode || inh->inh_rdev != sbuf.st_rdev) {
> - pr_info("Inherit fd %s -> %d has been reused\n",
> - inh->inh_id, inh->inh_fd);
> - return 1;
> - }
> - return 0;
> -}
> -
> /*
> * We can't print diagnostics messages in this function because the
> * log file isn't initialized yet.
> @@ -1595,10 +1551,6 @@ int inherit_fd_add(int fd, char *key)
>
> inh->inh_id = key;
> inh->inh_fd = fd;
> - inh->inh_dev = sbuf.st_dev;
> - inh->inh_ino = sbuf.st_ino;
> - inh->inh_mode = sbuf.st_mode;
> - inh->inh_rdev = sbuf.st_rdev;
> list_add_tail(&inh->inh_list, &opts.inherit_fds);
> return 0;
> }
> @@ -1617,6 +1569,20 @@ void inherit_fd_log(void)
> }
> }
>
> +int inherit_fd_move_to_fdstore(void)
> +{
> + struct inherit_fd *inh;
> +
> + list_for_each_entry(inh, &opts.inherit_fds, inh_list) {
> + inh->inh_fd_id = fdstore_add(inh->inh_fd);
> + if (inh->inh_fd_id < 0)
> + return -1;
> + close_safe(&inh->inh_fd);
> + }
> +
> + return 0;
> +}
> +
> /*
> * Look up the inherit fd list by a file identifier.
> */
> @@ -1628,11 +1594,9 @@ int inherit_fd_lookup_id(char *id)
> ret = -1;
> list_for_each_entry(inh, &opts.inherit_fds, inh_list) {
> if (!strcmp(inh->inh_id, id)) {
> - if (!inherit_fd_reused(inh)) {
> - ret = inh->inh_fd;
> - pr_debug("Found id %s (fd %d) in inherit fd list\n",
> - id, ret);
> - }
> + ret = fdstore_get(inh->inh_fd_id);
> + pr_debug("Found id %s (fd %d) in inherit fd list\n",
> + id, ret);
> break;
> }
> }
> @@ -1655,96 +1619,12 @@ bool inherited_fd(struct file_desc *d, int *fd_p)
> if (fd_p == NULL)
> return true;
>
> - *fd_p = dup(i_fd);
> - if (*fd_p < 0)
> - pr_perror("Inherit fd DUP failed");
> - else
> - pr_info("File %s will be restored from fd %d dumped "
> + *fd_p = i_fd;
> + pr_info("File %s will be restored from fd %d dumped "
> "from inherit fd %d\n", id_str, *fd_p, i_fd);
> return true;
> }
>
> -/*
> - * Look up the inherit fd list by a file descriptor.
> - */
> -static struct inherit_fd *inherit_fd_lookup_fd(int fd, const char *caller)
> -{
> - struct inherit_fd *ret;
> - struct inherit_fd *inh;
> -
> - ret = NULL;
> - list_for_each_entry(inh, &opts.inherit_fds, inh_list) {
> - if (inh->inh_fd == fd) {
> - if (!inherit_fd_reused(inh)) {
> - ret = inh;
> - pr_debug("Found fd %d (id %s) in inherit fd list (caller %s)\n",
> - fd, inh->inh_id, caller);
> - }
> - break;
> - }
> - }
> - return ret;
> -}
> -
> -/*
> - * If the specified fd clashes with an inherit fd,
> - * move the inherit fd.
> - */
> -int inherit_fd_resolve_clash(int fd)
> -{
> - int newfd;
> - struct inherit_fd *inh;
> -
> - inh = inherit_fd_lookup_fd(fd, __FUNCTION__);
> - if (inh == NULL)
> - return 0;
> -
> - newfd = dup(fd);
> - if (newfd == -1) {
> - pr_perror("Can't dup inherit fd %d", fd);
> - return -1;
> - }
> -
> - if (close(fd) == -1) {
> - close(newfd);
> - pr_perror("Can't close inherit fd %d", fd);
> - return -1;
> - }
> -
> - inh->inh_fd = newfd;
> - pr_debug("Inherit fd %d moved to %d to resolve clash\n", fd, inh->inh_fd);
> - return 0;
> -}
> -
> -/*
> - * Close all inherit fds.
> - */
> -int inherit_fd_fini()
> -{
> - int reused;
> - struct inherit_fd *inh;
> -
> - list_for_each_entry(inh, &opts.inherit_fds, inh_list) {
> - if (inh->inh_fd < 0) {
> - pr_err("File %s in inherit fd list has invalid fd %d\n",
> - inh->inh_id, inh->inh_fd);
> - return -1;
> - }
> -
> - reused = inherit_fd_reused(inh);
> - if (reused < 0)
> - return -1;
> -
> - if (!reused) {
> - pr_debug("Closing inherit fd %d -> %s\n", inh->inh_fd,
> - inh->inh_id);
> - if (close_safe(&inh->inh_fd) < 0)
> - return -1;
> - }
> - }
> - return 0;
> -}
> -
> int open_transport_socket(void)
> {
> pid_t pid = vpid(current);
> diff --git a/criu/include/files.h b/criu/include/files.h
> index 75757bd1d..2c1e1e723 100644
> --- a/criu/include/files.h
> +++ b/criu/include/files.h
> @@ -190,8 +190,7 @@ extern int dump_unsupp_fd(struct fd_parms *p, int lfd,
> extern int inherit_fd_parse(char *optarg);
> extern int inherit_fd_add(int fd, char *key);
> extern void inherit_fd_log(void);
> -extern int inherit_fd_resolve_clash(int fd);
> -extern int inherit_fd_fini(void);
> +extern int inherit_fd_move_to_fdstore(void);
>
> extern int inherit_fd_lookup_id(char *id);
>
> diff --git a/criu/util.c b/criu/util.c
> index 22d0be42f..1ffa6033f 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -241,10 +241,6 @@ int reopen_fd_as_safe(char *file, int line, int new_fd, int old_fd, bool allow_r
> int tmp;
>
> if (old_fd != new_fd) {
> - /* make sure we won't clash with an inherit fd */
> - if (inherit_fd_resolve_clash(new_fd) < 0)
> - return -1;
> -
> if (!allow_reuse_fd) {
> if (fcntl(new_fd, F_GETFD) != -1 || errno != EBADF) {
> pr_err("fd %d already in use (called at %s:%d)\n",
> --
> 2.14.4
>
>
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Adrian
--
Adrian Reber <adrian at lisas.de> http://lisas.de/~adrian/
New crypt. See /usr/news/crypt.
More information about the CRIU
mailing list