| From 28f86c337b2430a855490699542c6e875691cce3 Mon Sep 17 00:00:00 2001 |
| From: Al Viro <viro@zeniv.linux.org.uk> |
| Date: Sun, 3 Nov 2019 12:07:15 -0500 |
| Subject: [PATCH] ecryptfs: fix unlink and rmdir in face of underlying fs |
| modifications |
| |
| commit bcf0d9d4b76976f892154efdfc509b256fd898e8 upstream. |
| |
| A problem similar to the one caught in commit 74dd7c97ea2a ("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: 74dd7c97ea2 "ecryptfs_rename(): verify that lower dentries are still OK after lock_rename()" |
| Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c |
| index 9bf804cc7bf0..8d27fd208f67 100644 |
| --- a/fs/ecryptfs/inode.c |
| +++ b/fs/ecryptfs/inode.c |
| @@ -128,13 +128,20 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry, |
| struct inode *inode) |
| { |
| 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 inode *lower_dir_inode; |
| int rc; |
| |
| - dget(lower_dentry); |
| - lower_dir_dentry = lock_parent(lower_dentry); |
| - rc = vfs_unlink(lower_dir_inode, lower_dentry, NULL); |
| + lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent); |
| + 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); |
| if (rc) { |
| printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc); |
| goto out_unlock; |
| @@ -142,10 +149,11 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry, |
| fsstack_copy_attr_times(dir, lower_dir_inode); |
| set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink); |
| inode->i_ctime = dir->i_ctime; |
| - d_drop(dentry); |
| out_unlock: |
| - unlock_dir(lower_dir_dentry); |
| dput(lower_dentry); |
| + inode_unlock(lower_dir_inode); |
| + if (!rc) |
| + d_drop(dentry); |
| return rc; |
| } |
| |
| @@ -519,22 +527,30 @@ static int ecryptfs_rmdir(struct inode *dir, struct dentry *dentry) |
| { |
| struct dentry *lower_dentry; |
| struct dentry *lower_dir_dentry; |
| + struct inode *lower_dir_inode; |
| int rc; |
| |
| lower_dentry = ecryptfs_dentry_to_lower(dentry); |
| - dget(dentry); |
| - lower_dir_dentry = lock_parent(lower_dentry); |
| - dget(lower_dentry); |
| - rc = vfs_rmdir(d_inode(lower_dir_dentry), lower_dentry); |
| - dput(lower_dentry); |
| - if (!rc && d_really_is_positive(dentry)) |
| + lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent); |
| + 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_rmdir(lower_dir_inode, lower_dentry); |
| + if (!rc) { |
| clear_nlink(d_inode(dentry)); |
| - fsstack_copy_attr_times(dir, d_inode(lower_dir_dentry)); |
| - set_nlink(dir, d_inode(lower_dir_dentry)->i_nlink); |
| - unlock_dir(lower_dir_dentry); |
| + fsstack_copy_attr_times(dir, lower_dir_inode); |
| + set_nlink(dir, lower_dir_inode->i_nlink); |
| + } |
| + dput(lower_dentry); |
| + inode_unlock(lower_dir_inode); |
| if (!rc) |
| d_drop(dentry); |
| - dput(dentry); |
| return rc; |
| } |
| |
| @@ -572,20 +588,22 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry, |
| struct dentry *lower_new_dentry; |
| struct dentry *lower_old_dir_dentry; |
| struct dentry *lower_new_dir_dentry; |
| - struct dentry *trap = NULL; |
| + struct dentry *trap; |
| struct inode *target_inode; |
| |
| if (flags) |
| 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_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); |
| + |
| trap = lock_rename(lower_old_dir_dentry, lower_new_dir_dentry); |
| + dget(lower_new_dentry); |
| rc = -EINVAL; |
| if (lower_old_dentry->d_parent != lower_old_dir_dentry) |
| goto out_lock; |
| @@ -613,11 +631,8 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry, |
| if (new_dir != old_dir) |
| fsstack_copy_attr_all(old_dir, d_inode(lower_old_dir_dentry)); |
| 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_old_dentry); |
| + unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry); |
| return rc; |
| } |
| |
| -- |
| 2.7.4 |
| |