[CRIU] [PATCH REBASED 3/3] ns: Make pid_ns helpers as children of criu main process

[Kirill Tkhai]

On 11.07.2017 20:34, Andrei Vagin wrote:
> On Tue, Jul 11, 2017 at 12:02:27PM +0300, Kirill Tkhai wrote:
>> On 11.07.2017 03:46, Andrei Vagin wrote:
>>> On Thu, Jul 06, 2017 at 02:14:17PM +0300, Kirill Tkhai wrote:
>>>> If a task, holding userns_sync_lock unexpectedly exits,
>>>> criu will hang on error path in restore_root_task(),
>>>> because it can't use usernsd to destroy them.
>>>>
>>>> Lets remove the intermediary: we'll create pid_ns helpers
>>>> as children of criu main task, and criu main task will
>>>> be able to use simple kill() to stop them.
>>>>
>>>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>>>> ---
>>>>  criu/cr-restore.c |    8 ++++++++
>>>>  criu/namespaces.c |   35 +++--------------------------------
>>>>  2 files changed, 11 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
>>>> index 965770f48..8d8349f32 100644
>>>> --- a/criu/cr-restore.c
>>>> +++ b/criu/cr-restore.c
>>>> @@ -1470,6 +1470,7 @@ static inline int fork_with_pid(struct pstree_item *item)
>>>>  static void sigchld_handler(int signal, siginfo_t *siginfo, void *data)
>>>>  {
>>>>  	int status, pid, exit;
>>>> +	struct ns_id *ns;
>>>>  
>>>>  	while (1) {
>>>>  		pid = waitpid(-1, &status, WNOHANG);
>>>> @@ -1489,6 +1490,13 @@ static void sigchld_handler(int signal, siginfo_t *siginfo, void *data)
>>>>  		break;
>>>>  	}
>>>>  
>>>> +	if (!current) {
>>>> +		ns = handle_pid_ns_helper_exit(pid);
>>>
>>> Why is it out of the previous loop? What if two helpers exit together?
>>
>> As we destroy the helpers with SIGCHLD blocked in do_destroy_pidns_helpers(),
>> a seeing of a one helper is already a error, and we should stop restore
>> in this case.
>>
>> We mustn't see a helper in this function as we newer mustn't see any task
>> there. The helper handle_pid_ns_helper_exit() is only for printing debug
>> message to understand what happened.
> 
> Ok, I see. Pls, add something like this ^^^ in a comment before this
> code. Thanks!

Ok, I'll add comments and will change function name for better redability.

>>  
>>>> +		if (ns)
>>>> +			pr_err("Unexpected pid ns helper: pid=%d, ns_id=%d\n",
>>>> +				pid, ns->id);
>>>> +	}
>>>> +
>>>>  	if (exit)
>>>>  		pr_err("%d exited, status=%d\n", pid, status);
>>>>  	else
>>> 		...
>>>
>>> 	futex_abort_and_wake(&task_entries->nr_in_progress);
>>>
>>> it means that we always abort "restore", doesn't it?
>>
>> Yes, we break restore in this case.
>>  
>>>> diff --git a/criu/namespaces.c b/criu/namespaces.c
>>>> index 674b43820..fca125b6a 100644
>>>> --- a/criu/namespaces.c
>>>> +++ b/criu/namespaces.c
>>>> @@ -1603,26 +1603,6 @@ struct ns_id *handle_pid_ns_helper_exit(pid_t real_pid)
>>>>  	return ns;
>>>>  }
>>>>  
>>>> -static void usernsd_handler(int signal, siginfo_t *siginfo, void *data)
>>>> -{
>>>> -	pid_t pid = siginfo->si_pid;
>>>> -	struct ns_id *ns;
>>>> -	int status;
>>>> -
>>>> -	while (pid) {
>>>> -		pid = waitpid(-1, &status, WNOHANG);
>>>> -		if (pid <= 0)
>>>> -			return;
>>>> -
>>>> -		ns = handle_pid_ns_helper_exit(pid);
>>>> -		if (!ns)
>>>> -			pr_err("Spurious pid ns helper: pid=%d\n", pid);
>>>> -
>>>> -		pr_err("%d finished unexpected: status=%d\n", pid, status);
>>>> -		futex_abort_and_wake(&task_entries->nr_in_progress);
>>>> -	}
>>>> -}
>>>> -
>>>>  static int usernsd_recv_transport(void *arg, int fd, pid_t pid)
>>>>  {
>>>>  	if (install_service_fd(TRANSPORT_FD_OFF, fd) < 0) {
>>>> @@ -1666,11 +1646,6 @@ static int usernsd(int sk)
>>>>  {
>>>>  	pr_info("uns: Daemon started\n");
>>>>  
>>>> -	if (criu_signals_setup(usernsd_handler) < 0) {
>>>> -		pr_err("Can't setup handler\n");
>>>> -		return -1;
>>>> -	}
>>>> -
>>>>  	while (1) {
>>>>  		struct unsc_msg um;
>>>>  		static char msg[MAX_UNSFD_MSG_SIZE];
>>>> @@ -2725,7 +2700,7 @@ static int do_create_pid_ns_helper(void *arg, int sk, pid_t unused_pid)
>>>>  			unlock_last_pid();
>>>>  			goto restore_ns;
>>>>  		}
>>>> -	child = fork();
>>>> +	child = sys_clone_unified(CLONE_PARENT|SIGCHLD, NULL, NULL, NULL, 0);
>>>>  	if (!child)
>>>>  		exit(pid_ns_helper(ns, sk));
>>>>  	saved_errno = errno;
>>>> @@ -2787,7 +2762,7 @@ int create_pid_ns_helper(struct ns_id *ns)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static int do_destroy_pid_ns_helpers(void *arg, int fd, pid_t unused)
>>>> +static int do_destroy_pid_ns_helpers(void)
>>>>  {
>>>>  	int status, sig_blocked = true, ret = 0;
>>>>  	sigset_t sig_mask;
>>>> @@ -2836,11 +2811,7 @@ int destroy_pid_ns_helpers(void)
>>>>  	if (!(root_ns_mask & CLONE_NEWPID))
>>>>  		return 0;
>>>>  
>>>> -	if (userns_call(do_destroy_pid_ns_helpers, 0, NULL, 0, -1) < 0) {
>>>> -		pr_err("Can't create pid_ns helper\n");
>>>> -		return -1;
>>>> -	}
>>>> -	return 0;
>>>> +	return do_destroy_pid_ns_helpers();
>>>>  }
>>>>  
>>>>  struct ns_desc pid_ns_desc = NS_DESC_ENTRY(CLONE_NEWPID, "pid", "pid_for_children");
>>>>