> 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