[4/5] ns: Fix wrong opened net ns file

Submitted by Kirill Tkhai on March 24, 2017, 9:35 a.m.

Details

Message ID 149034811100.30232.6600685910792511968.stgit@localhost.localdomain
State New
Series "Restore of "/proc/self/ns/net" fixes"
Headers show

Commit Message

Kirill Tkhai March 24, 2017, 9:35 a.m.
Since net ns is assigned after prepare_fds() and,
in common case, at the moment of open_ns_fd() call
task points to a net ns, which differs to its target
net ns, we can't get the ns from a task. So, get it
from fdstore. Also, support userns ns fds.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 criu/namespaces.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/namespaces.c b/criu/namespaces.c
index e4555251..ee309045 100644
--- a/criu/namespaces.c
+++ b/criu/namespaces.c
@@ -556,8 +556,22 @@  static int open_ns_fd(struct file_desc *d, int *new_fd)
 	struct ns_file_info *nfi = container_of(d, struct ns_file_info, d);
 	struct pstree_item *item, *t;
 	struct ns_desc *nd = NULL;
+	struct ns_id *ns;
+	int nsfd_id, fd;
 	char path[64];
-	int fd;
+
+	for (ns = ns_ids; ns != NULL; ns = ns->next) {
+		if (ns->id != nfi->nfe->ns_id)
+			continue;
+		if (ns->nd == &user_ns_desc && (root_ns_mask & CLONE_NEWUSER))
+			nsfd_id = ns->user.nsfd_id;
+		else if (ns->nd == &net_ns_desc && (root_ns_mask & CLONE_NEWNET))
+			nsfd_id = ns->net.nsfd_id;
+		else
+			break;
+		fd = fdstore_get(nsfd_id);
+		goto check_open;
+	}
 
 	/*
 	 * Find out who can open us.
@@ -575,6 +589,10 @@  static int open_ns_fd(struct file_desc *d, int *new_fd)
 			item = t;
 			nd = &net_ns_desc;
 			break;
+		} else if (ids->user_ns_id == nfi->nfe->ns_id) {
+			item = t;
+			nd = &user_ns_desc;
+			break;
 		} else if (ids->ipc_ns_id == nfi->nfe->ns_id) {
 			item = t;
 			nd = &ipc_ns_desc;
@@ -608,6 +626,7 @@  static int open_ns_fd(struct file_desc *d, int *new_fd)
 	path[sizeof(path) - 1] = '\0';
 
 	fd = open(path, nfi->nfe->flags);
+check_open:
 	if (fd < 0) {
 		pr_perror("Can't open file %s on restore", path);
 		return fd;

Comments

Andrey Vagin March 27, 2017, 8:36 p.m.
On Fri, Mar 24, 2017 at 12:35:11PM +0300, Kirill Tkhai wrote:
> Since net ns is assigned after prepare_fds() and,
> in common case, at the moment of open_ns_fd() call
> task points to a net ns, which differs to its target
> net ns, we can't get the ns from a task. So, get it
> from fdstore. Also, support userns ns fds.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  criu/namespaces.c |   21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/namespaces.c b/criu/namespaces.c
> index e4555251..ee309045 100644
> --- a/criu/namespaces.c
> +++ b/criu/namespaces.c
> @@ -556,8 +556,22 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>  	struct ns_file_info *nfi = container_of(d, struct ns_file_info, d);
>  	struct pstree_item *item, *t;
>  	struct ns_desc *nd = NULL;
> +	struct ns_id *ns;
> +	int nsfd_id, fd;
>  	char path[64];
> -	int fd;
> +
> +	for (ns = ns_ids; ns != NULL; ns = ns->next) {
> +		if (ns->id != nfi->nfe->ns_id)
> +			continue;
> +		if (ns->nd == &user_ns_desc && (root_ns_mask & CLONE_NEWUSER))
> +			nsfd_id = ns->user.nsfd_id;
> +		else if (ns->nd == &net_ns_desc && (root_ns_mask & CLONE_NEWNET))
> +			nsfd_id = ns->net.nsfd_id;
> +		else
> +			break;
> +		fd = fdstore_get(nsfd_id);
> +		goto check_open;
> +	}
>  
>  	/*
>  	 * Find out who can open us.
> @@ -575,6 +589,10 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>  			item = t;
>  			nd = &net_ns_desc;
>  			break;
> +		} else if (ids->user_ns_id == nfi->nfe->ns_id) {
> +			item = t;
> +			nd = &user_ns_desc;
> +			break;

hm, why do we need this code here? The first loop has to handle them,
doesn't it?

>  		} else if (ids->ipc_ns_id == nfi->nfe->ns_id) {
>  			item = t;
>  			nd = &ipc_ns_desc;
> @@ -608,6 +626,7 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>  	path[sizeof(path) - 1] = '\0';
>  
>  	fd = open(path, nfi->nfe->flags);
> +check_open:
>  	if (fd < 0) {
>  		pr_perror("Can't open file %s on restore", path);
>  		return fd;
>
Kirill Tkhai March 28, 2017, 9:32 a.m.
On 27.03.2017 23:36, Andrei Vagin wrote:
> On Fri, Mar 24, 2017 at 12:35:11PM +0300, Kirill Tkhai wrote:
>> Since net ns is assigned after prepare_fds() and,
>> in common case, at the moment of open_ns_fd() call
>> task points to a net ns, which differs to its target
>> net ns, we can't get the ns from a task. So, get it
>> from fdstore. Also, support userns ns fds.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  criu/namespaces.c |   21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/criu/namespaces.c b/criu/namespaces.c
>> index e4555251..ee309045 100644
>> --- a/criu/namespaces.c
>> +++ b/criu/namespaces.c
>> @@ -556,8 +556,22 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>>  	struct ns_file_info *nfi = container_of(d, struct ns_file_info, d);
>>  	struct pstree_item *item, *t;
>>  	struct ns_desc *nd = NULL;
>> +	struct ns_id *ns;
>> +	int nsfd_id, fd;
>>  	char path[64];
>> -	int fd;
>> +
>> +	for (ns = ns_ids; ns != NULL; ns = ns->next) {
>> +		if (ns->id != nfi->nfe->ns_id)
>> +			continue;
>> +		if (ns->nd == &user_ns_desc && (root_ns_mask & CLONE_NEWUSER))
>> +			nsfd_id = ns->user.nsfd_id;
>> +		else if (ns->nd == &net_ns_desc && (root_ns_mask & CLONE_NEWNET))
>> +			nsfd_id = ns->net.nsfd_id;
>> +		else
>> +			break;
>> +		fd = fdstore_get(nsfd_id);
>> +		goto check_open;
>> +	}
>>  
>>  	/*
>>  	 * Find out who can open us.
>> @@ -575,6 +589,10 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>>  			item = t;
>>  			nd = &net_ns_desc;
>>  			break;
>> +		} else if (ids->user_ns_id == nfi->nfe->ns_id) {
>> +			item = t;
>> +			nd = &user_ns_desc;
>> +			break;
> 
> hm, why do we need this code here? The first loop has to handle them,
> doesn't it?

We do add net_ns and user_ns fds to fdstore only if we have
appropriate bit in root_ns_mask is set.
 
>>  		} else if (ids->ipc_ns_id == nfi->nfe->ns_id) {
>>  			item = t;
>>  			nd = &ipc_ns_desc;
>> @@ -608,6 +626,7 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>>  	path[sizeof(path) - 1] = '\0';
>>  
>>  	fd = open(path, nfi->nfe->flags);
>> +check_open:
>>  	if (fd < 0) {
>>  		pr_perror("Can't open file %s on restore", path);
>>  		return fd;
>>
Andrey Vagin March 28, 2017, 9:38 p.m.
On Tue, Mar 28, 2017 at 12:32:08PM +0300, Kirill Tkhai wrote:
> On 27.03.2017 23:36, Andrei Vagin wrote:
> > On Fri, Mar 24, 2017 at 12:35:11PM +0300, Kirill Tkhai wrote:
> >> Since net ns is assigned after prepare_fds() and,
> >> in common case, at the moment of open_ns_fd() call
> >> task points to a net ns, which differs to its target
> >> net ns, we can't get the ns from a task. So, get it
> >> from fdstore. Also, support userns ns fds.
> >>
> >> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> >> ---
> >>  criu/namespaces.c |   21 ++++++++++++++++++++-
> >>  1 file changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/criu/namespaces.c b/criu/namespaces.c
> >> index e4555251..ee309045 100644
> >> --- a/criu/namespaces.c
> >> +++ b/criu/namespaces.c
> >> @@ -556,8 +556,22 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
> >>  	struct ns_file_info *nfi = container_of(d, struct ns_file_info, d);
> >>  	struct pstree_item *item, *t;
> >>  	struct ns_desc *nd = NULL;
> >> +	struct ns_id *ns;
> >> +	int nsfd_id, fd;
> >>  	char path[64];
> >> -	int fd;
> >> +
> >> +	for (ns = ns_ids; ns != NULL; ns = ns->next) {
> >> +		if (ns->id != nfi->nfe->ns_id)
> >> +			continue;
> >> +		if (ns->nd == &user_ns_desc && (root_ns_mask & CLONE_NEWUSER))
> >> +			nsfd_id = ns->user.nsfd_id;
> >> +		else if (ns->nd == &net_ns_desc && (root_ns_mask & CLONE_NEWNET))
> >> +			nsfd_id = ns->net.nsfd_id;
> >> +		else
> >> +			break;
> >> +		fd = fdstore_get(nsfd_id);
> >> +		goto check_open;
> >> +	}
> >>  
> >>  	/*
> >>  	 * Find out who can open us.
> >> @@ -575,6 +589,10 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
> >>  			item = t;
> >>  			nd = &net_ns_desc;
> >>  			break;
> >> +		} else if (ids->user_ns_id == nfi->nfe->ns_id) {
> >> +			item = t;
> >> +			nd = &user_ns_desc;
> >> +			break;
> > 
> > hm, why do we need this code here? The first loop has to handle them,
> > doesn't it?
> 
> We do add net_ns and user_ns fds to fdstore only if we have
> appropriate bit in root_ns_mask is set.

Could you add a comment about this?
>  
> >>  		} else if (ids->ipc_ns_id == nfi->nfe->ns_id) {
> >>  			item = t;
> >>  			nd = &ipc_ns_desc;
> >> @@ -608,6 +626,7 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
> >>  	path[sizeof(path) - 1] = '\0';
> >>  
> >>  	fd = open(path, nfi->nfe->flags);
> >> +check_open:
> >>  	if (fd < 0) {
> >>  		pr_perror("Can't open file %s on restore", path);
> >>  		return fd;
> >>
Kirill Tkhai March 29, 2017, 8:47 a.m.
On 29.03.2017 00:38, Andrei Vagin wrote:
> On Tue, Mar 28, 2017 at 12:32:08PM +0300, Kirill Tkhai wrote:
>> On 27.03.2017 23:36, Andrei Vagin wrote:
>>> On Fri, Mar 24, 2017 at 12:35:11PM +0300, Kirill Tkhai wrote:
>>>> Since net ns is assigned after prepare_fds() and,
>>>> in common case, at the moment of open_ns_fd() call
>>>> task points to a net ns, which differs to its target
>>>> net ns, we can't get the ns from a task. So, get it
>>>> from fdstore. Also, support userns ns fds.
>>>>
>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>> ---
>>>>  criu/namespaces.c |   21 ++++++++++++++++++++-
>>>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/criu/namespaces.c b/criu/namespaces.c
>>>> index e4555251..ee309045 100644
>>>> --- a/criu/namespaces.c
>>>> +++ b/criu/namespaces.c
>>>> @@ -556,8 +556,22 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>>>>  	struct ns_file_info *nfi = container_of(d, struct ns_file_info, d);
>>>>  	struct pstree_item *item, *t;
>>>>  	struct ns_desc *nd = NULL;
>>>> +	struct ns_id *ns;
>>>> +	int nsfd_id, fd;
>>>>  	char path[64];
>>>> -	int fd;
>>>> +
>>>> +	for (ns = ns_ids; ns != NULL; ns = ns->next) {
>>>> +		if (ns->id != nfi->nfe->ns_id)
>>>> +			continue;
>>>> +		if (ns->nd == &user_ns_desc && (root_ns_mask & CLONE_NEWUSER))
>>>> +			nsfd_id = ns->user.nsfd_id;
>>>> +		else if (ns->nd == &net_ns_desc && (root_ns_mask & CLONE_NEWNET))
>>>> +			nsfd_id = ns->net.nsfd_id;
>>>> +		else
>>>> +			break;
>>>> +		fd = fdstore_get(nsfd_id);
>>>> +		goto check_open;
>>>> +	}
>>>>  
>>>>  	/*
>>>>  	 * Find out who can open us.
>>>> @@ -575,6 +589,10 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>>>>  			item = t;
>>>>  			nd = &net_ns_desc;
>>>>  			break;
>>>> +		} else if (ids->user_ns_id == nfi->nfe->ns_id) {
>>>> +			item = t;
>>>> +			nd = &user_ns_desc;
>>>> +			break;
>>>
>>> hm, why do we need this code here? The first loop has to handle them,
>>> doesn't it?
>>
>> We do add net_ns and user_ns fds to fdstore only if we have
>> appropriate bit in root_ns_mask is set.
> 
> Could you add a comment about this?

Ok

>>  
>>>>  		} else if (ids->ipc_ns_id == nfi->nfe->ns_id) {
>>>>  			item = t;
>>>>  			nd = &ipc_ns_desc;
>>>> @@ -608,6 +626,7 @@ static int open_ns_fd(struct file_desc *d, int *new_fd)
>>>>  	path[sizeof(path) - 1] = '\0';
>>>>  
>>>>  	fd = open(path, nfi->nfe->flags);
>>>> +check_open:
>>>>  	if (fd < 0) {
>>>>  		pr_perror("Can't open file %s on restore", path);
>>>>  		return fd;
>>>>