[v2] criu: Add --keep-on-exec option

Submitted by Cyrill Gorcunov on June 4, 2019, 7:41 a.m.

Details

Message ID 20190604074121.839-1-gorcunov@gmail.com
State New
Series "criu: Add --keep-on-exec option"
Headers show

Commit Message

Cyrill Gorcunov June 4, 2019, 7:41 a.m.
When we run action scripts we communicate with third-party
tools via file descriptors inherited from a criu execution
caller.

Since commit 6c8ea60491305482eb60d9b17a8635ab9248a5ab this
no longer possible, moreover it breaks API.

Thus lets provide --keep-on-exec option where we could
tell explicitly which exactly fds should be kept opened.

v2:
 - provide qsort_cmp_int_array to avoid code duplication

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 Documentation/criu.txt    |  7 +++++++
 criu/config.c             | 23 +++++++++++++++++++++++
 criu/include/cr_options.h |  2 ++
 criu/include/util.h       |  2 ++
 criu/util.c               | 23 +++++++++++++++++++++++
 5 files changed, 57 insertions(+)

Patch hide | download patch | download mbox

diff --git a/Documentation/criu.txt b/Documentation/criu.txt
index 6111c3baf0e1..5ffdd7cb1b28 100644
--- a/Documentation/criu.txt
+++ b/Documentation/criu.txt
@@ -138,6 +138,13 @@  Common options are applicable to any 'command'.
             notification message contains a file descriptor for
             the master pty
 
+*--keep-on-exec* 'fd'::
+    Keep 'fd' file descriptor when executing service programs and scripts
+    during certain stages. When *criu* is executed it inherits opened file
+    descriptors or a caller (if they are present). Sometimes it is needed
+    to pass such descriptors to the *--action-script* scripts, otherwise
+    they get closed for security reason.
+
 *-V*, *--version*::
     Print program version and exit.
 
diff --git a/criu/config.c b/criu/config.c
index 24f0b33858ad..a53f0c0ab6ce 100644
--- a/criu/config.c
+++ b/criu/config.c
@@ -416,6 +416,22 @@  static int parse_join_ns(const char *ptr)
 	return 0;
 }
 
+static int parse_keep_on_exec(char *optarg)
+{
+	size_t size = sizeof(opts.keep_on_exec[0]) * (opts.nr_keep_on_exec + 1);
+	int fd = atoi(optarg);
+
+	if (xrealloc_safe(&opts.keep_on_exec, size))
+		return -ENOMEM;
+
+	opts.keep_on_exec[opts.nr_keep_on_exec++] = fd;
+	qsort(opts.keep_on_exec, opts.nr_keep_on_exec,
+	      sizeof(opts.keep_on_exec[0]),
+	      qsort_cmp_int_array);
+
+	return 0;
+}
+
 /*
  * parse_options() is the point where the getopt parsing happens. The CLI
  * parsing as well as the configuration file parsing happens here.
@@ -511,6 +527,7 @@  int parse_options(int argc, char **argv, bool *usage_error,
 		BOOL_OPT("remote", &opts.remote),
 		{ "config",			required_argument,	0, 1089},
 		{ "no-default-config",		no_argument,		0, 1090},
+		{ "keep-on-exec",		required_argument,	0, 1092},
 		{ },
 	};
 
@@ -797,6 +814,12 @@  int parse_options(int argc, char **argv, bool *usage_error,
 		case 1091:
 			opts.ps_socket = atoi(optarg);
 			break;
+		case 1092:
+			if (parse_keep_on_exec(optarg)) {
+				pr_err("Unable to parse value for --keep-on-exec\n");
+				return 1;
+			}
+			break;
 		case 'V':
 			pr_msg("Version: %s\n", CRIU_VERSION);
 			if (strcmp(CRIU_GITID, "0"))
diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
index 8ddbf2341a48..2022ba12f8ec 100644
--- a/criu/include/cr_options.h
+++ b/criu/include/cr_options.h
@@ -90,6 +90,8 @@  struct cr_options {
 	struct list_head	inherit_fds;
 	struct list_head	external;
 	struct list_head	join_ns;
+	int			*keep_on_exec;
+	size_t			nr_keep_on_exec;
 	char			*libdir;
 	int			use_page_server;
 	unsigned short		port;
diff --git a/criu/include/util.h b/criu/include/util.h
index a14be72293be..f4d2171bca35 100644
--- a/criu/include/util.h
+++ b/criu/include/util.h
@@ -378,4 +378,6 @@  static inline void print_stack_trace(pid_t pid) {}
 		___ret;									\
 	})
 
+extern int qsort_cmp_int_array(const void *__a, const void *__b);
+
 #endif /* __CR_UTIL_H__ */
diff --git a/criu/util.c b/criu/util.c
index 31cdee1ff60f..3cfe7e02cce2 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -515,6 +515,20 @@  int cr_system(int in, int out, int err, char *cmd, char *const argv[], unsigned
 	return cr_system_userns(in, out, err, cmd, argv, flags, -1);
 }
 
+static bool should_keep_on_exec(int fd)
+{
+	if (!opts.nr_keep_on_exec)
+		return false;
+
+	if (bsearch(&fd, opts.keep_on_exec,
+		    opts.nr_keep_on_exec,
+		    sizeof(opts.keep_on_exec[0]),
+		    qsort_cmp_int_array))
+	    return true;
+
+	return false;
+}
+
 static int close_fds(int minfd)
 {
 	DIR *dir;
@@ -541,6 +555,8 @@  static int close_fds(int minfd)
 			continue;
 		if (fd < minfd)
 			continue;
+		if (should_keep_on_exec(fd))
+			continue;
 		close(fd);
 	}
 	closedir(dir);
@@ -1341,6 +1357,13 @@  void rlimit_unlimit_nofile(void)
 	service_fd_rlim_cur = kdat.sysctl_nr_open;
 }
 
+int qsort_cmp_int_array(const void *__a, const void *__b)
+{
+	int a = ((int *)__a)[0];
+	int b = ((int *)__b)[0];
+	return a == b ? 0 : (a < b ? -1 : 1);
+}
+
 
 #ifdef __GLIBC__
 #include <execinfo.h>

Comments

Andrei Vagin June 22, 2019, 3:42 a.m.
On Tue, Jun 04, 2019 at 10:41:21AM +0300, Cyrill Gorcunov wrote:
> When we run action scripts we communicate with third-party
> tools via file descriptors inherited from a criu execution
> caller.
> 
> Since commit 6c8ea60491305482eb60d9b17a8635ab9248a5ab this
> no longer possible, moreover it breaks API.
> 
> Thus lets provide --keep-on-exec option where we could
> tell explicitly which exactly fds should be kept opened.
> 
> v2:
>  - provide qsort_cmp_int_array to avoid code duplication

We need a test for this functionality...

> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  Documentation/criu.txt    |  7 +++++++
>  criu/config.c             | 23 +++++++++++++++++++++++
>  criu/include/cr_options.h |  2 ++
>  criu/include/util.h       |  2 ++
>  criu/util.c               | 23 +++++++++++++++++++++++
>  5 files changed, 57 insertions(+)
> 
> diff --git a/Documentation/criu.txt b/Documentation/criu.txt
> index 6111c3baf0e1..5ffdd7cb1b28 100644
> --- a/Documentation/criu.txt
> +++ b/Documentation/criu.txt
> @@ -138,6 +138,13 @@ Common options are applicable to any 'command'.
>              notification message contains a file descriptor for
>              the master pty
>  
> +*--keep-on-exec* 'fd'::
> +    Keep 'fd' file descriptor when executing service programs and scripts
> +    during certain stages. When *criu* is executed it inherits opened file
> +    descriptors or a caller (if they are present). Sometimes it is needed
> +    to pass such descriptors to the *--action-script* scripts, otherwise
> +    they get closed for security reason.
> +
>  *-V*, *--version*::
>      Print program version and exit.
>  
> diff --git a/criu/config.c b/criu/config.c
> index 24f0b33858ad..a53f0c0ab6ce 100644
> --- a/criu/config.c
> +++ b/criu/config.c
> @@ -416,6 +416,22 @@ static int parse_join_ns(const char *ptr)
>  	return 0;
>  }
>  
> +static int parse_keep_on_exec(char *optarg)
> +{
> +	size_t size = sizeof(opts.keep_on_exec[0]) * (opts.nr_keep_on_exec + 1);
> +	int fd = atoi(optarg);

atoi() does not detect errors. You can use strtol or sscanf.

> +
> +	if (xrealloc_safe(&opts.keep_on_exec, size))
> +		return -ENOMEM;
> +
> +	opts.keep_on_exec[opts.nr_keep_on_exec++] = fd;
> +	qsort(opts.keep_on_exec, opts.nr_keep_on_exec,
> +	      sizeof(opts.keep_on_exec[0]),
> +	      qsort_cmp_int_array);
> +
> +	return 0;
> +}
> +
>  /*
>   * parse_options() is the point where the getopt parsing happens. The CLI
>   * parsing as well as the configuration file parsing happens here.
> @@ -511,6 +527,7 @@ int parse_options(int argc, char **argv, bool *usage_error,
>  		BOOL_OPT("remote", &opts.remote),
>  		{ "config",			required_argument,	0, 1089},
>  		{ "no-default-config",		no_argument,		0, 1090},
> +		{ "keep-on-exec",		required_argument,	0, 1092},
>  		{ },
>  	};
>  
> @@ -797,6 +814,12 @@ int parse_options(int argc, char **argv, bool *usage_error,
>  		case 1091:
>  			opts.ps_socket = atoi(optarg);
>  			break;
> +		case 1092:
> +			if (parse_keep_on_exec(optarg)) {
> +				pr_err("Unable to parse value for --keep-on-exec\n");
> +				return 1;
> +			}
> +			break;
>  		case 'V':
>  			pr_msg("Version: %s\n", CRIU_VERSION);
>  			if (strcmp(CRIU_GITID, "0"))
> diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
> index 8ddbf2341a48..2022ba12f8ec 100644
> --- a/criu/include/cr_options.h
> +++ b/criu/include/cr_options.h
> @@ -90,6 +90,8 @@ struct cr_options {
>  	struct list_head	inherit_fds;
>  	struct list_head	external;
>  	struct list_head	join_ns;
> +	int			*keep_on_exec;
> +	size_t			nr_keep_on_exec;
>  	char			*libdir;
>  	int			use_page_server;
>  	unsigned short		port;
> diff --git a/criu/include/util.h b/criu/include/util.h
> index a14be72293be..f4d2171bca35 100644
> --- a/criu/include/util.h
> +++ b/criu/include/util.h
> @@ -378,4 +378,6 @@ static inline void print_stack_trace(pid_t pid) {}
>  		___ret;									\
>  	})
>  
> +extern int qsort_cmp_int_array(const void *__a, const void *__b);
> +
>  #endif /* __CR_UTIL_H__ */
> diff --git a/criu/util.c b/criu/util.c
> index 31cdee1ff60f..3cfe7e02cce2 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -515,6 +515,20 @@ int cr_system(int in, int out, int err, char *cmd, char *const argv[], unsigned
>  	return cr_system_userns(in, out, err, cmd, argv, flags, -1);
>  }
>  
> +static bool should_keep_on_exec(int fd)
> +{
> +	if (!opts.nr_keep_on_exec)
> +		return false;
> +
> +	if (bsearch(&fd, opts.keep_on_exec,
> +		    opts.nr_keep_on_exec,
> +		    sizeof(opts.keep_on_exec[0]),
> +		    qsort_cmp_int_array))
> +	    return true;
> +
> +	return false;
> +}
> +
>  static int close_fds(int minfd)
>  {
>  	DIR *dir;
> @@ -541,6 +555,8 @@ static int close_fds(int minfd)
>  			continue;
>  		if (fd < minfd)
>  			continue;
> +		if (should_keep_on_exec(fd))
> +			continue;
>  		close(fd);
>  	}
>  	closedir(dir);
> @@ -1341,6 +1357,13 @@ void rlimit_unlimit_nofile(void)
>  	service_fd_rlim_cur = kdat.sysctl_nr_open;
>  }
>  
> +int qsort_cmp_int_array(const void *__a, const void *__b)
> +{
> +	int a = ((int *)__a)[0];
> +	int b = ((int *)__b)[0];
> +	return a == b ? 0 : (a < b ? -1 : 1);
> +}
> +
>  
>  #ifdef __GLIBC__
>  #include <execinfo.h>
> -- 
> 2.20.1
>