[Devel] [PATCH rh7] ve/fs/sync: fix CT's mountpoints traversal

[Andrey Ryabinin]

On 02/24/2016 07:15 PM, Vladimir Davydov wrote:
> On Wed, Feb 24, 2016 at 06:36:22PM +0300, Andrey Ryabinin wrote:
>> Currently sync reads 've->root_path.mnt' mount and iterate over it childs.
>> This doesn't work, as not all in-container mounts could be reached that way.
>>
>> This patch slightly rework's mounts traversal. Now sync just iterates over all
>> mounts of current mount namespace (actually we iterate over all mounts that
>> seen in container's /proc/mounts). This approach seems much more correct and
>> also simpler.
>>
>> I think we could get rid of 've->root_path' now.
> 
> It's still used in follow_dotdot to prevent chroot escape.
> 
Does it still needed? Shouldn't mount namespace prevent this?

>>
>> https://jira.sw.ru/browse/PSBM-44125
>>
>> Signed-off-by: Andrey Ryabinin <aryabinin at virtuozzo.com>
> ...
>> diff --git a/fs/sync.c b/fs/sync.c
>> index 8119ab4..9242a60 100644
>> --- a/fs/sync.c
>> +++ b/fs/sync.c
>> @@ -127,17 +127,17 @@ static int sync_filesystem_collected(struct list_head *sync_list, struct super_b
>>  	return 0;
>>  }
>>  
>> -static int sync_collect_filesystems(struct ve_struct *ve, struct list_head *sync_list)
>> +static int sync_collect_filesystems(struct list_head *sync_list)
>>  {
>> -	struct mount *root = real_mount(ve->root_path.mnt);
>>  	struct mount *mnt;
>> +	struct mnt_namespace *mnt_ns = current->nsproxy->mnt_ns;
> 
> Will it work as expected if there are mount namespaces inside a
> container?
> 

Depends on what is expected. I guess this unexpected since this makes sync per mount-namespace rather then per-container.

Could be fixed with ve->ve_ns->mnt_ns.

>>  	struct sync_sb *ss;
>>  	int ret = 0;
>>  
>>  	BUG_ON(!list_empty(sync_list));
>>  
>>  	down_read(&namespace_sem);
>> -	for (mnt = root; mnt; mnt = next_mnt(mnt, root)) {
>> +	list_for_each_entry(mnt, &mnt_ns->list, mnt_list) {
>>  		if (sync_filesystem_collected(sync_list, mnt->mnt.mnt_sb))
>>  			continue;
>>  
>> @@ -161,20 +161,18 @@ static int sync_collect_filesystems(struct ve_struct *ve, struct list_head *sync
>>  	return ret;
>>  }
>>  
>> -static void sync_filesystems_ve(struct ve_struct *ve, struct user_beancounter *ub, int wait)
>> +static void sync_filesystems_ub(struct user_beancounter *ub, int wait)
>>  {
>>  	struct super_block *sb;
>>  	LIST_HEAD(sync_list);
>>  	struct sync_sb *ss;
>>  
>> -	mutex_lock(&ve->sync_mutex);		/* Could be down_interruptible */
>> -
> 
> This mutex isn't here to protect ve->root_path. I don't know why we need
> it or whether we need it at all, but anyway this should go in a separate
> patch with a proper comment.
> 

Ok