> On Thu, 01 Nov 2007 16:08:47 -0700
> Dave Hansen <[EMAIL PROTECTED]> wrote:
>
> > Some ioctl()s can cause writes to the filesystem. Take these, and make them
> > use mnt_want/drop_write() instead.
> >
> > We need to pass the filp one layer deeper in XFS, but somebody _just_ pulled
> > it out in February because nobody was using it, so I don't feel guilty for
> > adding it back.
>
> See, when we combine this patch with Jan's
> forbid-user-to-change-file-flags-on-quota-files.patch we silently add bugs
> to five filesystems. Lessons:
>
> - never ever ever do `return' from deep in the guts of a function. This
> is a *classic* instance of the maintainability risks which this practice
> introduces.
OK, lesson learned ;) Thanks for spotting this. But if there already
was a label like:
err_out:
return err;
And I would have just added a place which does 'goto err_out', then
the same problem could arise... But I agree it's less likely than if we
do just return err;.
> - this whole elevate-the-write-count-in-a-zillion-places stuff is quite
> fragile.
>
> diff -puN
> fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> fs/ext2/ioctl.c
> ---
> a/fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/ext2/ioctl.c
> @@ -57,7 +57,8 @@ int ext2_ioctl (struct inode * inode, st
> /* Is it quota file? Do not allow user to mess with it */
> if (IS_NOQUOTA(inode)) {
> mutex_unlock(&inode->i_mutex);
> - return -EPERM;
> + ret = -EPERM;
> + goto setflags_out;
> }
> oldflags = ei->i_flags;
>
> diff -puN
> fs/ext3/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> fs/ext3/ioctl.c
> ---
> a/fs/ext3/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/ext3/ioctl.c
> @@ -60,7 +60,8 @@ int ext3_ioctl (struct inode * inode, st
> /* Is it quota file? Do not allow user to mess with it */
> if (IS_NOQUOTA(inode)) {
> mutex_unlock(&inode->i_mutex);
> - return -EPERM;
> + err = -EPERM;
> + goto flags_out;
> }
> oldflags = ei->i_flags;
>
> diff -puN
> fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> fs/ext4/ioctl.c
> ---
> a/fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/ext4/ioctl.c
> @@ -59,7 +59,8 @@ int ext4_ioctl (struct inode * inode, st
> /* Is it quota file? Do not allow user to mess with it */
> if (IS_NOQUOTA(inode)) {
> mutex_unlock(&inode->i_mutex);
> - return -EPERM;
> + err = -EPERM;
> + goto flags_out;
> }
> oldflags = ei->i_flags;
>
> diff -puN
> fs/jfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> fs/jfs/ioctl.c
> ---
> a/fs/jfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/jfs/ioctl.c
> @@ -85,8 +85,10 @@ int jfs_ioctl(struct inode * inode, stru
> flags &= ~JFS_DIRSYNC_FL;
>
> /* Is it quota file? Do not allow user to mess with it */
> - if (IS_NOQUOTA(inode))
> - return -EPERM;
> + if (IS_NOQUOTA(inode)) {
> + err = -EPERM;
> + goto setflags_out;
> + }
> jfs_get_inode_flags(jfs_inode);
> oldflags = jfs_inode->mode2;
>
> diff -puN
> fs/reiserfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> fs/reiserfs/ioctl.c
> _
Why there's no modification for reiserfs?
Honza
--
Jan Kara <[EMAIL PROTECTED]>
SuSE CR Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
