restore: Wait for userns being prepared before moving into cgroups (v2)

Submitted by Andrei Vagin on April 20, 2016, 10:50 p.m.

Details

Message ID 1461192654-22028-1-git-send-email-avagin@openvz.org
State Rejected
Series "restore: Wait for userns being prepared before moving into cgroups"
Headers show

Patch hide | download patch | download mbox

diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index 34db7f7..a92b27b 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -1608,6 +1608,11 @@  static int restore_task_with_children(void *_arg)
 
 	current = ca->item;
 
+	if (ca->clone_flags & CLONE_NEWUSER) {
+		if (restore_finish_stage(CR_STATE_RESTORE_USERNS) < 0)
+			goto err;
+	}
+
 	if (current != root_item) {
 		char buf[12];
 		int fd;
@@ -1752,6 +1757,7 @@  static inline int stage_participants(int next_stage)
 	switch (next_stage) {
 	case CR_STATE_FAIL:
 		return 0;
+	case CR_STATE_RESTORE_USERNS:
 	case CR_STATE_RESTORE_NS:
 	case CR_STATE_RESTORE_SHARED:
 		return 1;
@@ -2047,6 +2053,9 @@  static int restore_root_task(struct pstree_item *init)
 	futex_set(&task_entries->nr_in_progress,
 			stage_participants(CR_STATE_RESTORE_NS));
 
+	if (root_ns_mask & CLONE_NEWUSER)
+		__restore_switch_stage(CR_STATE_RESTORE_USERNS);
+
 	ret = fork_with_pid(init);
 	if (ret < 0)
 		goto out;
@@ -2074,18 +2083,23 @@  static int restore_root_task(struct pstree_item *init)
 		}
 	}
 
-	/*
-	 * uid_map and gid_map must be filled from a parent user namespace.
-	 * prepare_userns_creds() must be called after filling mappings.
-	 */
-	if ((root_ns_mask & CLONE_NEWUSER) && prepare_userns(init))
-		goto out_kill;
-
 	pr_info("Wait until namespaces are created\n");
 	ret = restore_wait_inprogress_tasks();
 	if (ret)
 		goto out_kill;
 
+	/*
+	 * uid_map and gid_map must be filled from a parent user namespace.
+	 * prepare_userns_creds() must be called after filling mappings.
+	 */
+	if (root_ns_mask & CLONE_NEWUSER) {
+		if  (prepare_userns(init))
+			goto out_kill;
+		ret = restore_switch_stage(CR_STATE_RESTORE_NS);
+		if (ret < 0)
+			goto out_kill;
+	}
+
 	if (root_ns_mask & CLONE_NEWNS) {
 		mnt_ns_fd = open_proc(init->pid.real, "ns/mnt");
 		if (mnt_ns_fd < 0) {
diff --git a/criu/include/restorer.h b/criu/include/restorer.h
index 941955e..675c51e 100644
--- a/criu/include/restorer.h
+++ b/criu/include/restorer.h
@@ -213,7 +213,8 @@  static inline unsigned long restorer_stack(struct thread_restore_args *a)
 
 enum {
 	CR_STATE_FAIL		= -1,
-	CR_STATE_RESTORE_NS	= 0, /* is used for executing "setup-namespace" scripts */
+	CR_STATE_RESTORE_USERNS = 0,	/* restore uid_map and gid_map */
+	CR_STATE_RESTORE_NS,		/* is used for executing "setup-namespace" scripts */
 	CR_STATE_RESTORE_SHARED,
 	CR_STATE_FORKING,
 	CR_STATE_RESTORE,

Comments

Pavel Emelianov April 21, 2016, 12:28 p.m.
On 04/21/2016 01:50 AM, Andrey Vagin wrote:
> From: Andrew Vagin <avagin@virtuozzo.com>
> 
> When user namespace are stepping in we should wait for their preparation
> to complete before start using userns daemon (internally the kernel
> checks for uids and if uids are not set -EINVAL will be returned
> when usersn calls for sendmsg()).
> 
> Thus use completion and wait for uid maps being written first.

The commit message doesn't match the patch itself.

> @@ -213,7 +213,8 @@ static inline unsigned long restorer_stack(struct thread_restore_args *a)
>  
>  enum {
>  	CR_STATE_FAIL		= -1,
> -	CR_STATE_RESTORE_NS	= 0, /* is used for executing "setup-namespace" scripts */
> +	CR_STATE_RESTORE_USERNS = 0,	/* restore uid_map and gid_map */
> +	CR_STATE_RESTORE_NS,		/* is used for executing "setup-namespace" scripts */

Why not merge it with CR_STATE_RESTORE_NS?

>  	CR_STATE_RESTORE_SHARED,
>  	CR_STATE_FORKING,
>  	CR_STATE_RESTORE,
>
Cyrill Gorcunov April 21, 2016, 12:42 p.m.
On Thu, Apr 21, 2016 at 03:28:54PM +0300, Pavel Emelyanov wrote:
> The commit message doesn't match the patch itself.

These stages are used for completion.

> 
> > @@ -213,7 +213,8 @@ static inline unsigned long restorer_stack(struct thread_restore_args *a)
> >  
> >  enum {
> >  	CR_STATE_FAIL		= -1,
> > -	CR_STATE_RESTORE_NS	= 0, /* is used for executing "setup-namespace" scripts */
> > +	CR_STATE_RESTORE_USERNS = 0,	/* restore uid_map and gid_map */
> > +	CR_STATE_RESTORE_NS,		/* is used for executing "setup-namespace" scripts */
> 
> Why not merge it with CR_STATE_RESTORE_NS?

At least because it makes change more visible, and
moreover CR_STATE_RESTORE_NS already sync'ing
join-ns and such.
Pavel Emelianov April 21, 2016, 12:45 p.m.
On 04/21/2016 03:42 PM, Cyrill Gorcunov wrote:
> On Thu, Apr 21, 2016 at 03:28:54PM +0300, Pavel Emelyanov wrote:
>> The commit message doesn't match the patch itself.
> 
> These stages are used for completion.
> 
>>
>>> @@ -213,7 +213,8 @@ static inline unsigned long restorer_stack(struct thread_restore_args *a)
>>>  
>>>  enum {
>>>  	CR_STATE_FAIL		= -1,
>>> -	CR_STATE_RESTORE_NS	= 0, /* is used for executing "setup-namespace" scripts */
>>> +	CR_STATE_RESTORE_USERNS = 0,	/* restore uid_map and gid_map */
>>> +	CR_STATE_RESTORE_NS,		/* is used for executing "setup-namespace" scripts */
>>
>> Why not merge it with CR_STATE_RESTORE_NS?
> 
> At least because it makes change more visible,

:\

> and moreover CR_STATE_RESTORE_NS already sync'ing
> join-ns and such.

And... why not sync userns setup too?

-- Pavel
Cyrill Gorcunov April 21, 2016, 1:06 p.m.
On Thu, Apr 21, 2016 at 03:45:48PM +0300, Pavel Emelyanov wrote:
> On 04/21/2016 03:42 PM, Cyrill Gorcunov wrote:
> > On Thu, Apr 21, 2016 at 03:28:54PM +0300, Pavel Emelyanov wrote:
> >> The commit message doesn't match the patch itself.
> > 
> > These stages are used for completion.
> > 
> >>
> >>> @@ -213,7 +213,8 @@ static inline unsigned long restorer_stack(struct thread_restore_args *a)
> >>>  
> >>>  enum {
> >>>  	CR_STATE_FAIL		= -1,
> >>> -	CR_STATE_RESTORE_NS	= 0, /* is used for executing "setup-namespace" scripts */
> >>> +	CR_STATE_RESTORE_USERNS = 0,	/* restore uid_map and gid_map */
> >>> +	CR_STATE_RESTORE_NS,		/* is used for executing "setup-namespace" scripts */
> >>
> >> Why not merge it with CR_STATE_RESTORE_NS?
> > 
> > At least because it makes change more visible,
> 
> :\
> 
> > and moreover CR_STATE_RESTORE_NS already sync'ing
> > join-ns and such.
> 
> And... why not sync userns setup too?

As to me this look more convenient. But up to you and Andrew,
whatever works -- suits me just fine.