Patchwork fs/ext{3,4}: fix potential race when setversion ioctl updates inode

login
register
mail settings
Submitter Djalal Harouni
Date Jan. 6, 2012, 1 a.m.
Message ID <20120106010011.GA9028@dztty>
Download mbox | patch
Permalink /patch/134563/
State Not Applicable
Headers show

Comments

Djalal Harouni - Jan. 6, 2012, 1 a.m.
On Thu, Jan 05, 2012 at 12:42:05PM +0100, Jan Kara wrote:
> On Thu 05-01-12 01:40:09, Djalal Harouni wrote:
> > Only the reiserfs and ext{2,3,4} filesystems support this ioctl. The reiserfs
> > do not use mutexes at all, even in the REISERFS_IOC_SETFLAGS ioctl which will
> > test and set _all_ the possible values of the i_flags field.
> > Perhaps I should also send a patch for this ?
>   Yes, possibly reiserfs should use i_mutex for that ioctl.
For the reiserfs I've missed some locks.

It seems that reiserfs uses its own 'reiserfs_sb_info->lock' (reiserfs
super block data) to serialize writers in all the ioctl, even the GET
(readers) ioctl operations. But I also know that the VFS layer uses i_mutex
to protect inode changes, so ? I'm not sure, If I can test it I'll send a
patch.

> > And perhaps ext2 should also be updated.
>   Sure. Send a patch my way when you have it.
Here's the tested ext2 patch, thanks.

--------

From: Djalal Harouni <tixxdz@opendz.org>

ext2: protect inode changes in the SETVERSION and SETFLAGS ioctls

Unlock mutex after i_flags and i_ctime updates in the EXT2_IOC_SETFLAGS
ioctl.

Use i_mutex in the EXT2_IOC_SETVERSION ioctl to protect i_ctime and
i_generation updates and make the ioctl consistent since i_mutex is
also used in other places to protect timestamps and inode changes.

Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Jan Kara <jack@suse.cz>
Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/ext2/ioctl.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)
Jan Kara - Jan. 9, 2012, 3:03 p.m.
On Fri 06-01-12 02:00:11, Djalal Harouni wrote:
> On Thu, Jan 05, 2012 at 12:42:05PM +0100, Jan Kara wrote:
> > On Thu 05-01-12 01:40:09, Djalal Harouni wrote:
> > > Only the reiserfs and ext{2,3,4} filesystems support this ioctl. The reiserfs
> > > do not use mutexes at all, even in the REISERFS_IOC_SETFLAGS ioctl which will
> > > test and set _all_ the possible values of the i_flags field.
> > > Perhaps I should also send a patch for this ?
> >   Yes, possibly reiserfs should use i_mutex for that ioctl.
> For the reiserfs I've missed some locks.
> 
> It seems that reiserfs uses its own 'reiserfs_sb_info->lock' (reiserfs
> super block data) to serialize writers in all the ioctl, even the GET
> (readers) ioctl operations. But I also know that the VFS layer uses i_mutex
> to protect inode changes, so ? I'm not sure, If I can test it I'll send a
> patch.
> 
> > > And perhaps ext2 should also be updated.
> >   Sure. Send a patch my way when you have it.
> Here's the tested ext2 patch, thanks.
  Thanks I've taken it into my tree.

									Honza
> --------
> 
> From: Djalal Harouni <tixxdz@opendz.org>
> 
> ext2: protect inode changes in the SETVERSION and SETFLAGS ioctls
> 
> Unlock mutex after i_flags and i_ctime updates in the EXT2_IOC_SETFLAGS
> ioctl.
> 
> Use i_mutex in the EXT2_IOC_SETVERSION ioctl to protect i_ctime and
> i_generation updates and make the ioctl consistent since i_mutex is
> also used in other places to protect timestamps and inode changes.
> 
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
> ---
>  fs/ext2/ioctl.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
> index f81e250..b7f931f 100644
> --- a/fs/ext2/ioctl.c
> +++ b/fs/ext2/ioctl.c
> @@ -77,10 +77,11 @@ long ext2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		flags = flags & EXT2_FL_USER_MODIFIABLE;
>  		flags |= oldflags & ~EXT2_FL_USER_MODIFIABLE;
>  		ei->i_flags = flags;
> -		mutex_unlock(&inode->i_mutex);
>  
>  		ext2_set_inode_flags(inode);
>  		inode->i_ctime = CURRENT_TIME_SEC;
> +		mutex_unlock(&inode->i_mutex);
> +
>  		mark_inode_dirty(inode);
>  setflags_out:
>  		mnt_drop_write(filp->f_path.mnt);
> @@ -88,20 +89,29 @@ setflags_out:
>  	}
>  	case EXT2_IOC_GETVERSION:
>  		return put_user(inode->i_generation, (int __user *) arg);
> -	case EXT2_IOC_SETVERSION:
> +	case EXT2_IOC_SETVERSION: {
> +		__u32 generation;
> +
>  		if (!inode_owner_or_capable(inode))
>  			return -EPERM;
>  		ret = mnt_want_write(filp->f_path.mnt);
>  		if (ret)
>  			return ret;
> -		if (get_user(inode->i_generation, (int __user *) arg)) {
> +		if (get_user(generation, (int __user *) arg)) {
>  			ret = -EFAULT;
> -		} else {
> -			inode->i_ctime = CURRENT_TIME_SEC;
> -			mark_inode_dirty(inode);
> +			goto setversion_out;
>  		}
> +
> +		mutex_lock(&inode->i_mutex);
> +		inode->i_ctime = CURRENT_TIME_SEC;
> +		inode->i_generation = generation;
> +		mutex_unlock(&inode->i_mutex);
> +
> +		mark_inode_dirty(inode);
> +setversion_out:
>  		mnt_drop_write(filp->f_path.mnt);
>  		return ret;
> +	}
>  	case EXT2_IOC_GETRSVSZ:
>  		if (test_opt(inode->i_sb, RESERVATION)
>  			&& S_ISREG(inode->i_mode)
> -- 
> 1.7.1

Patch

diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
index f81e250..b7f931f 100644
--- a/fs/ext2/ioctl.c
+++ b/fs/ext2/ioctl.c
@@ -77,10 +77,11 @@  long ext2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		flags = flags & EXT2_FL_USER_MODIFIABLE;
 		flags |= oldflags & ~EXT2_FL_USER_MODIFIABLE;
 		ei->i_flags = flags;
-		mutex_unlock(&inode->i_mutex);
 
 		ext2_set_inode_flags(inode);
 		inode->i_ctime = CURRENT_TIME_SEC;
+		mutex_unlock(&inode->i_mutex);
+
 		mark_inode_dirty(inode);
 setflags_out:
 		mnt_drop_write(filp->f_path.mnt);
@@ -88,20 +89,29 @@  setflags_out:
 	}
 	case EXT2_IOC_GETVERSION:
 		return put_user(inode->i_generation, (int __user *) arg);
-	case EXT2_IOC_SETVERSION:
+	case EXT2_IOC_SETVERSION: {
+		__u32 generation;
+
 		if (!inode_owner_or_capable(inode))
 			return -EPERM;
 		ret = mnt_want_write(filp->f_path.mnt);
 		if (ret)
 			return ret;
-		if (get_user(inode->i_generation, (int __user *) arg)) {
+		if (get_user(generation, (int __user *) arg)) {
 			ret = -EFAULT;
-		} else {
-			inode->i_ctime = CURRENT_TIME_SEC;
-			mark_inode_dirty(inode);
+			goto setversion_out;
 		}
+
+		mutex_lock(&inode->i_mutex);
+		inode->i_ctime = CURRENT_TIME_SEC;
+		inode->i_generation = generation;
+		mutex_unlock(&inode->i_mutex);
+
+		mark_inode_dirty(inode);
+setversion_out:
 		mnt_drop_write(filp->f_path.mnt);
 		return ret;
+	}
 	case EXT2_IOC_GETRSVSZ:
 		if (test_opt(inode->i_sb, RESERVATION)
 			&& S_ISREG(inode->i_mode)