Messages in this thread | | | Date | Tue, 6 Nov 2007 10:01:02 +0100 | From | Jan Kara <> | Subject | Re: [PATCH 16/27] r-o-bind-mounts-elevate-write-count-for-some-ioctls |
| |
> On Thu, 01 Nov 2007 16:08:47 -0700 > Dave Hansen <haveblue@us.ibm.com> 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 <jack@suse.cz> SuSE CR Labs - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |