[CRIU] [PATCH] restore_root_task(): don't clean remaps if mnt_ns_fd is closed

[Kir Kolyshkin]

On 10/14/2015 05:59 AM, Pavel Emelyanov wrote:
> On 10/13/2015 09:21 PM, Kir Kolyshkin wrote:
>> As pointed out by Coverity (CID 114629), mnt_ns_fd is closed,
>> but then the function calls try_clean_remaps(mnt_ns_fd)
>> which tries to close the file descriptor which is already closed.
>>
>> Also, as pointed out by Pavel, "calling the whole try_clean_remaps()
>> is not required once we've passed the cleanup_mnt_ns() point".
>>
>> So, let's just set mnt_ns_fd to -1 after closing it, and
>> return from try_clean_remaps() if the argument is negative.
> Still incorrect. mnt_ns_fd can be -1 validly meaning that no mntns's
> are in there, but ghost cleaning on error path is _still_ required.

It seems to me you are trying to address a different issue
(optimizing out a call to try_clean_remaps()) than the one
I was fixing in my initial patch (not closing a closed fd).

Anyway, v3 has just been sent, please review.

>
>> Signed-off-by: Kir Kolyshkin <kir at openvz.org>
>> ---
>>   cr-restore.c |  4 +++-
>>   files-reg.c  | 28 ++++++++++++++--------------
>>   2 files changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/cr-restore.c b/cr-restore.c
>> index 5d1b501..6677edf 100644
>> --- a/cr-restore.c
>> +++ b/cr-restore.c
>> @@ -1854,12 +1854,14 @@ static int restore_root_task(struct pstree_item *init)
>>   	 */
>>   	task_entries->nr_threads -= atomic_read(&task_entries->nr_zombies);
>>   
>> -	if (mnt_ns_fd >= 0)
>> +	if (mnt_ns_fd >= 0) {
>>   		/*
>>   		 * Don't try_clean_remaps here, since restore went OK
>>   		 * and all ghosts were removed by the openers.
>>   		 */
>>   		close(mnt_ns_fd);
>> +		mnt_ns_fd = -1;
>> +	}
>>   	cleanup_mnt_ns();
>>   
>>   	ret = stop_usernsd();
>> diff --git a/files-reg.c b/files-reg.c
>> index 59d88ca..82e4742 100644
>> --- a/files-reg.c
>> +++ b/files-reg.c
>> @@ -423,23 +423,24 @@ void try_clean_remaps(int ns_fd)
>>   	struct remap_info *ri;
>>   	int old_ns = -1;
>>   
>> +	if (ns_fd < 0)
>> +		return;
>> +
>>   	if (list_empty(&remaps))
>>   		goto out;
>>   
>> -	if (ns_fd >= 0) {
>> -		pr_info("Switching to new ns to clean ghosts\n");
>> +	pr_info("Switching to new ns to clean ghosts\n");
>>   
>> -		old_ns = open_proc(PROC_SELF, "ns/mnt");
>> -		if (old_ns < 0) {
>> -			pr_perror("`- Can't keep old ns");
>> -			return;
>> -		}
>> +	old_ns = open_proc(PROC_SELF, "ns/mnt");
>> +	if (old_ns < 0) {
>> +		pr_perror("`- Can't keep old ns");
>> +		return;
>> +	}
>>   
>> -		if (setns(ns_fd, CLONE_NEWNS) < 0) {
>> -			close(old_ns);
>> -			pr_perror("`- Can't switch");
>> -			return;
>> -		}
>> +	if (setns(ns_fd, CLONE_NEWNS) < 0) {
>> +		close(old_ns);
>> +		pr_perror("`- Can't switch");
>> +		return;
>>   	}
>>   
>>   	list_for_each_entry(ri, &remaps, list)
>> @@ -453,8 +454,7 @@ void try_clean_remaps(int ns_fd)
>>   	}
>>   
>>   out:
>> -	if (ns_fd >= 0)
>> -		close(ns_fd);
>> +	close(ns_fd);
>>   }
>>   
>>   static struct collect_image_info remap_cinfo = {
>>