Messages in this thread | | | Subject | Re: [patch 04/14] gfs2: dont call permission() | From | Miklos Szeredi <> | Date | Fri, 23 May 2008 11:30:59 +0200 |
| |
> On Wed, 2008-05-21 at 19:15 +0200, Miklos Szeredi wrote: > > plain text document attachment (gfs2_permission_fix.patch) > > From: Miklos Szeredi <mszeredi@suse.cz> > > > > GFS2 calls permission() to verify permissions after locks on the files > > have been taken. > > > > For this it's sufficient to call gfs2_permission() instead. This > > results in the following changes: > > > > - IS_RDONLY() check is not performed > > - IS_IMMUTABLE() check is not performed > > - devcgroup_inode_permission() is not called > > - security_inode_permission() is not called > > > > IS_RDONLY() should be unnecessary anyway, as the per-mount read-only > > flag should provide protection against read-only remounts during > > operations. do_gfs2_set_flags() has been fixed to perform > > mnt_want_write()/mnt_drop_write() to protect against remounting > > read-only. > > > > IS_IMMUTABLE has beed added to gfs2_do_permission() > > > That looks ok, but I wonder do we really need gfs2_do_permission() and > gfs2_permission when the only difference seems to be one argument?
Later in this series ->permission() is changed to take a dentry as the first argument, so a separate function would've had to be reintroduced anyway.
> > Repeating the security checks seems to be pointless, as they don't > > normally change, and if they do, it's independent of the filesystem > > state. > > > I hope eventually we can fix this by allowing GFS2 to do its own > lookups, via a suitable VFS library function. I understand that is the > preferred option to replace "open intents" (which we don't currently use > anyway) in the longer term. > > > I also suspect the conditional locking in gfs2_do_permission() could > > be cleaned up, due to the removal of the implicit recursion. > > > In order to be sure we'd have to check that there are no NFS code paths > left which can reach this code. That has usually been the reason for > conditional locking.
OK, my impression was that in this case the conditional locking was because of things like:
gfs2_create() gfs2_createi() create_ok() permission() gfs2_permission()
So moving the locing out of gfs2_do_permission() into gfs_permission() these cases should be fixed. I don't know about NFS.
> In general the patch looks ok to me, and since it doesn't appear to > depend on anything else, I can drop it in my GFS2 git tree if that would > be helpful at this stage,
Yes, that would help.
Thanks, Miklos
| |