[Devel] [PATCH] binfmt_misc: fix mount after umount in CT

[Nikita Yushchenko]

>> -	bm_data = kzalloc(sizeof(struct binfmt_misc), GFP_KERNEL);
>> -	if (!bm_data)
>> -		return -ENOMEM;
>> +		INIT_LIST_HEAD(&bm_data->entries);
>> +		rwlock_init(&bm_data->entries_lock);
>>   
>> -	INIT_LIST_HEAD(&bm_data->entries);
>> -	rwlock_init(&bm_data->entries_lock);
>> +		ve->binfmt_misc = bm_data;
> 
> Isn't it better to move ve->binfmt_misc assignment to the
> end of function where we know that all operations was successful?

Since ve->binfmt_misc is global, not per-mount, the logic is simpler if creation of bm_data is made 
independent from mount success/failure.  I.e. regardless of success of mount operation, bm_data is 
created at the first mount and saved in ve->binfmt_misc. Then, it will be cleaned at ve destruction time.

> 
>> +		/* this will be cleared by ve_binfmt_fini() */
>> +	}
>>   
>>   	err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
>> -	if (err) {
>> -		kfree(bm_data);
> 
> If we have ve->binfmt_misc assignment in the upper part of code, then
> we need to do ve->binfmt_misc = NULL here.

This will mishandle case when ve->binfmt_misc was initialized at previous mount.

> 
>> +	if (err)
>>   		return err;
>> -	}
>>   
>>   	sb->s_op = &s_ops;
>> -
>> -	ve->binfmt_misc = bm_data;
> see above
> 
>>   	bm_data->enabled = 1;
>>   
>>   	return 0;
>> @@ -971,6 +958,8 @@ static void ve_binfmt_fini(void *data)
>>   	while (!list_empty(&bm_data->entries))
>>   		kill_node(bm_data, list_first_entry(
>>   			&bm_data->entries, Node, list));
>> +
>> +	kfree(bm_data);
> 
> We have kfree in ve_destroy (kernel/ve/ve.c) already.

Indeed.

But, why splitting destruction into two parts?
Why not doing both at the same location.

Nikita