[2/3] ns: Set NS_ROOT namespaces after tasks are read

Submitted by Kirill Tkhai on Feb. 28, 2017, 1:41 p.m.

Details

Message ID 148828931242.1058.6938503600569029991.stgit@localhost.localdomain
State New
Series "Fix "NS_ROOT is not assigned""
Headers show

Commit Message

Kirill Tkhai Feb. 28, 2017, 1:41 p.m.
Namespaces are read in read_ns_with_hookups(),
when tasks are not read. So, root_item is NULL,
and NS_ROOT is not set for appropriate namespaces.

This patch fixes NS_ROOT after tasks are read. Also
it adds uts, ipc and cgroup namespaces for uniformity.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 criu/include/namespaces.h |    1 +
 criu/namespaces.c         |   33 +++++++++++++++++++++++++++++----
 criu/pstree.c             |   17 +++++++++++++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/include/namespaces.h b/criu/include/namespaces.h
index 2a3dc085b..05c08b52e 100644
--- a/criu/include/namespaces.h
+++ b/criu/include/namespaces.h
@@ -149,6 +149,7 @@  extern int collect_mnt_namespaces(bool for_dump);
 extern int dump_mnt_namespaces(void);
 extern int dump_namespaces(struct pstree_item *item, unsigned int ns_flags);
 extern int read_ns_with_hookups(void);
+extern int set_ns_roots(void);
 extern int prepare_namespace_before_tasks(void);
 extern int prepare_namespace(struct pstree_item *item, unsigned long clone_flags);
 
diff --git a/criu/namespaces.c b/criu/namespaces.c
index b1e65df43..a92477df0 100644
--- a/criu/namespaces.c
+++ b/criu/namespaces.c
@@ -315,7 +315,6 @@  struct ns_id *rst_new_ns_id(unsigned int id, pid_t pid,
 int rst_add_ns_id(unsigned int id, struct pstree_item *i, struct ns_desc *nd)
 {
 	pid_t pid = vpid(i);
-	int type = NS_OTHER;
 	struct ns_id *nsid;
 
 	nsid = lookup_ns_by_id(id, nd);
@@ -325,9 +324,7 @@  int rst_add_ns_id(unsigned int id, struct pstree_item *i, struct ns_desc *nd)
 		return 0;
 	}
 
-	if (i == root_item && nd != &user_ns_desc)
-		type = NS_ROOT;
-	nsid = rst_new_ns_id(id, pid, nd, type);
+	nsid = rst_new_ns_id(id, pid, nd, NS_OTHER);
 	if (nsid == NULL)
 		return -1;
 
@@ -1985,6 +1982,34 @@  int read_ns_with_hookups(void)
 	return ret;
 }
 
+static int mark_root_ns(uint32_t id, struct ns_desc *desc)
+{
+	struct ns_id *ns;
+
+	ns = lookup_ns_by_id(id, desc);
+	if (!ns) {
+		pr_err("Can't find root ns %u, %s\n", id, desc->str);
+		return -1;
+	}
+	ns->type = NS_ROOT;
+	return 0;
+}
+
+int set_ns_roots(void)
+{
+	TaskKobjIdsEntry *ids = root_item->ids;
+
+	/* Set root for all namespaces except user_ns, which is set in read_ns_with_hookups() */
+	if ((ids->has_pid_ns_id && mark_root_ns(ids->pid_ns_id, &pid_ns_desc) < 0) ||
+	    (ids->has_net_ns_id && mark_root_ns(ids->net_ns_id, &net_ns_desc) < 0) ||
+	    (ids->has_ipc_ns_id && mark_root_ns(ids->ipc_ns_id, &ipc_ns_desc) < 0) ||
+	    (ids->has_uts_ns_id && mark_root_ns(ids->uts_ns_id, &uts_ns_desc) < 0) ||
+	    (ids->has_mnt_ns_id && mark_root_ns(ids->mnt_ns_id, &mnt_ns_desc) < 0) ||
+	    (ids->has_cgroup_ns_id && mark_root_ns(ids->cgroup_ns_id, &cgroup_ns_desc) < 0))
+		return -1;
+	return 0;
+}
+
 int prepare_userns(pid_t real_pid, UsernsEntry *e)
 {
 	if (write_id_map(real_pid, e->uid_map, e->n_uid_map, "uid_map"))
diff --git a/criu/pstree.c b/criu/pstree.c
index dd40fbcee..b497e1989 100644
--- a/criu/pstree.c
+++ b/criu/pstree.c
@@ -9,6 +9,9 @@ 
 #include "rst-malloc.h"
 #include "common/lock.h"
 #include "namespaces.h"
+#include "cgroup.h"
+#include "ipc_ns.h"
+#include "uts_ns.h"
 #include "files.h"
 #include "tty.h"
 #include "mount.h"
@@ -488,6 +491,18 @@  static int read_pstree_ids(struct pstree_item *pi)
 		if (rst_add_ns_id(pi->ids->pid_ns_id, pi, &pid_ns_desc))
 			return -1;
 	}
+	if (pi->ids->has_ipc_ns_id) {
+		if (rst_add_ns_id(pi->ids->ipc_ns_id, pi, &ipc_ns_desc))
+			return -1;
+	}
+	if (pi->ids->has_uts_ns_id) {
+		if (rst_add_ns_id(pi->ids->uts_ns_id, pi, &uts_ns_desc))
+			return -1;
+	}
+	if (pi->ids->has_cgroup_ns_id) {
+		if (rst_add_ns_id(pi->ids->cgroup_ns_id, pi, &cgroup_ns_desc))
+			return -1;
+	}
 
 	return 0;
 }
@@ -596,6 +611,8 @@  static int read_pstree_image(pid_t *pid_max)
 			goto err;
 	}
 
+	if (ret == 0)
+		ret = set_ns_roots();
 err:
 	close_image(img);
 	return ret;

Comments

Andrey Vagin March 1, 2017, 12:57 a.m.
On Tue, Feb 28, 2017 at 04:41:52PM +0300, Kirill Tkhai wrote:
> Namespaces are read in read_ns_with_hookups(),
> when tasks are not read. So, root_item is NULL,
> and NS_ROOT is not set for appropriate namespaces.
> 
> This patch fixes NS_ROOT after tasks are read. Also
> it adds uts, ipc and cgroup namespaces for uniformity.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  criu/include/namespaces.h |    1 +
>  criu/namespaces.c         |   33 +++++++++++++++++++++++++++++----
>  criu/pstree.c             |   17 +++++++++++++++++
>  3 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/criu/include/namespaces.h b/criu/include/namespaces.h
> index 2a3dc085b..05c08b52e 100644
> --- a/criu/include/namespaces.h
> +++ b/criu/include/namespaces.h
> @@ -149,6 +149,7 @@ extern int collect_mnt_namespaces(bool for_dump);
>  extern int dump_mnt_namespaces(void);
>  extern int dump_namespaces(struct pstree_item *item, unsigned int ns_flags);
>  extern int read_ns_with_hookups(void);
> +extern int set_ns_roots(void);
>  extern int prepare_namespace_before_tasks(void);
>  extern int prepare_namespace(struct pstree_item *item, unsigned long clone_flags);
>  
> diff --git a/criu/namespaces.c b/criu/namespaces.c
> index b1e65df43..a92477df0 100644
> --- a/criu/namespaces.c
> +++ b/criu/namespaces.c
> @@ -315,7 +315,6 @@ struct ns_id *rst_new_ns_id(unsigned int id, pid_t pid,
>  int rst_add_ns_id(unsigned int id, struct pstree_item *i, struct ns_desc *nd)
>  {
>  	pid_t pid = vpid(i);
> -	int type = NS_OTHER;
>  	struct ns_id *nsid;
>  
>  	nsid = lookup_ns_by_id(id, nd);
> @@ -325,9 +324,7 @@ int rst_add_ns_id(unsigned int id, struct pstree_item *i, struct ns_desc *nd)
>  		return 0;
>  	}

Can we remove the hack with the fake pstree_entry?

        pid.ns[0].virt = -1;
        fake.pid = &pid;

....
	if (rst_add_ns_id(e->id, &fake, desc)) {
		pr_err("Can't add user ns\n");
		goto close;
	}

Can we call rst_new_ns_id instead of rst_add_ns_id in read_ns_with_hookups()?


>  
> -	if (i == root_item && nd != &user_ns_desc)
> -		type = NS_ROOT;
> -	nsid = rst_new_ns_id(id, pid, nd, type);
> +	nsid = rst_new_ns_id(id, pid, nd, NS_OTHER);
>  	if (nsid == NULL)
>  		return -1;
>  
> @@ -1985,6 +1982,34 @@ int read_ns_with_hookups(void)
>  	return ret;
>  }
>  
> +static int mark_root_ns(uint32_t id, struct ns_desc *desc)
> +{
> +	struct ns_id *ns;
> +
> +	ns = lookup_ns_by_id(id, desc);
> +	if (!ns) {
> +		pr_err("Can't find root ns %u, %s\n", id, desc->str);
> +		return -1;
> +	}
> +	ns->type = NS_ROOT;
> +	return 0;
> +}
> +
> +int set_ns_roots(void)
> +{
> +	TaskKobjIdsEntry *ids = root_item->ids;
> +
> +	/* Set root for all namespaces except user_ns, which is set in read_ns_with_hookups() */
> +	if ((ids->has_pid_ns_id && mark_root_ns(ids->pid_ns_id, &pid_ns_desc) < 0) ||
> +	    (ids->has_net_ns_id && mark_root_ns(ids->net_ns_id, &net_ns_desc) < 0) ||
> +	    (ids->has_ipc_ns_id && mark_root_ns(ids->ipc_ns_id, &ipc_ns_desc) < 0) ||
> +	    (ids->has_uts_ns_id && mark_root_ns(ids->uts_ns_id, &uts_ns_desc) < 0) ||
> +	    (ids->has_mnt_ns_id && mark_root_ns(ids->mnt_ns_id, &mnt_ns_desc) < 0) ||
> +	    (ids->has_cgroup_ns_id && mark_root_ns(ids->cgroup_ns_id, &cgroup_ns_desc) < 0))

maybe we can add a marcos to make this code readable

	MARK_ROOT_NS(mnt);
	MARK_ROOT_NS(ipc);
	...
> +		return -1;
> +	return 0;
> +}
> +
>  int prepare_userns(pid_t real_pid, UsernsEntry *e)
>  {
>  	if (write_id_map(real_pid, e->uid_map, e->n_uid_map, "uid_map"))
> diff --git a/criu/pstree.c b/criu/pstree.c
> index dd40fbcee..b497e1989 100644
> --- a/criu/pstree.c
> +++ b/criu/pstree.c
> @@ -9,6 +9,9 @@
>  #include "rst-malloc.h"
>  #include "common/lock.h"
>  #include "namespaces.h"
> +#include "cgroup.h"
> +#include "ipc_ns.h"
> +#include "uts_ns.h"
>  #include "files.h"
>  #include "tty.h"
>  #include "mount.h"
> @@ -488,6 +491,18 @@ static int read_pstree_ids(struct pstree_item *pi)
>  		if (rst_add_ns_id(pi->ids->pid_ns_id, pi, &pid_ns_desc))
>  			return -1;
>  	}
> +	if (pi->ids->has_ipc_ns_id) {
> +		if (rst_add_ns_id(pi->ids->ipc_ns_id, pi, &ipc_ns_desc))
> +			return -1;
> +	}
> +	if (pi->ids->has_uts_ns_id) {
> +		if (rst_add_ns_id(pi->ids->uts_ns_id, pi, &uts_ns_desc))
> +			return -1;
> +	}
> +	if (pi->ids->has_cgroup_ns_id) {
> +		if (rst_add_ns_id(pi->ids->cgroup_ns_id, pi, &cgroup_ns_desc))
> +			return -1;
> +	}
>  
>  	return 0;
>  }
> @@ -596,6 +611,8 @@ static int read_pstree_image(pid_t *pid_max)
>  			goto err;
>  	}
>  
> +	if (ret == 0)
> +		ret = set_ns_roots();
>  err:
>  	close_image(img);
>  	return ret;
>
Kirill Tkhai March 1, 2017, 9:26 a.m.
On 01.03.2017 03:57, Andrei Vagin wrote:
> On Tue, Feb 28, 2017 at 04:41:52PM +0300, Kirill Tkhai wrote:
>> Namespaces are read in read_ns_with_hookups(),
>> when tasks are not read. So, root_item is NULL,
>> and NS_ROOT is not set for appropriate namespaces.
>>
>> This patch fixes NS_ROOT after tasks are read. Also
>> it adds uts, ipc and cgroup namespaces for uniformity.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  criu/include/namespaces.h |    1 +
>>  criu/namespaces.c         |   33 +++++++++++++++++++++++++++++----
>>  criu/pstree.c             |   17 +++++++++++++++++
>>  3 files changed, 47 insertions(+), 4 deletions(-)
>>
>> diff --git a/criu/include/namespaces.h b/criu/include/namespaces.h
>> index 2a3dc085b..05c08b52e 100644
>> --- a/criu/include/namespaces.h
>> +++ b/criu/include/namespaces.h
>> @@ -149,6 +149,7 @@ extern int collect_mnt_namespaces(bool for_dump);
>>  extern int dump_mnt_namespaces(void);
>>  extern int dump_namespaces(struct pstree_item *item, unsigned int ns_flags);
>>  extern int read_ns_with_hookups(void);
>> +extern int set_ns_roots(void);
>>  extern int prepare_namespace_before_tasks(void);
>>  extern int prepare_namespace(struct pstree_item *item, unsigned long clone_flags);
>>  
>> diff --git a/criu/namespaces.c b/criu/namespaces.c
>> index b1e65df43..a92477df0 100644
>> --- a/criu/namespaces.c
>> +++ b/criu/namespaces.c
>> @@ -315,7 +315,6 @@ struct ns_id *rst_new_ns_id(unsigned int id, pid_t pid,
>>  int rst_add_ns_id(unsigned int id, struct pstree_item *i, struct ns_desc *nd)
>>  {
>>  	pid_t pid = vpid(i);
>> -	int type = NS_OTHER;
>>  	struct ns_id *nsid;
>>  
>>  	nsid = lookup_ns_by_id(id, nd);
>> @@ -325,9 +324,7 @@ int rst_add_ns_id(unsigned int id, struct pstree_item *i, struct ns_desc *nd)
>>  		return 0;
>>  	}
> 
> Can we remove the hack with the fake pstree_entry?
> 
>         pid.ns[0].virt = -1;
>         fake.pid = &pid;

It's in the patch set; see [3/3].

> ....
> 	if (rst_add_ns_id(e->id, &fake, desc)) {
> 		pr_err("Can't add user ns\n");
> 		goto close;
> 	}
> 
> Can we call rst_new_ns_id instead of rst_add_ns_id in read_ns_with_hookups()?

Yes, we can since read_ns_with_hookups() is called before tasks image,
and ns should be uniqul.
 
>>  
>> -	if (i == root_item && nd != &user_ns_desc)
>> -		type = NS_ROOT;
>> -	nsid = rst_new_ns_id(id, pid, nd, type);
>> +	nsid = rst_new_ns_id(id, pid, nd, NS_OTHER);
>>  	if (nsid == NULL)
>>  		return -1;
>>  
>> @@ -1985,6 +1982,34 @@ int read_ns_with_hookups(void)
>>  	return ret;
>>  }
>>  
>> +static int mark_root_ns(uint32_t id, struct ns_desc *desc)
>> +{
>> +	struct ns_id *ns;
>> +
>> +	ns = lookup_ns_by_id(id, desc);
>> +	if (!ns) {
>> +		pr_err("Can't find root ns %u, %s\n", id, desc->str);
>> +		return -1;
>> +	}
>> +	ns->type = NS_ROOT;
>> +	return 0;
>> +}
>> +
>> +int set_ns_roots(void)
>> +{
>> +	TaskKobjIdsEntry *ids = root_item->ids;
>> +
>> +	/* Set root for all namespaces except user_ns, which is set in read_ns_with_hookups() */
>> +	if ((ids->has_pid_ns_id && mark_root_ns(ids->pid_ns_id, &pid_ns_desc) < 0) ||
>> +	    (ids->has_net_ns_id && mark_root_ns(ids->net_ns_id, &net_ns_desc) < 0) ||
>> +	    (ids->has_ipc_ns_id && mark_root_ns(ids->ipc_ns_id, &ipc_ns_desc) < 0) ||
>> +	    (ids->has_uts_ns_id && mark_root_ns(ids->uts_ns_id, &uts_ns_desc) < 0) ||
>> +	    (ids->has_mnt_ns_id && mark_root_ns(ids->mnt_ns_id, &mnt_ns_desc) < 0) ||
>> +	    (ids->has_cgroup_ns_id && mark_root_ns(ids->cgroup_ns_id, &cgroup_ns_desc) < 0))
> 
> maybe we can add a marcos to make this code readable
> 
> 	MARK_ROOT_NS(mnt);
> 	MARK_ROOT_NS(ipc);
> 	...

Yeah, sounds good.

>> +		return -1;
>> +	return 0;
>> +}
>> +
>>  int prepare_userns(pid_t real_pid, UsernsEntry *e)
>>  {
>>  	if (write_id_map(real_pid, e->uid_map, e->n_uid_map, "uid_map"))
>> diff --git a/criu/pstree.c b/criu/pstree.c
>> index dd40fbcee..b497e1989 100644
>> --- a/criu/pstree.c
>> +++ b/criu/pstree.c
>> @@ -9,6 +9,9 @@
>>  #include "rst-malloc.h"
>>  #include "common/lock.h"
>>  #include "namespaces.h"
>> +#include "cgroup.h"
>> +#include "ipc_ns.h"
>> +#include "uts_ns.h"
>>  #include "files.h"
>>  #include "tty.h"
>>  #include "mount.h"
>> @@ -488,6 +491,18 @@ static int read_pstree_ids(struct pstree_item *pi)
>>  		if (rst_add_ns_id(pi->ids->pid_ns_id, pi, &pid_ns_desc))
>>  			return -1;
>>  	}
>> +	if (pi->ids->has_ipc_ns_id) {
>> +		if (rst_add_ns_id(pi->ids->ipc_ns_id, pi, &ipc_ns_desc))
>> +			return -1;
>> +	}
>> +	if (pi->ids->has_uts_ns_id) {
>> +		if (rst_add_ns_id(pi->ids->uts_ns_id, pi, &uts_ns_desc))
>> +			return -1;
>> +	}
>> +	if (pi->ids->has_cgroup_ns_id) {
>> +		if (rst_add_ns_id(pi->ids->cgroup_ns_id, pi, &cgroup_ns_desc))
>> +			return -1;
>> +	}
>>  
>>  	return 0;
>>  }
>> @@ -596,6 +611,8 @@ static int read_pstree_image(pid_t *pid_max)
>>  			goto err;
>>  	}
>>  
>> +	if (ret == 0)
>> +		ret = set_ns_roots();
>>  err:
>>  	close_image(img);
>>  	return ret;
>>