Index: sys/fs/unionfs/union_subr.c =================================================================== --- sys/fs/unionfs/union_subr.c +++ sys/fs/unionfs/union_subr.c @@ -526,7 +526,7 @@ /* * Get the unionfs node status. - * You need exclusive lock this vnode. + * The vnode backing unp must be locked exclusive. */ void unionfs_get_node_status(struct unionfs_node *unp, struct thread *td, Index: sys/fs/unionfs/union_vnops.c =================================================================== --- sys/fs/unionfs/union_vnops.c +++ sys/fs/unionfs/union_vnops.c @@ -475,24 +475,44 @@ { struct unionfs_node *unp; struct unionfs_node_status *unsp; + struct vnode *vp; struct vnode *uvp; struct vnode *lvp; struct vnode *targetvp; struct ucred *cred; struct thread *td; int error; + bool downgrade; UNIONFS_INTERNAL_DEBUG("unionfs_open: enter\n"); KASSERT_UNIONFS_VNODE(ap->a_vp); error = 0; - unp = VTOUNIONFS(ap->a_vp); + vp = ap->a_vp; + unp = VTOUNIONFS(vp); uvp = unp->un_uppervp; lvp = unp->un_lowervp; targetvp = NULLVP; cred = ap->a_cred; td = ap->a_td; + downgrade = false; + + ASSERT_VOP_LOCKED(vp, __func__); + /* + * The executable loader path may call this function with + * vp locked shared. + */ + if (VOP_ISLOCKED(vp) != LK_EXCLUSIVE) { + downgrade = true; + /* + * If the vnode is doomed while the lock is dropped here, + * we assume that VOP_OPEN below will simply displatch to + * a no-op function. + */ + if (vn_lock(vp, LK_UPGRADE) != 0) + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); + } unionfs_get_node_status(unp, td, &unsp); @@ -540,13 +560,16 @@ unsp->uns_lower_opencnt++; unsp->uns_lower_openmode = ap->a_mode; } - ap->a_vp->v_object = targetvp->v_object; + vp->v_object = targetvp->v_object; } unionfs_open_abort: if (error != 0) unionfs_tryrem_node_status(unp, unsp); + if (downgrade) + vn_lock(vp, LK_DOWNGRADE | LK_RETRY); + UNIONFS_INTERNAL_DEBUG("unionfs_open: leave (%d)\n", error); return (error); @@ -562,23 +585,30 @@ struct vnode *vp; struct vnode *ovp; int error; - int locked; + bool downgrade; UNIONFS_INTERNAL_DEBUG("unionfs_close: enter\n"); KASSERT_UNIONFS_VNODE(ap->a_vp); - locked = 0; vp = ap->a_vp; unp = VTOUNIONFS(vp); cred = ap->a_cred; td = ap->a_td; + downgrade = false; + ASSERT_VOP_LOCKED(vp, __func__); if (VOP_ISLOCKED(vp) != LK_EXCLUSIVE) { + downgrade = true; + /* + * If the vnode is doomed while the lock is dropped here, + * we assume that VOP_CLOSE below will simply displatch to + * a no-op function. + */ if (vn_lock(vp, LK_UPGRADE) != 0) vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); - locked = 1; } + unionfs_get_node_status(unp, td, &unsp); if (unsp->uns_lower_opencnt <= 0 && unsp->uns_upper_opencnt <= 0) { @@ -618,7 +648,7 @@ unionfs_close_abort: unionfs_tryrem_node_status(unp, unsp); - if (locked != 0) + if (downgrade) vn_lock(vp, LK_DOWNGRADE | LK_RETRY); UNIONFS_INTERNAL_DEBUG("unionfs_close: leave (%d)\n", error); @@ -1444,6 +1474,11 @@ VNPASS(vrefcnt(ap->a_vp) > 0, ap->a_vp); error = unionfs_relookup_for_delete(ap->a_dvp, cnp, td); vn_lock(ap->a_vp, LK_EXCLUSIVE | LK_RETRY); + /* + * VOP_RMDIR is dispatched against udvp, so if uvp became + * doomed while the lock was dropped above the target + * filesystem may not be able to cope. + */ if (error == 0 && VN_IS_DOOMED(uvp)) error = ENOENT; if (error == 0) @@ -1906,7 +1941,8 @@ if ((revlock = unionfs_get_llt_revlock(vp, flags)) == 0) panic("unknown lock type: 0x%x", flags & LK_TYPE_MASK); - if ((vp->v_iflag & VI_OWEINACT) != 0) + if ((flags & LK_TYPE_MASK) != LK_DOWNGRADE && + (vp->v_iflag & VI_OWEINACT) != 0) flags |= LK_NOWAIT; /* Index: tools/test/stress2/misc/unionfs10.sh =================================================================== --- /dev/null +++ tools/test/stress2/misc/unionfs10.sh @@ -0,0 +1,87 @@ +#!/bin/sh + +# +# Copyright (c) 2022 Jason Harmening +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. +# + +[ `id -u ` -ne 0 ] && echo "Must be root!" && exit 1 + +# Regression test: + +# There is an issue, namely, when the lower file is already +# opened r/w, but its nullfs alias is executed. This situation obviously +# shall result in ETXTBUSY, but it currently does not. + +# Based on nullfs10.sh by pho@, original test scenario by kib@ + +. ../default.cfg + +md1=$mdstart +md2=$((md1 + 1)) +mp1=/mnt$md1 +mp2=/mnt$md2 +mkdir -p $mp1 $mp2 +set -e +for i in $mp1 $mp2; do + mount | grep -q "on $i " && umount -f $i +done +for i in $md1 $md2; do + mdconfig -l | grep -q md$i && mdconfig -d -u $i +done + +mdconfig -a -t swap -s 4g -u $md1 +mdconfig -a -t swap -s 4g -u $md2 +newfs $newfs_flags -n md$md1 > /dev/null +newfs $newfs_flags -n md$md2 > /dev/null +mount /dev/md$md1 $mp1 +mount /dev/md$md2 $mp2 + +mount -t unionfs -o noatime $mp1 $mp2 +set +e + +mount | grep -E "$mp1|$mp2" + +chmod 777 $mp1 +chmod 777 $mp2 + +cp /bin/ls $mp1 +chmod +w $mp1/ls +sleep 2 >> $mp1/ls & +sleep .5 + +# This line should cause a "$mp2/ls: Text file busy" error +$mp2/ls -l /bin/ls $mp1 $mp2 && echo FAIL || echo OK +kill $! +wait + +while mount | grep -q "$mp2 "; do + umount $mp2 || sleep 1 +done + +while mount | grep -q "$mp1 "; do + umount $mp1 || sleep 1 +done +mdconfig -d -u $md2 +mdconfig -d -u $md1 Index: tools/test/stress2/misc/unionfs11.sh =================================================================== --- /dev/null +++ tools/test/stress2/misc/unionfs11.sh @@ -0,0 +1,87 @@ +#!/bin/sh + +# +# Copyright (c) 2022 Jason Harmening +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. +# + +[ `id -u ` -ne 0 ] && echo "Must be root!" && exit 1 + +# Regression test: + +# There is an issue, namely, when the lower file is already +# opened r/w, but its nullfs alias is executed. This situation obviously +# shall result in ETXTBUSY, but it currently does not. + +# Based on nullfs10.sh by pho@, original test scenario by kib@ + +. ../default.cfg + +md1=$mdstart +md2=$((md1 + 1)) +mp1=/mnt$md1 +mp2=/mnt$md2 +mkdir -p $mp1 $mp2 +set -e +for i in $mp1 $mp2; do + mount | grep -q "on $i " && umount -f $i +done +for i in $md1 $md2; do + mdconfig -l | grep -q md$i && mdconfig -d -u $i +done + +mdconfig -a -t swap -s 4g -u $md1 +mdconfig -a -t swap -s 4g -u $md2 +newfs $newfs_flags -n md$md1 > /dev/null +newfs $newfs_flags -n md$md2 > /dev/null +mount /dev/md$md1 $mp1 +mount /dev/md$md2 $mp2 + +mount -t unionfs -o noatime $mp1 $mp2 +set +e + +mount | grep -E "$mp1|$mp2" + +chmod 777 $mp1 +chmod 777 $mp2 + +cp /bin/ls $mp2 +chmod +w $mp2/ls +sleep 2 >> $mp2/ls & +sleep .5 + +# This line should cause a "$mp1/ls: Text file busy" error +$mp1/ls -l /bin/ls $mp1 $mp2 && echo FAIL || echo OK +kill $! +wait + +while mount | grep -q "$mp2 "; do + umount $mp2 || sleep 1 +done + +while mount | grep -q "$mp1 "; do + umount $mp1 || sleep 1 +done +mdconfig -d -u $md2 +mdconfig -d -u $md1 Index: tools/test/stress2/misc/unionfs12.sh =================================================================== --- /dev/null +++ tools/test/stress2/misc/unionfs12.sh @@ -0,0 +1,88 @@ +#!/bin/sh + +# +# Copyright (c) 2022 Jason Harmening +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. +# + +[ `id -u ` -ne 0 ] && echo "Must be root!" && exit 1 + +# Regression test: + +# There is an issue, namely, when the lower file is already +# opened r/w, but its nullfs alias is executed. This situation obviously +# shall result in ETXTBUSY, but it currently does not. + +# Based on nullfs10.sh by pho@, original test scenario by kib@ + +. ../default.cfg + +md1=$mdstart +md2=$((md1 + 1)) +mp1=/mnt$md1 +mp2=/mnt$md2 +mkdir -p $mp1 $mp2 +set -e +for i in $mp1 $mp2; do + mount | grep -q "on $i " && umount -f $i +done +for i in $md1 $md2; do + mdconfig -l | grep -q md$i && mdconfig -d -u $i +done + +mdconfig -a -t swap -s 4g -u $md1 +mdconfig -a -t swap -s 4g -u $md2 +newfs $newfs_flags -n md$md1 > /dev/null +newfs $newfs_flags -n md$md2 > /dev/null +mount /dev/md$md1 $mp1 +mount /dev/md$md2 $mp2 + +mkdir $mp2/shadow +cp /bin/ls $mp2/shadow/ +mount -t unionfs -o noatime $mp1 $mp2 +set +e + +mount | grep -E "$mp1|$mp2" + +chmod 777 $mp1 +chmod 777 $mp2 + +chmod +w $mp2/shadow/ls +sleep 2 >> $mp2/shadow/ls & +sleep .5 + +# This line should cause a "$mp1/shadow/ls: Text file busy" error +$mp1/shadow/ls -l /bin/ls $mp1 $mp2 && echo FAIL || echo OK +kill $! +wait + +while mount | grep -q "$mp2 "; do + umount $mp2 || sleep 1 +done + +while mount | grep -q "$mp1 "; do + umount $mp1 || sleep 1 +done +mdconfig -d -u $md2 +mdconfig -d -u $md1