[PATCH] ecryptfs: don't ignore return value from lock_rename

[PATCH] ecryptfs: don't ignore return value from lock_rename

[Erez Zadok]

Tyler/Dustin,

Here's another patch: use return value from lock_rename() in
ecryptfs_rename, and properly check that return value.  I've tested it
briefly with ecryptfs: moved some files/directories at both the same level,
as well as into subdirectories which may share a common ancestor.  Things
seem to work, but perhaps more testing is needed.

BTW, identical code to this has been running in Unionfs for a couple of
years, so I'm reasonably confident that this patch is correct.

diffstat:
 inode.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Cheers,
Erez.

------------------------------------------------------------------------------

ecryptfs: don't ignore return value from lock_rename

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 429ca0b..e4dd0c6 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -614,6 +614,7 @@ 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;
 
 	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
 	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
@@ -621,7 +622,17 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	dget(lower_new_dentry);
 	lower_old_dir_dentry = dget_parent(lower_old_dentry);
 	lower_new_dir_dentry = dget_parent(lower_new_dentry);
-	lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
+	trap = lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
+	/* source should not be ancestor of target */
+	if (trap == lower_old_dentry) {
+		rc = -EINVAL;
+		goto out_lock;
+	}
+	/* target should not be ancestor of source */
+	if (trap == lower_new_dentry) {
+		rc = -ENOTEMPTY;
+		goto out_lock;
+	}
 	rc = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
 			lower_new_dir_dentry->d_inode, lower_new_dentry);
 	if (rc)

Re: [PATCH] ecryptfs: don't ignore return value from lock_rename

[Tyler Hicks]

On Sat Dec 05, 2009 at 09:17:09PM -0500, Erez Zadok (ezk@cs.sunysb.edu) was quoted:
> Tyler/Dustin,
> 
> Here's another patch: use return value from lock_rename() in
> ecryptfs_rename, and properly check that return value.  I've tested it
> briefly with ecryptfs: moved some files/directories at both the same level,
> as well as into subdirectories which may share a common ancestor.  Things
> seem to work, but perhaps more testing is needed.
> 
> BTW, identical code to this has been running in Unionfs for a couple of
> years, so I'm reasonably confident that this patch is correct.
> 
> diffstat:
>  inode.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> Cheers,
> Erez.
> 
> ------------------------------------------------------------------------------
> 
> ecryptfs: don't ignore return value from lock_rename
> 
> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

Looks correct to me - thanks!

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git#next

> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 429ca0b..e4dd0c6 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -614,6 +614,7 @@ 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;
> 
>  	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
>  	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
> @@ -621,7 +622,17 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	dget(lower_new_dentry);
>  	lower_old_dir_dentry = dget_parent(lower_old_dentry);
>  	lower_new_dir_dentry = dget_parent(lower_new_dentry);
> -	lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
> +	trap = lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
> +	/* source should not be ancestor of target */
> +	if (trap == lower_old_dentry) {
> +		rc = -EINVAL;
> +		goto out_lock;
> +	}
> +	/* target should not be ancestor of source */
> +	if (trap == lower_new_dentry) {
> +		rc = -ENOTEMPTY;
> +		goto out_lock;
> +	}
>  	rc = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
>  			lower_new_dir_dentry->d_inode, lower_new_dentry);
>  	if (rc)