From 34024dfdcbc011ca91a5a2eac090826f96139ff2 Mon Sep 17 00:00:00 2001 From: Pavel Tikhomirov Date: Tue, 10 Nov 2020 11:36:02 +0300 Subject: [PATCH] util: move open_proc_self_fd to service_fd We need this to avoid conflicts with file descriptors which has to be restored. Currently open_proc PROC_SELF is not used during restoring file descriptors, but we are going to use it for memfd restore. Note: in get_proc_fd let's not close service fd if we detect service fd is not ours, it will be replaced in open_pid_proc anyway, and e.g. in protected sfd context we can't close or open sfd, but can replace it without any problems. While on it also add FIXME because the check in get_proc_fd is error prone in some rare cases (nested pidns is not supported yet). We need to populate this new SELF service fd in populate_pid_proc, so that it is later available in protected context. Also don't close /proc/self service fd in prep_unix_sk_cwd as it can be called from protected context and there is no much point of closing it anyway. Close proc self servicefd in close_old_fds, because first we don't wan't to reuse it from some ancestor, second there can be some junk fd as we are yet only in the beginning of close_old_fds. This junk fd can come from service fds of other tasks from parent's shared fdtable, and this fd would not allow us to do opendir_proc(PROC_SELF). Signed-off-by: Pavel Tikhomirov --- criu/cr-restore.c | 4 ++++ criu/files.c | 9 ++++++++- criu/include/servicefd.h | 2 ++ criu/include/util.h | 2 +- criu/servicefd.c | 10 ++++++++++ criu/sk-unix.c | 1 - criu/util.c | 29 ++++++++++++++++++++--------- 7 files changed, 45 insertions(+), 12 deletions(-) diff --git a/criu/cr-restore.c b/criu/cr-restore.c index 4d98a0790..19ab0c5ae 100644 --- a/criu/cr-restore.c +++ b/criu/cr-restore.c @@ -418,6 +418,10 @@ static int populate_pid_proc(void) pr_err("Can't open PROC_SELF\n"); return -1; } + if (open_pid_proc(PROC_SELF) < 0) { + pr_err("Can't open PROC_SELF\n"); + return -1; + } return 0; } diff --git a/criu/files.c b/criu/files.c index 0912d1a4d..f10527b59 100644 --- a/criu/files.c +++ b/criu/files.c @@ -1256,6 +1256,14 @@ int close_old_fds(void) struct dirent *de; int fd, ret; + /** + * Close previous /proc/self/ service fd, as we don't wan't to reuse it + * from a different task. Also there can be some junk fd in it's place + * after we've moved our service fds (e.g. from other task of parents + * shared fdtable), we need to close it before opendir_proc() below. + */ + __close_service_fd(PROC_SELF_FD_OFF); + dir = opendir_proc(PROC_SELF, "fd"); if (dir == NULL) return -1; @@ -1297,7 +1305,6 @@ int prepare_fds(struct pstree_item *me) sfds_protected = false; close_service_fd(CGROUP_YARD); sfds_protected = true; - set_proc_self_fd(-1); /* flush any proc cached fds we may have */ if (rsti(me)->fdt) { struct fdt *fdt = rsti(me)->fdt; diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h index c11f89d37..b03bdaf59 100644 --- a/criu/include/servicefd.h +++ b/criu/include/servicefd.h @@ -17,6 +17,7 @@ enum sfd_type { IMG_STREAMER_FD_OFF, PROC_FD_OFF, /* fd with /proc for all proc_ calls */ PROC_PID_FD_OFF, + PROC_SELF_FD_OFF, CR_PROC_FD_OFF, /* some other's proc fd: * - For dump -- target ns' proc * - For restore -- CRIU ns' proc @@ -44,6 +45,7 @@ extern bool is_service_fd(int fd, enum sfd_type type); extern int service_fd_min_fd(struct pstree_item *item); extern int install_service_fd(enum sfd_type type, int fd); extern int close_service_fd(enum sfd_type type); +extern void __close_service_fd(enum sfd_type type); extern int clone_service_fd(struct pstree_item *me); #endif /* __CR_SERVICE_FD_H__ */ diff --git a/criu/include/util.h b/criu/include/util.h index c2baf2788..7cb8c262c 100644 --- a/criu/include/util.h +++ b/criu/include/util.h @@ -180,7 +180,7 @@ extern int cr_daemon(int nochdir, int noclose, int close_fd); extern int status_ready(void); extern int is_root_user(void); -extern void set_proc_self_fd(int fd); +extern int set_proc_self_fd(int fd); static inline bool dir_dots(const struct dirent *de) { diff --git a/criu/servicefd.c b/criu/servicefd.c index 4aeb15360..952410a8d 100644 --- a/criu/servicefd.c +++ b/criu/servicefd.c @@ -46,6 +46,7 @@ const char *sfd_type_name(enum sfd_type type) [IMG_FD_OFF] = __stringify_1(IMG_FD_OFF), [PROC_FD_OFF] = __stringify_1(PROC_FD_OFF), [PROC_PID_FD_OFF] = __stringify_1(PROC_PID_FD_OFF), + [PROC_SELF_FD_OFF] = __stringify_1(PROC_SELF_FD_OFF), [CR_PROC_FD_OFF] = __stringify_1(CR_PROC_FD_OFF), [ROOT_FD_OFF] = __stringify_1(ROOT_FD_OFF), [CGROUP_YARD] = __stringify_1(CGROUP_YARD), @@ -206,6 +207,15 @@ int close_service_fd(enum sfd_type type) return 0; } +void __close_service_fd(enum sfd_type type) +{ + int fd; + + fd = __get_service_fd(type, service_fd_id); + close(fd); + clear_bit(type, sfd_map); +} + static int move_service_fd(struct pstree_item *me, int type, int new_id, int new_base) { int old = get_service_fd(type); diff --git a/criu/sk-unix.c b/criu/sk-unix.c index a3964081b..a5b931b2a 100644 --- a/criu/sk-unix.c +++ b/criu/sk-unix.c @@ -1211,7 +1211,6 @@ static int prep_unix_sk_cwd(struct unix_sk_info *ui, int *prev_cwd_fd, if (switch_ns_by_fd(ns_fd, &mnt_ns_desc, prev_mntns_fd)) return -1; - set_proc_self_fd(-1); close(ns_fd); } diff --git a/criu/util.c b/criu/util.c index 670ef158d..91add00f3 100644 --- a/criu/util.c +++ b/criu/util.c @@ -281,15 +281,18 @@ int move_fd_from(int *img_fd, int want_fd) static pid_t open_proc_pid = PROC_NONE; static pid_t open_proc_self_pid; -static int open_proc_self_fd = -1; -void set_proc_self_fd(int fd) +int set_proc_self_fd(int fd) { - if (open_proc_self_fd >= 0) - close(open_proc_self_fd); + int ret; + + if (fd < 0) + return close_service_fd(PROC_SELF_FD_OFF); - open_proc_self_fd = fd; open_proc_self_pid = getpid(); + ret = install_service_fd(PROC_SELF_FD_OFF, fd); + + return ret; } static inline int set_proc_pid_fd(int pid, int fd) @@ -308,10 +311,18 @@ static inline int set_proc_pid_fd(int pid, int fd) static inline int get_proc_fd(int pid) { if (pid == PROC_SELF) { - if (open_proc_self_fd != -1 && open_proc_self_pid != getpid()) { - close(open_proc_self_fd); + int open_proc_self_fd; + + open_proc_self_fd = get_service_fd(PROC_SELF_FD_OFF); + /** + * FIXME in case two processes from different pidnses have the + * same pid from getpid() and one inherited service fds from + * another or they share them by shared fdt - this check will + * not detect that one of them reuses /proc/self of another. + * Everything proc related may break in this case. + */ + if (open_proc_self_fd >= 0 && open_proc_self_pid != getpid()) open_proc_self_fd = -1; - } return open_proc_self_fd; } else if (pid == open_proc_pid) return get_service_fd(PROC_PID_FD_OFF); @@ -402,7 +413,7 @@ inline int open_pid_proc(pid_t pid) } if (pid == PROC_SELF) - set_proc_self_fd(fd); + fd = set_proc_self_fd(fd); else fd = set_proc_pid_fd(pid, fd);