Hi, On Wed, 2008-05-21 at 19:15 +0200, Miklos Szeredi wrote: > plain text document attachment (gfs2_permission_fix.patch) > From: Miklos Szeredi <mszeredi@xxxxxxx> > > 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? > 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. 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, Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
