Re: More duplicate code, vnode locking question

[Antti Kantee <pooka%cs.hut.fi@localhost>]

On Tue Apr 21 2009 at 00:25:32 +0300, Elad Efrat wrote:
> Hi,
> 
> Another piece of duplicated code in our file-systems is a snippet like
> the following, taken from ffs_vfsops.c:
> 
>     381       /*
>     382        * If mount by non-root, then verify that user has necessary
>     383        * permissions on the device.
>     384        */
>     385       if (error == 0 && kauth_authorize_generic(l->l_cred,
>     386           KAUTH_GENERIC_ISSUSER, NULL) != 0) {
>     387               accessmode = VREAD;
>     388               if (update ?
>     389                   (mp->mnt_iflag & IMNT_WANTRDWR) != 0 :
>     390                   (mp->mnt_flag & MNT_RDONLY) == 0)
>     391                       accessmode |= VWRITE;
>     392               vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY);
>     393               error = VOP_ACCESS(devvp, accessmode, l->l_cred);
>     394               VOP_UNLOCK(devvp, 0);
>     395       }
>       
> Here, too, I would like to remove all duplicates and add a single
> function to be used from all places.
> 
> However, this time vnode locking is involved, and I'm not sure what
> the rules are about functions that are not in the vnode interface
> (vnode_if?) that lock/unlock vnodes.

vnodes are always locked from the root up.  When you have two which are
not (parent,child), it gets tricky, since you need to know which you
have to lock first.  See ufs_checkpath() used by the rename VOP for your
daily share of fun.  Notable, it does it only inside the file system.

The mount code is a bit special and has to make the mountpoint lock
recursively to prevent potential deadlock against itself when looking
up the device vp.

> In other words, what I have in mind is adding the following:
> 
>       int
>       common_mount_allowed(kauth_cred_t cred, struct vnode *vp,
>           bool vnode_locked, int mode)
>       {
>               /* if root, always allow. */
>               ...
> 
>               if (!vnode_locked)
>                       vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
> 
>               error = VOP_ACCESS(vp, mode, cred);
> 
>               if (!vnode_locked)
>                       VOP_UNLOCK(vp, 0);
> 
>               return (error);
>       }
> 
> and replacing the above snippet from ffs_vfsops.c with something like:
> 
>       if (error == 0) {
>               accessmode = VREAD;
>               ...
>               /* devvp isn't locked at this point, so pass "false". */
>               error = common_mount_allowed(l->l_cred, devvp, false,
>                   accessmode);
>       }
> 
> Is this considered okay? or do we avoid such things?

That is an unnecessarily complex interface.  Just require that the vnode
is always (un)locked.

> Also, it seems that some file-systems call VOP_OPEN() without devvp
> being locked. Compare, for example, ffs_mount() and efs_mount(). Is
> this possible, or am I missing something? :)

specfs doesn't do locking so in practise it doesn't matter.  If it did,
you could trick the kernel into deadlock (not a very likely scenario,
but)... so it's your call really: violate the locking protocol of VOPs
or violate the locking order of vnodes.

It's a shame that you rushed the common_chmod stuff in.  dholland has been
working on componentizing certain parts of file systems, and committing
things with allowing less than two days for review is too quick.  Even I
would've wanted to review the diff, but have been quite busy in the last
few days.