[CRIU] [PATCH 4/4] cr-service: use inherit-fd keys to ask for FD
Adrian Reber
areber at redhat.com
Wed Aug 8 12:28:51 MSK 2018
On Tue, Aug 07, 2018 at 10:47:02PM -0700, Andrei Vagin wrote:
> 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.
Yes, that would be a really nice and easy solution. But 'criu service'
breaks all this.
> BTW: Do you remember when we decided that inherit_fd isn't supported for
> the swrk mode?
What do you mean? This:
if (req->n_inherit_fd && !opts.swrk_restore) {
pr_err("inherit_fd is not allowed in standalone service\n");
goto err;
}
Doesn't this mean it is only supported in swrk mode? So with my patches
it would work also in 'criu service' mode, but it is much more
complicated. Which is questionable as it seems nobody seems to use it in
service 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".
So do we care about about 'criu service'? My feeling is, not really.
So maybe (almost) not any patches are necessary for CRIU to make it work
with runc (especially for the external network namespaces use case I am
interested in).
> > > > 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