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

[Miklos Szeredi]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/