zfs code - Sep 2007 - Code/comment mismatch in delegated administration code.

In zfs_mount() function, when we process a mount by a regular user
through the delegated administration, the comment states:

	/*
	 * Make sure user is the owner of the mount point
	 * or has sufficient privileges.
	 */

This makes sense, but the code doesn''t match the comment. The code
ensures that user is the owner of the mount point _and_ can write to the
directory.
Or does "has sufficient privileges" means that he has PRIV_FILE_OWNER
privilege?

IMHO if either of those two (is the owner or can write) is true, we
should allow the mount. Am I right? If I am right, the patch below
implements my thinking.

--- uts/common/fs/zfs/zfs_vfsops.c
+++ uts/common/fs/zfs/zfs_vfsops.c
@@ -608,11 +608,9 @@
				goto out;
			}

-			if (error = secpolicy_vnode_owner(cr, vattr.va_uid)) {
-				goto out;
-			}
-
-			if (error = VOP_ACCESS(mvp, VWRITE, cr, td)) {
+			if (secpolicy_vnode_owner(cr, vattr.va_uid) != 0 &&
+			    VOP_ACCESS(mvp, VWRITE, cr, td) != 0) {
+				error = EPERM;
				goto out;
			}

-- 
Pawel Jakub Dawidek                       http://www.wheel.pl
pjd at FreeBSD.org                           http://www.FreeBSD.org
FreeBSD committer                         Am I Evil? Yes, I Am!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
URL:
<http://mail.opensolaris.org/pipermail/zfs-code/attachments/20070904/93c05f33/attachment.bin>

Pawel Jakub Dawidek wrote:
> In zfs_mount() function, when we process a mount by a regular user
> through the delegated administration, the comment states:
> 
> 	/*
> 	 * Make sure user is the owner of the mount point
> 	 * or has sufficient privileges.
> 	 */
> 
> This makes sense, but the code doesn''t match the comment. The code
> ensures that user is the owner of the mount point _and_ can write to the
> directory.
> Or does "has sufficient privileges" means that he has
PRIV_FILE_OWNER
> privilege?
> 
> IMHO if either of those two (is the owner or can write) is true, we
> should allow the mount. Am I right? If I am right, the patch below
> implements my thinking.
> 
This seems reasonable to me.  I will open a bug for this.

   -Mark

> --- uts/common/fs/zfs/zfs_vfsops.c
> +++ uts/common/fs/zfs/zfs_vfsops.c
> @@ -608,11 +608,9 @@
> 				goto out;
> 			}
> 
> -			if (error = secpolicy_vnode_owner(cr, vattr.va_uid)) {
> -				goto out;
> -			}
> -
> -			if (error = VOP_ACCESS(mvp, VWRITE, cr, td)) {
> +			if (secpolicy_vnode_owner(cr, vattr.va_uid) != 0 &&
> +			    VOP_ACCESS(mvp, VWRITE, cr, td) != 0) {
> +				error = EPERM;
> 				goto out;
> 			}
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> zfs-code mailing list
> zfs-code at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/zfs-code