Commit bcf0d9d4 authored by Al Viro's avatar Al Viro
Browse files

ecryptfs: fix unlink and rmdir in face of underlying fs modifications 



A problem similar to the one caught in commit 74dd7c97 ("ecryptfs_rename():
verify that lower dentries are still OK after lock_rename()") exists for
unlink/rmdir as well.

Instead of playing with dget_parent() of underlying dentry of victim
and hoping it's the same as underlying dentry of our directory,
do the following:
        * find the underlying dentry of victim
        * find the underlying directory of victim's parent (stable
since the victim is ecryptfs dentry and inode of its parent is
held exclusive by the caller).
        * lock the inode of dentry underlying the victim's parent
        * check that underlying dentry of victim is still hashed and
has the right parent - it can be moved, but it can't be moved to/from
the directory we are holding exclusive.  So while ->d_parent itself
might not be stable, the result of comparison is.

If the check passes, everything is fine - underlying directory is locked,
underlying victim is still a child of that directory and we can go ahead
and feed them to vfs_unlink().  As in the current mainline we need to
pin the underlying dentry of victim, so that it wouldn't go negative under
us, but that's the only temporary reference that needs to be grabbed there.
Underlying dentry of parent won't go away (it's pinned by the parent,
which is held by caller), so there's no need to grab it.

The same problem (with the same solution) exists for rmdir.  Moreover,
rename gets simpler and more robust with the same "don't bother with
dget_parent()" approach.

Fixes: 74dd7c97 "ecryptfs_rename(): verify that lower dentries are still OK after lock_rename()"
Signed-off-by: default avatarAl Viro <viro@zeniv.linux.org.uk>
parent 69924b89

fs/ecryptfs/inode.c

+40 −25
Original line number Original line Diff line number Diff line
@@ -128,12 +128,19 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry, 
			      struct inode *inode)

			      struct inode *inode)

{

{

	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);

	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);

	struct inode *lower_dir_inode = ecryptfs_inode_to_lower(dir);

	struct dentry *lower_dir_dentry;

	struct dentry *lower_dir_dentry;

	struct inode *lower_dir_inode;

	int rc;

	int rc;





	dget(lower_dentry);

	lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent);

	lower_dir_dentry = lock_parent(lower_dentry);

	lower_dir_inode = d_inode(lower_dir_dentry);

	inode_lock_nested(lower_dir_inode, I_MUTEX_PARENT);

	dget(lower_dentry);	// don't even try to make the lower negative

	if (lower_dentry->d_parent != lower_dir_dentry)

		rc = -EINVAL;

	else if (d_unhashed(lower_dentry))

		rc = -EINVAL;

	else

		rc = vfs_unlink(lower_dir_inode, lower_dentry, NULL);

		rc = vfs_unlink(lower_dir_inode, lower_dentry, NULL);

	if (rc) {

	if (rc) {

		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);

		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
@@ -142,10 +149,11 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry, 
	fsstack_copy_attr_times(dir, lower_dir_inode);

	fsstack_copy_attr_times(dir, lower_dir_inode);

	set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);

	set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);

	inode->i_ctime = dir->i_ctime;

	inode->i_ctime = dir->i_ctime;

	d_drop(dentry);

out_unlock:

out_unlock:

	unlock_dir(lower_dir_dentry);

	dput(lower_dentry);

	dput(lower_dentry);

	inode_unlock(lower_dir_inode);

	if (!rc)

		d_drop(dentry);

	return rc;

	return rc;

}

}
@@ -512,22 +520,30 @@ static int ecryptfs_rmdir(struct inode *dir, struct dentry *dentry) 
{

{

	struct dentry *lower_dentry;

	struct dentry *lower_dentry;

	struct dentry *lower_dir_dentry;

	struct dentry *lower_dir_dentry;

	struct inode *lower_dir_inode;

	int rc;

	int rc;





	lower_dentry = ecryptfs_dentry_to_lower(dentry);

	lower_dentry = ecryptfs_dentry_to_lower(dentry);

	dget(dentry);

	lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent);

	lower_dir_dentry = lock_parent(lower_dentry);

	lower_dir_inode = d_inode(lower_dir_dentry);

	dget(lower_dentry);



	rc = vfs_rmdir(d_inode(lower_dir_dentry), lower_dentry);

	inode_lock_nested(lower_dir_inode, I_MUTEX_PARENT);

	dput(lower_dentry);

	dget(lower_dentry);	// don't even try to make the lower negative

	if (!rc && d_really_is_positive(dentry))

	if (lower_dentry->d_parent != lower_dir_dentry)

		rc = -EINVAL;

	else if (d_unhashed(lower_dentry))

		rc = -EINVAL;

	else

		rc = vfs_rmdir(lower_dir_inode, lower_dentry);

	if (!rc) {

		clear_nlink(d_inode(dentry));

		clear_nlink(d_inode(dentry));

	fsstack_copy_attr_times(dir, d_inode(lower_dir_dentry));

		fsstack_copy_attr_times(dir, lower_dir_inode);

	set_nlink(dir, d_inode(lower_dir_dentry)->i_nlink);

		set_nlink(dir, lower_dir_inode->i_nlink);

	unlock_dir(lower_dir_dentry);

	}

	dput(lower_dentry);

	inode_unlock(lower_dir_inode);

	if (!rc)

	if (!rc)

		d_drop(dentry);

		d_drop(dentry);

	dput(dentry);

	return rc;

	return rc;

}

}
@@ -565,20 +581,22 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry, 
	struct dentry *lower_new_dentry;

	struct dentry *lower_new_dentry;

	struct dentry *lower_old_dir_dentry;

	struct dentry *lower_old_dir_dentry;

	struct dentry *lower_new_dir_dentry;

	struct dentry *lower_new_dir_dentry;

	struct dentry *trap = NULL;

	struct dentry *trap;

	struct inode *target_inode;

	struct inode *target_inode;





	if (flags)

	if (flags)

		return -EINVAL;

		return -EINVAL;





	lower_old_dir_dentry = ecryptfs_dentry_to_lower(old_dentry->d_parent);

	lower_new_dir_dentry = ecryptfs_dentry_to_lower(new_dentry->d_parent);



	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);

	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);

	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);

	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);

	dget(lower_old_dentry);



	dget(lower_new_dentry);

	lower_old_dir_dentry = dget_parent(lower_old_dentry);

	lower_new_dir_dentry = dget_parent(lower_new_dentry);

	target_inode = d_inode(new_dentry);

	target_inode = d_inode(new_dentry);



	trap = lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);

	trap = lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);

	dget(lower_new_dentry);

	rc = -EINVAL;

	rc = -EINVAL;

	if (lower_old_dentry->d_parent != lower_old_dir_dentry)

	if (lower_old_dentry->d_parent != lower_old_dir_dentry)

		goto out_lock;

		goto out_lock;
@@ -606,11 +624,8 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry, 
	if (new_dir != old_dir)

	if (new_dir != old_dir)

		fsstack_copy_attr_all(old_dir, d_inode(lower_old_dir_dentry));

		fsstack_copy_attr_all(old_dir, d_inode(lower_old_dir_dentry));

out_lock:

out_lock:

	unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);

	dput(lower_new_dir_dentry);

	dput(lower_old_dir_dentry);

	dput(lower_new_dentry);

	dput(lower_new_dentry);

	dput(lower_old_dentry);

	unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);

	return rc;

	return rc;

}

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE