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@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Djalal Harouni <tixxdz@xxxxxxxxxx>
> ---
> 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
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
