Re: [RFC][PATCH 4/3] vfs: Allow rmdir to remove mounts in all but the current mount namespace

[ebiederm@xmission.com (Eric W. Biederman)]

"Serge E. Hallyn" <serge@hallyn.com> writes:

> Quoting Eric W. Biederman (ebiederm@xmission.com):
>> 
>> Programs have been known to test for empty directories by attempting
>> to remove them.  To keep from violating the principle of least
>> surprise don't let directories the caller can see with someting
>> mounted on them be deleted.
>
> Do you think we should do the same thing for over-mounted file at
> vfs_unlink()?

We easily could.

The point of the patch is to just preserve the directory is empty don't
allow rmdir to succeed semantics, and as typically we can see something
in the directory because of the mount it doesn't make sense for rmdir to
succeed.

unlink doesn't have any occassions when the permissions are sufficient
to remove a directory where it will fail.  So I don't see the point of
doing this for anything except directories.

Except for possibly the oddball rmdir semantics mentioned I don't think
this patch should be part of anyone's correctness analysis.



It is easiest to see that this series of changes is semantically safe if
we are safe to run unprivileged code in a mount namespace where root has
locally unmounted every mount point.

We do have the restriction that in a user namespace we can't unmount
anything root was mounted outside the user namespace.  Which combined
with the above patch would be roughly equivalent to todays mount
restrictions for the common case.  Unfortunately being only roughly
equivalent the analysis gets very complicated, and complicated reasoning
usually means invalid reasoning.


So if we can feel safe just depending on the parent directory
permissions (which are not hidden by a mount) protecting our mount
points, I feel much better about this patchset.



But if you can articulate some reasons why it would be better and less
surprising for unlink to fail I am willing to listen.



>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  fs/namei.c |   21 +++++++++++++++++++++
>>  1 files changed, 21 insertions(+), 0 deletions(-)
>> 
>> diff --git a/fs/namei.c b/fs/namei.c
>> index b18b017c946b..b9cae480ac27 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -3547,6 +3547,20 @@ void dentry_unhash(struct dentry *dentry)
>>  	spin_unlock(&dentry->d_lock);
>>  }
>>  
>> +static bool covered(struct vfsmount *mnt, struct dentry *dentry)
>> +{
>> +	/* test to see if a dentry is covered with a mount in
>> +	 * the current mount namespace.
>> +	 */
>> +	bool is_covered;
>> +
>> +	rcu_read_lock();
>> +	is_covered = d_mountpoint(dentry) && __lookup_mnt(mnt, dentry, 1);
>> +	rcu_read_unlock();
>> +
>> +	return is_covered;
>> +}
>> +
>>  int vfs_rmdir(struct inode *dir, struct dentry *dentry)
>>  {
>>  	int error = may_delete(dir, dentry, 1);
>> @@ -3619,6 +3633,9 @@ retry:
>>  		error = -ENOENT;
>>  		goto exit3;
>>  	}
>> +	error = -EBUSY;
>> +	if (covered(nd.path.mnt, dentry))
>> +		goto exit3;
>>  	error = security_path_rmdir(&nd.path, dentry);
>>  	if (error)
>>  		goto exit3;
>> @@ -4155,6 +4172,10 @@ retry:
>>  	error = -ENOTEMPTY;
>>  	if (new_dentry == trap)
>>  		goto exit5;
>> +	error = -EBUSY;
>> +	if (new_dentry->d_inode && S_ISDIR(new_dentry->d_inode->i_mode) &&
>> +	    covered(newnd.path.mnt, new_dentry))
>> +		goto exit5;
>>  
>>  	error = security_path_rename(&oldnd.path, old_dentry,
>>  				     &newnd.path, new_dentry);
>> -- 
>> 1.7.5.4