[CRIU] [PATCH] mount: close mnt.ns_fd only for sub-namespaces (v2)

[Pavel Emelyanov]

On 11/26/2015 05:48 PM, Andrew Vagin wrote:
> On Thu, Nov 26, 2015 at 05:11:56PM +0300, Pavel Emelyanov wrote:
>> On 11/26/2015 05:02 PM, Andrey Vagin wrote:
>>> From: Andrew Vagin <avagin at virtuozzo.com>
>>>
>>> nsid->mnt.ns_fd is initialized into 0, so currently
>>> fini_restore_mntns() closes the 0 descriptor if processes
>>> lives in a current mount namespace (NS_CRIU).
>>>
>>> Without this patch I get the following error:
>>> (00.166444)   4109: Inherit fd tty:[8800:d] -> 0 has been closed
>>>
>>> v2: typo fix
>>> Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
>>> ---
>>>  mount.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mount.c b/mount.c
>>> index 20ae79b..d0f05f6 100644
>>> --- a/mount.c
>>> +++ b/mount.c
>>> @@ -2653,7 +2653,7 @@ void fini_restore_mntns(void)
>>>  	for (nsid = ns_ids; nsid != NULL; nsid = nsid->next) {
>>>  		if (nsid->nd != &mnt_ns_desc)
>>>  			continue;
>>> -		if (root_item->ids->mnt_ns_id == nsid->id)
>>> +		if (nsid->type != NS_OTHER)
>>>  			continue;
>>>  		close(nsid->mnt.ns_fd);
>>
>> Look at prepare_mnt_ns() it does this:
> 
> It calls rst_collect_local_mntns(), which allocates ns_id for criu mount
> namespaces.
> 
>         if (!(root_ns_mask & CLONE_NEWNS))
>                 return rst_collect_local_mntns();
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

OK, so maybe in fini we should just put

	if (!(root_ns_mask & CLONE_NEWNS))
		return;

at the beginning to be symmetrical with this?

>>
>>        for (nsid = ns_ids; nsid != NULL; nsid = nsid->next) {
>>                 char path[PATH_MAX];
>>
>>                 if (nsid->nd != &mnt_ns_desc)
>>                         continue;
>>                 if (root_item->ids->mnt_ns_id == nsid->id)
>>                         continue;
> 
> We skip the root mount namespace ^^^^
> 
>>
>> 		...
>>
>> 		nsid->mnt.ns_fd = open_proc(PROC_SELF, "ns/mnt");
>>                 if (nsid->mnt.ns_fd < 0)
>>                         goto err;
>>
>> There's no single check for nsid->type, why do we have one on
>> the closing part?
> 
> 
> 
>>
>>>  	}
>>>
>>
> .
>