Re: [patch 04/14] gfs2: dont call permission()

[Steven Whitehouse <swhiteho@redhat.com>]

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@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?

> 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.