[v3] restore: Block SIGCHLD during root_item initialization

Submitted by Kirill Tkhai on April 6, 2017, 10:07 a.m.

Details

Message ID 149147320843.26174.8691927941652961686.stgit@localhost.localdomain
State New
Series "restore: Block SIGCHLD during root_item initialization"
Headers show

Commit Message

Kirill Tkhai April 6, 2017, 10:07 a.m.
(Was "user_ns: Block SIGCHLD during namespaces generation")

We don't want asynchronous signal handler during creation
of namespaces (for example, in create_user_ns_hierarhy())
as we do wait() synchronous. So we need to block the signal.
Do this once globally (till creation of real children of
root_item).

v2: Set initial ret = 0
v3: Block signal globally in root_item before its children
are created.

https://travis-ci.org/tkhai/criu

Suggested-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 criu/cr-restore.c   |   11 +++++++++++
 criu/include/util.h |   21 +++++++++++++++++++++
 2 files changed, 32 insertions(+)

Patch hide | download patch | download mbox

diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index 7842e169e..2ae2fd172 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -1326,6 +1326,8 @@  static int create_children_and_session(void)
 static int restore_task_with_children(void *_arg)
 {
 	struct cr_clone_arg *ca = _arg;
+	bool sig_blocked = false;
+	sigset_t sig_mask;
 	pid_t pid;
 	int ret;
 
@@ -1337,6 +1339,10 @@  static int restore_task_with_children(void *_arg)
 				current->pid->real, vpid(current));
 		if (current->pid->real < 0)
 			goto err;
+	} else {
+		if (block_sigmask(&sig_mask, SIGCHLD) < 0)
+			goto err;
+		sig_blocked = true;
 	}
 
 	if ( !(ca->clone_flags & CLONE_FILES))
@@ -1438,6 +1444,9 @@  static int restore_task_with_children(void *_arg)
 		BUG();
 	}
 
+	if (sig_blocked && restore_sigmask(&sig_mask) < 0)
+		goto err;
+
 	if (create_children_and_session())
 		goto err;
 
@@ -1469,6 +1478,8 @@  static int restore_task_with_children(void *_arg)
 	return 0;
 
 err:
+	if (sig_blocked)
+		restore_sigmask(&sig_mask);
 	if (current->parent == NULL)
 		futex_abort_and_wake(&task_entries->nr_in_progress);
 	exit(1);
diff --git a/criu/include/util.h b/criu/include/util.h
index 0428490bd..91587ef1c 100644
--- a/criu/include/util.h
+++ b/criu/include/util.h
@@ -311,4 +311,25 @@  extern int open_fd_of_real_pid(pid_t pid, int fd, int flags);
 
 extern int call_in_child_process(int (*fn)(void *), void *arg);
 
+#define block_sigmask(saved_mask, sig_mask)	({					\
+		sigset_t ___blocked_mask;						\
+		int ___ret = 0;								\
+		sigemptyset(&___blocked_mask);						\
+		sigaddset(&___blocked_mask, sig_mask);					\
+		if (sigprocmask(SIG_BLOCK, &___blocked_mask, saved_mask) == -1) {	\
+			pr_perror("Can not set mask of blocked signals");		\
+			___ret = -1;							\
+		}									\
+		___ret;									\
+	})
+
+#define restore_sigmask(saved_mask)	({						\
+		int ___ret = 0;								\
+		if (sigprocmask(SIG_SETMASK, saved_mask, NULL) == -1) {			\
+			pr_perror("Can not unset mask of blocked signals");		\
+			___ret = -1;							\
+		}									\
+		___ret;									\
+	})
+
 #endif /* __CR_UTIL_H__ */

Comments

Andrey Vagin April 7, 2017, 4:47 a.m.
On Thu, Apr 06, 2017 at 01:07:21PM +0300, Kirill Tkhai wrote:
> (Was "user_ns: Block SIGCHLD during namespaces generation")
> 
> We don't want asynchronous signal handler during creation
> of namespaces (for example, in create_user_ns_hierarhy())
> as we do wait() synchronous. So we need to block the signal.
> Do this once globally (till creation of real children of
> root_item).
> 
> v2: Set initial ret = 0
> v3: Block signal globally in root_item before its children
> are created.
> 
> https://travis-ci.org/tkhai/criu
> 
> Suggested-by: Andrew Vagin <avagin@virtuozzo.com>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  criu/cr-restore.c   |   11 +++++++++++
>  criu/include/util.h |   21 +++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index 7842e169e..2ae2fd172 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -1326,6 +1326,8 @@ static int create_children_and_session(void)
>  static int restore_task_with_children(void *_arg)
>  {
>  	struct cr_clone_arg *ca = _arg;
> +	bool sig_blocked = false;
> +	sigset_t sig_mask;
>  	pid_t pid;
>  	int ret;
>  
> @@ -1337,6 +1339,10 @@ static int restore_task_with_children(void *_arg)
>  				current->pid->real, vpid(current));
>  		if (current->pid->real < 0)
>  			goto err;
> +	} else {
> +		if (block_sigmask(&sig_mask, SIGCHLD) < 0)
> +			goto err;
> +		sig_blocked = true;

Why do you block signals here.

I think it should be smth like this:

@@ -1416,10 +1411,14 @@ static int restore_task_with_children(void *_arg)
                                return -1;
                }
 
+               if (block_sigmask(&sig_mask, SIGCHLD) < 0)
+                       goto err;
 
                if (prepare_namespace(current, ca->clone_flags))
                        goto err;
 
+               restore_sigmask(&sig_mask);
+
                if (restore_finish_stage(task_entries, CR_STATE_POST_RESTORE_NS) < 0)
                        goto err;
 

>  	}
>  
>  	if ( !(ca->clone_flags & CLONE_FILES))
> @@ -1438,6 +1444,9 @@ static int restore_task_with_children(void *_arg)
>  		BUG();
>  	}
>  
> +	if (sig_blocked && restore_sigmask(&sig_mask) < 0)
> +		goto err;
> +
>  	if (create_children_and_session())
>  		goto err;
>  
> @@ -1469,6 +1478,8 @@ static int restore_task_with_children(void *_arg)
>  	return 0;
>  
>  err:
> +	if (sig_blocked)
> +		restore_sigmask(&sig_mask);
>  	if (current->parent == NULL)
>  		futex_abort_and_wake(&task_entries->nr_in_progress);
>  	exit(1);
> diff --git a/criu/include/util.h b/criu/include/util.h
> index 0428490bd..91587ef1c 100644
> --- a/criu/include/util.h
> +++ b/criu/include/util.h
> @@ -311,4 +311,25 @@ extern int open_fd_of_real_pid(pid_t pid, int fd, int flags);
>  
>  extern int call_in_child_process(int (*fn)(void *), void *arg);
>  
> +#define block_sigmask(saved_mask, sig_mask)	({					\

I think static inline functions look better
> +		sigset_t ___blocked_mask;						\
> +		int ___ret = 0;								\
> +		sigemptyset(&___blocked_mask);						\
> +		sigaddset(&___blocked_mask, sig_mask);					\
> +		if (sigprocmask(SIG_BLOCK, &___blocked_mask, saved_mask) == -1) {	\
> +			pr_perror("Can not set mask of blocked signals");		\
> +			___ret = -1;							\
> +		}									\
> +		___ret;									\
> +	})
> +
> +#define restore_sigmask(saved_mask)	({						\
> +		int ___ret = 0;								\
> +		if (sigprocmask(SIG_SETMASK, saved_mask, NULL) == -1) {			\
> +			pr_perror("Can not unset mask of blocked signals");		\
> +			___ret = -1;							\
> +		}									\
> +		___ret;									\
> +	})
> +
>  #endif /* __CR_UTIL_H__ */
>