[CRIU] [PATCH 4/4] cr-service: use inherit-fd keys to ask for FD
Andrei Vagin
avagin at virtuozzo.com
Wed Aug 8 08:47:02 MSK 2018
On Tue, Aug 07, 2018 at 10:44:01PM +0200, Adrian Reber wrote:
> On Mon, Aug 06, 2018 at 02:01:05PM -0700, Andrei Vagin wrote:
> > On Mon, Aug 06, 2018 at 04:55:00PM +0000, Adrian Reber wrote:
> > > From: Adrian Reber <areber at redhat.com>
> > >
> > > Using inherit-fd in RPC mode uses the notify mechanism to ask the RPC
> > > client for the real FD.
> > >
> > > The basic workflow of inherit-fd in RPC mode is:
> > >
> > > * RPC clients fills in inherit-fd field:
> > > * inheritFd.Key = proto.String(path.Base(nsPath))
> > > * inheritFd.Fd is irrelevant as it will be transmitted later
> > > * CRIU receives RPC messages and calls inherit_fd_add_rpc()
> > > * this transmits the inheritFd.Key back to the RPC client
> > > * RPC client uses unix domain socket to send the real FD
> > > to CRIU
> > >
> > > Unfortunately this is more complicated as runc already uses
> > > inheritFd.Fd to send the file descriptors for 0, 1, 2. As those are
> > > always stdin, stdout, stderr is just worked even if the transmitted
> > > values via RPC were not actual FDs but only numbers.
> > >
> > > To handle this, CRIU still accepts inherit-fds directly as FDs if it is
> > > 0, 1, 2. This is a compatibility hack to avoid breaking runc.
> > >
> > > Signed-off-by: Adrian Reber <areber at redhat.com>
> > > ---
> > > criu/action-scripts.c | 10 ++++++---
> > > criu/cr-service.c | 38 ++++++++++++++++++++++++++++++++---
> > > criu/files.c | 10 +++++++++
> > > criu/include/action-scripts.h | 4 ++--
> > > criu/include/files.h | 1 +
> > > criu/tty.c | 2 +-
> > > 6 files changed, 56 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/criu/action-scripts.c b/criu/action-scripts.c
> > > index 4e9eb65cf..1a7523527 100644
> > > --- a/criu/action-scripts.c
> > > +++ b/criu/action-scripts.c
> > > @@ -100,7 +100,11 @@ static int run_shell_scripts(const char *action)
> > > return retval;
> > > }
> > >
> > > -int rpc_send_fd(enum script_actions act, int fd)
> > > +/*
> > > + * The name of this function is misleading. Right now it only
> > > + * sends an FD in one of three possible use cases.
> > > + */
> > > +int rpc_send_fd(enum script_actions act, int fd, char *key)
> > > {
> > > const char *action = action_names[act];
> > > int rpc_sk;
> > > @@ -113,7 +117,7 @@ int rpc_send_fd(enum script_actions act, int fd)
> > > return -1;
> > >
> > > pr_debug("\tRPC\n");
> > > - return send_criu_rpc_script(act, (char *)action, rpc_sk, fd);
> > > + return send_criu_rpc_script(act, (char *)action, rpc_sk, fd, key);
> > > }
> > >
> > > int run_scripts(enum script_actions act)
> > > @@ -127,7 +131,7 @@ int run_scripts(enum script_actions act)
> > > return 0;
> > >
> > > if (scripts_mode == SCRIPTS_RPC) {
> > > - ret = rpc_send_fd(act, -1);
> > > + ret = rpc_send_fd(act, -1, NULL);
> > > goto out;
> > > }
> > >
> > > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > > index 643aba9cf..45d8b3f44 100644
> > > --- a/criu/cr-service.c
> > > +++ b/criu/cr-service.c
> > > @@ -188,9 +188,10 @@ int send_criu_restore_resp(int socket_fd, bool success, int pid)
> > > return send_criu_msg(socket_fd, &msg);
> > > }
> > >
> > > -int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > > +int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd, char *key)
> > > {
> > > int ret;
> > > + int ret_fd = 0;
> > > CriuResp msg = CRIU_RESP__INIT;
> > > CriuReq *req;
> > > CriuNotify cn = CRIU_NOTIFY__INIT;
> > > @@ -211,6 +212,15 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > > cn.has_pid = true;
> > > cn.pid = root_item->pid->real;
> > > break;
> > > + case ACT_REQ_INHERIT_FD:
> > > + /*
> > > + * Sending a 'ACT_REQ_INHERIT_FD' notify message
> > > + * only makes sense when 'key' is set.
> > > + */
> > > + if (!key)
> > > + return -1;
> > > + cn.inherit_fd_key = key;
> > > + break;
> > > default:
> > > break;
> > > }
> > > @@ -219,6 +229,18 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > > if (ret < 0)
> > > return ret;
> > >
> > > + if (act == ACT_REQ_INHERIT_FD) {
> > > + /*
> > > + * Sending a ACT_REQ_INHERIT_FD notify message means, that CRIU
> > > + * expects to get a FD from the RPC client.
> > > + */
> > > + ret_fd = recv_fd(sk);
> > > + if (ret_fd <= 0) {
> > > + pr_perror("recv_fd error\n");
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > ret = recv_criu_msg(sk, &req);
> > > if (ret < 0)
> > > return ret;
> > > @@ -229,7 +251,7 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > > }
> > >
> > > criu_req__free_unpacked(req, NULL);
> > > - return 0;
> > > + return ret_fd;
> > > }
> > >
> > > static char images_dir[PATH_MAX];
> > > @@ -481,11 +503,21 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> > > }
> > >
> > > if (req->n_inherit_fd && !opts.swrk_restore) {
> > > + /* TODO: Is this really still true, even with callback handling for inherit_fd */
> > > pr_err("inherit_fd is not allowed in standalone service\n");
> > > goto err;
> > > }
> > > for (i = 0; i < req->n_inherit_fd; i++) {
> > > - if (inherit_fd_add(req->inherit_fd[i]->fd, req->inherit_fd[i]->key))
> > > + /*
> > > + * For the FDs 0, 1, 2 this falls back to old, non callbacb
> > > + * inherit_fd mode. At some point in the future this can be removed.
> > > + */
> > > + if (req->inherit_fd[i]->fd >=0 && req->inherit_fd[i]->fd <= 2) {
> >
> > We can add the following patch and handle all non-negative fd
> >
> > diff --git a/images/rpc.proto b/images/rpc.proto
> > index 33ef3302b..04b933674 100644
> > --- a/images/rpc.proto
> > +++ b/images/rpc.proto
> > @@ -25,7 +25,7 @@ message join_namespace {
> >
> > message inherit_fd {
> > required string key = 1;
> > - required int32 fd = 2;
> > + required int32 fd = 2 [default = -1];
> > };
> >
> > message cgroup_root {
>
> Hmm, how could this be used? With my current CRIU and runc patches, CRIU
> would look at 'fd' at only do something special if it is 0, 1, 2. See
> below. If it is > 2 or < 0 would be the same. CRIU just ignores all
> other FD values as they have to be retrieved via the new callback.
>
> > > + if (inherit_fd_add(req->inherit_fd[i]->fd, req->inherit_fd[i]->key))
> > > + goto err;
> > > + continue;
> > > + }
> > > + if (inherit_fd_add_rpc(req->inherit_fd[i]->key))
> >
> > I think you misunderstood me. I was thinking to request an inherit fd from
> > inherit_fd_lookup_id() to avoid fd clashes on restore
>
> I still do not get it. How is this related to inherit_fd_lookup_id()
>
> > (inherit_fd_resolve_clash). It will work this way too, but in case of
> > swrk, it will be more efficiant just to run criu swrk will all required
> > fds (this way is handled in a previous "if").
>
> I also do not understand this. 'criu swrk with all required fds'? Like
> runc starts CRIU in swrk mode, with an additional parameter as an FD?
>
> Right now runc does 'criu swrk 3'. Where '3' is the RPC data channel,
> right? And you say to add more FDs after the '3'?
No. I'm trying to say a different thing.
When we start a new process, it inherits all file descriptors from a
parent for which the FD_CLOEXEC bit isn't set. So it is possible to
start a criu swrk process which will have all required descriptors from
a parent process and then we will only need to send their numbers in an
rpc message.
BTW: Do you remember when we decided that inherit_fd isn't supported for
the swrk mode?
>
> And what do you mean with 'this way is handled in a previous "if"'? No
> idea to what this refers?
>
> Sorry, I am lost.
It may be my fault. I have to remember that inherit_fd works fine for
criu swrk and actually we only have an issue with "criu service".
>
> Adrian
>
> > > goto err; }
> > >
> > > diff --git a/criu/files.c b/criu/files.c index
> > > 4ed84305e..6d71bb9df 100644 --- a/criu/files.c +++ b/criu/files.c
> > > @@ -26,6 +26,7 @@ #include "sockets.h" #include "pstree.h"
> > > #include "tty.h" +#include "action-scripts.h" #include "pipes.h"
> > > #include "fifo.h" #include "eventfd.h" @@ -1572,6 +1573,15 @@ int
> > > inherit_fd_parse(char *optarg) return inherit_fd_add(fd, cp); }
> > >
> > > +int inherit_fd_add_rpc(char *key) +{ + int fd; + fd =
> > > rpc_send_fd(ACT_REQ_INHERIT_FD, -1, key); + if (fd <= 0) +
> > > return -1; + return inherit_fd_add(fd, key); +} + int
> > > inherit_fd_add(int fd, char *key) { struct inherit_fd *inh; diff
> > > --git a/criu/include/action-scripts.h
> > > b/criu/include/action-scripts.h index 4b90feb92..bd96e256d 100644
> > > --- a/criu/include/action-scripts.h +++
> > > b/criu/include/action-scripts.h @@ -23,7 +23,7 @@ enum
> > > script_actions { extern int add_script(char *path); extern int
> > > add_rpc_notify(int sk); extern int run_scripts(enum
> > > script_actions); -extern int rpc_send_fd(enum script_actions, int
> > > fd); -extern int send_criu_rpc_script(enum script_actions act,
> > > char *name, int sk, int fd); +extern int rpc_send_fd(enum
> > > script_actions, int fd, char *key); +extern int
> > > send_criu_rpc_script(enum script_actions act, char *name, int sk,
> > > int fd, char *key);
> > >
> > > #endif /* __CR_ACTION_SCRIPTS_H__ */ diff --git
> > > a/criu/include/files.h b/criu/include/files.h index
> > > 75757bd1d..9cffe960c 100644 --- a/criu/include/files.h +++
> > > b/criu/include/files.h @@ -188,6 +188,7 @@ extern int
> > > dump_unsupp_fd(struct fd_parms *p, int lfd, char *more, char
> > > *info, FdinfoEntry *);
> > >
> > > extern int inherit_fd_parse(char *optarg); +extern int
> > > inherit_fd_add_rpc(char *key); extern int inherit_fd_add(int fd,
> > > char *key); extern void inherit_fd_log(void); extern int
> > > inherit_fd_resolve_clash(int fd); diff --git a/criu/tty.c
> > > b/criu/tty.c index 30e3d7288..676ad888c 100644 --- a/criu/tty.c
> > > +++ b/criu/tty.c @@ -984,7 +984,7 @@ static int
> > > pty_open_unpaired_slave(struct file_desc *d, struct tty_info
> > > *slave) unlock_pty(master);
> > >
> > > if (opts.orphan_pts_master && -
> > > rpc_send_fd(ACT_ORPHAN_PTS_MASTER, master) == 0)
> > > { +
> > > rpc_send_fd(ACT_ORPHAN_PTS_MASTER, master, NULL)
> > > == 0) {
> > >
> > > fd = open_tty_reg(slave->reg_d,
> > > slave->tfe->flags); if (fd < 0) { --
> > > 2.18.0
> > >
More information about the CRIU
mailing list