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()

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 also suspect the conditional locking in gfs2_do_permission() could
be cleaned up, due to the removal of the implicit recursion.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Steven Whitehouse <swhiteho@redhat.com>
---
 fs/gfs2/inode.c     |    6 +++---
 fs/gfs2/inode.h     |    1 +
 fs/gfs2/ops_file.c  |   11 +++++++++--
 fs/gfs2/ops_inode.c |   18 +++++++++++++-----
 4 files changed, 26 insertions(+), 10 deletions(-)

Index: linux-2.6/fs/gfs2/inode.c
===================================================================
--- linux-2.6.orig/fs/gfs2/inode.c	2008-05-21 15:45:11.000000000 +0200
+++ linux-2.6/fs/gfs2/inode.c	2008-05-21 15:45:45.000000000 +0200
@@ -504,7 +504,7 @@ struct inode *gfs2_lookupi(struct inode 
 	}
 
 	if (!is_root) {
-		error = permission(dir, MAY_EXEC, NULL);
+		error = gfs2_do_permission(dir, MAY_EXEC);
 		if (error)
 			goto out;
 	}
@@ -667,7 +667,7 @@ static int create_ok(struct gfs2_inode *
 {
 	int error;
 
-	error = permission(&dip->i_inode, MAY_WRITE | MAY_EXEC, NULL);
+	error = gfs2_do_permission(&dip->i_inode, MAY_WRITE | MAY_EXEC);
 	if (error)
 		return ...
[ message continues ]

Hi,

That looks ok, but I wonder do we really need gfs2_do_permission() and
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
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.


--

[ message continues ]

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

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()

Yes, that would help.

Thanks,
Miklos
--

[ message continues ]

The NFS ->lookup inside filldir recursion is still there, but I plan to
fix that for .27.  You'll be Cc'ed on that patch.

--

[ message continues ]

Bad idea.  You're duplicating bits out of permission for no good
reason.  I spent quite a bit of effort to make sure we don't have
this duplicated logic around.

--

[ message continues ]

In this case you are wrong.  Look at the ugly conditional locking
gfs2_permission() does, which is probably due to the fact that it's
doing a recursion via calling permission() from inside already locked
parts in the filesystem.

That's _much_ worse than a duplicated IS_IMMUTABLE() call.  Which btw,
is a filesystem implementation detail: it needs to re-check the
immutability of the file after it has been locked.  I'm not even sure
it's strictly needed.  Steven?

Generally this sort of recursion through the VFS is ugly and
unnecessary, it's much better to provide helper for what the VFS is
doing if there's a lot of duplication.  But in this case there's
really no point in that at all.

Miklos
--

[ message continues ]

And that's true of lookup_one_len() as well, the last bit of horror
left in this permission() saga.  It's called from lots of places,
often for doing things it's not at all meant to do (e.g. where caller
_knows_ it will return negative, etc...).  One day maybe I'll have
strength to clean that up as well..., but not now.

Miklos
--

[ message continues ]

lookup_one_len is not just a horror for permission, it's a nighmare
in general.  I'm not convinced any use of it except for silly renames
is actually valid.

--

[ message continues ]

Hi,

Given the fact that (a) its only a very minor change and (b) as soon as
we have a solution to what we really want to do:

 - inode/file operation:
   - Do lookup via VFS
   - Get GFS2 glock
     - Do perm check via VFS
     - Do actual operation
   - Drop GFS2 glock

as opposed to the current situation of:

 - Do lookup via VFS:
   - Get GFS2 glock
   - Do perm check
   - Drop GFS2 glock
 - inode/file operation:
   - Get GFS2 glock
   - Recheck perms
   - Do the actual operation
   - Drop GFS2 glock

then the rechecking of perms will no longer be required anyway. I'm
inclined to agree with Miklos and say that its ok, but strictly on the

Indeed I've spent a lot of time tracking down these cases and at least
now its possible to see them all by greping for
gfs2_glock_is_locked_by_me() in gfs2, whereas previously these cases
were hidden and less obvious,

Steve.


--

[ message continues ]

Well, fuse/nfs already do something similar, except they have their
actual permission checking in the server, as opposed to the vfs.  They
basically do:

 ->permission() does nothing

 ->foo_operation() does everything, including permission checking

The reality is a bit more complicated, and both nfs and fuse do
sometimes check permissions in ->permission() but most of the cases,
when they know that the permission will be checked later anyway they
just omit it.

Of course this would mean, that for example the LSM security checks
are not done within the gfs locked region.  Does that matter?

Miklos
--

[ message continues ]

The conditional locking will have to stay anyway until the NFS issues
are sorted out.  And as Steve mention I'd rather eventually sort out
the need why gfs2 needs these permission/foo_permission calls at all.

Currently out VFS support for clustered filesystem sucks quite badly,
and we need some way to allow callouts into cluster locks around the
whole namespace operation so we can rely on a single permission check.

That beeing said once the nfs issues are sorted out you patch might
not be too bad it it kills all remaining recursive locking in gfs.

But that's left up to Steve to decide.

--

[ message continues ]