NetBSD-Users archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: mount_union(8) vs. open(O_RDWR)



> On 4. Dec 2021, at 09:51, Greg A. Woods <woods%planix.ca@localhost> wrote:
> 
> The following seems to work, for the simplest cases at least.
> 
> I would appreciate if someone who knows this code could have a look to
> see if I've missed any steps, or botched any locks, and to answer the
> embedded questions.
> 
> --- union_vnops.c.~1.74.~	2021-03-07 17:23:02.000000000 -0800
> +++ union_vnops.c	2021-12-04 00:36:56.318634542 -0800
> @@ -622,6 +622,12 @@
> 				un->un_uppervp->v_writecount++;
> 				mutex_exit(un->un_uppervp->v_interlock);
> 			}
> +#ifdef UNION_DIAGNOSTIC
> +			else {
> +				printf("union_open: failed to open upper file for write %s: %d\n",
> +				       un->un_path, error);
> +			}
> +#endif
> 			return (error);
> 		}
> 
> @@ -636,6 +642,12 @@
> 		error = VOP_OPEN(tvp, mode, cred);
> 		VOP_UNLOCK(tvp);
> 
> +#ifdef UNION_DIAGNOSTIC
> +		if (error != 0) {
> +			printf("union_open: failed to open lower file %s: %d\n",
> +			       un->un_path, error);
> +		}
> +#endif
> 		return (error);
> 	}
> 	/*
> @@ -651,6 +663,12 @@
> 		tvp->v_writecount++;
> 		mutex_exit(tvp->v_interlock);
> 	}
> +#ifdef UNION_DIAGNOSTIC
> +	if (error != 0) {
> +		printf("union_open: failed to open upper file %s: %d\n",
> +		       un->un_path, error);
> +	}
> +#endif
> 
> 	return (error);
> }
> @@ -718,17 +736,63 @@
> 	struct union_mount *um = MOUNTTOUNIONMOUNT(vp->v_mount);
> 
> 	/*
> -	 * Disallow write attempts on read-only file systems;
> -	 * unless the file is a socket, fifo, or a block or
> -	 * character device resident on the file system.
> +	 * Disallow write attempts on read-only file systems; unless the file is
> +	 * readable by the user, or is a socket, fifo, or a block or character
> +	 * device resident on the file system.
> 	 */
> -	if (ap->a_accmode & VWRITE) {
> +	if (ap->a_accmode & VWRITE && un->un_uppervp == NULLVP) {
> 		switch (vp->v_type) {
> 		case VDIR:
> 		case VLNK:
> 		case VREG:
> -			if (vp->v_mount->mnt_flag & MNT_RDONLY)
> +			/*
> +			 * XXX ToDo:  First check for read (and write?) access
> +			 * on the underlying file system object (un->un_lowervp)
> +			 *
> +			 * If un->un_lowervp is readable then copy it up and
> +			 * finally check the upper node (i.e. continue to the
> +			 * next if).
> +			 *
> +			 * Does this work for VLNK and VDIR too?
> +			 */
> +			if (un->un_lowervp != NULLVP) {
> +				struct vop_access_args ra;
> +				int mode = ap->a_accmode;
> +
> +				ra = *ap;
> +				ra.a_vp = un->un_lowervp;
> +				ra.a_accmode &= !VWRITE;
> +				ra.a_accmode |= VREAD;
> +				vn_lock(un->un_lowervp, LK_EXCLUSIVE | LK_RETRY); /* xxx ? */
> +				error = VCALL(un->un_lowervp, VOFFSET(vop_access), &ra);
> +				VOP_UNLOCK(un->un_lowervp); /* xxx ? */
> +				if (error != 0) {
> +#ifdef UNION_DIAGNOSTIC
> +					printf("union_access: read access on lvp denied for %s: %d\n",
> +					       un->un_path, error);
> +#endif
> +					return (error);
> +				}
> +				/*
> +				 * XXX in theory we only need to make the vnode,
> +				 * not copy the file, but there's no logic
> +				 * elsewhere to complete the copy later if/when
> +				 * an open is attempted.
> +				 */
> +				error = union_copyup(un, ((mode & O_TRUNC) == 0), ap->a_cred, curlwp);
> +				if (error != 0) {
> +#ifdef UNION_DIAGNOSTIC
> +					printf("union_access: failed copyup for %s: %d\n",
> +					       un->un_path, error);
> +#endif
> +					return (error);
> +				}
> +			} else if (vp->v_mount->mnt_flag & MNT_RDONLY) {
> +#ifdef UNION_DIAGNOSTIC
> +				printf("union_access: EROFS for %s\n", un->un_path);
> +#endif
> 				return (EROFS);
> +			}
> 			break;
> 		case VBAD:
> 		case VBLK:
> @@ -744,7 +808,14 @@
> 
> 	if ((vp = un->un_uppervp) != NULLVP) {
> 		ap->a_vp = vp;
> -		return (VCALL(vp, VOFFSET(vop_access), ap));
> +		error = VCALL(vp, VOFFSET(vop_access), ap);
> +#ifdef UNION_DIAGNOSTIC
> +		if (error) {
> +			printf("union_access: failed upper access check for %s: %d\n",
> +			       un->un_path, error);
> +		}
> +#endif
> +		return (error);
> 	}
> 
> 	if ((vp = un->un_lowervp) != NULLVP) {
> @@ -758,8 +829,13 @@
> 			}
> 		}
> 		VOP_UNLOCK(vp);
> -		if (error)
> +		if (error) {
> +#ifdef UNION_DIAGNOSTIC
> +			printf("union_access: failed lower access check for %s: %d\n",
> +			       un->un_path, error);
> +#endif
> 			return (error);
> +		}
> 	}
> 
> 	return (error);
> @@ -1231,7 +1307,7 @@
> 				 */
> 				vp  = NULLVP;
> 				if (dun->un_uppervp == NULLVP)
> -					 panic("union: null upperdvp?");
> +					 panic("union: null uppervp?");
> 				error = relookup(ap->a_dvp, &vp, ap->a_cnp, 0);
> 				if (error) {
> 					VOP_UNLOCK(ap->a_vp);
> @@ -1242,6 +1318,10 @@
> 					 * The name we want to create has
> 					 * mysteriously appeared (a race?)
> 					 */
> +#ifdef UNION_DIAGNOSTIC
> +					printf("union_link: %s: already exists?\n",
> +					       un->un_path);
> +#endif
> 					error = EEXIST;
> 					VOP_UNLOCK(ap->a_vp);
> 					vput(vp);
> 
> 
> 
> [   2.3112534] root file system type: cd9660
> [   2.3262533] kern.module.path=/stand/amd64/9.99.81/modules
> [   2.3362553] warning: no /dev/console
> [   2.3362553] warning: no /dev/constty
> Created tmpfs /dev (2064384 byte, 4000 inodes)
> [   4.2142574] union_mount(mp = 0xffff888007b3e000)
> [   4.2142574] union_mount: from <above>:/tmp/uvar, on /var
> [   4.2142574] union_statvfs(mp = 0xffff888007b3e000, lvp = 0xffff888008cfcd40, uvp = 0xffff888008cfcac0)
> [   4.3342534] union_newsize: wtmpx: lower size now 0
> [   4.3352568] union: copied up wtmpx
> [   4.3352568] union_newsize: wtmpx: upper size now 520
> [   4.3352568] union_newsize: wtmp: lower size now 0
> [   4.3352568] union: copied up wtmp
> [   4.3402531] union_newsize: wtmp: upper size now 0
> [   4.3452562] union_newsize: wtmp: upper size now 40
> [   4.3502564] union_newsize: utmpx: lower size now 0
> [   4.3502564] union: copied up utmpx
> [   4.3502564] union_newsize: utmpx: upper size now 0
> [   4.3602577] union_newsize: utmpx: upper size now 520
> [   4.3602577] union_newsize: utmpx: upper size now 1040
> [   4.3602577] union_newsize: utmpx: upper size now 1560
> [   4.3742532] union_newsize: utmpx: upper size now 2080
> init: kernel security level changed from 0 to 1
> [   4.4112513] union_newsize: utmpx: upper size now 2600
> 
> 
> --
> 					Greg A. Woods <gwoods%acm.org@localhost>
> 
> Kelowna, BC     +1 250 762-7675           RoboHack <woods%robohack.ca@localhost>
> Planix, Inc. <woods%planix.com@localhost>     Avoncote Farms <woods%avoncote.ca@localhost>

Ok some comments:

- union_copyup() works and is needed for regular nodes only.

- it copies the node attributes up so checking the access on
  the (copied) upper node should be sufficient.

- the switch() on top checks for read-only mounted upper layer,
  not the lower layer.

The attached diff just copies up in the VWRITE/VREG case and
this should be sufficient -- please give it a try.

--
J. Hannken-Illjes - hannken%mailbox.org@localhost

Attachment: 002_union_access.diff
Description: Binary data

Attachment: signature.asc
Description: Message signed with OpenPGP



Home | Main Index | Thread Index | Old Index