Hi Eric,
Something wrong is in this patch. Pls, take a look at this script:
[root@fc24 ~]# unshare -m bash -x xxx.sh
+ set -x -e -m
+ mount --make-rprivate /
+ mount --make-shared /
+ mount -t tmpfs xxx /mnt
+ mount --make-private /mnt
+ mkdir /mnt/yyy
+ mount -t tmpfs xxx /mnt/yyy
+ sleep 1
+ unshare --propagation unchanged -m sleep 1000
+ pid=452
+ umount /mnt/yyy
+ umount /mnt/
umount: /mnt/: target is busy
(In some cases useful info about processes that
use the device is found by lsof(8) or fuser(1).)
+ echo FAIL
FAIL
+ kill 452
+ wait
xxx.sh: line 15: 452 Terminated unshare --propagation
unchanged -m sleep 1000
[root@fc24 ~]# cat xxx.sh
set -x -e -m
mount --make-rprivate /
mount --make-shared /
mount -t tmpfs xxx /mnt
mount --make-private /mnt
mkdir /mnt/yyy
mount -t tmpfs xxx /mnt/yyy
unshare --propagation unchanged -m sleep 1000 &
sleep 1
pid=$!
umount /mnt/yyy
umount /mnt/ || echo FAIL
kill $pid
wait
On Thu, Jan 12, 2017 at 05:18:12AM +1300, Eric W. Biederman wrote:
>
> When I look at what propagate_mount_busy is trying to do and I look
> at the code closely I discover there is a great disconnect between the
> two. In the ordinary non-propagation case propagate_mount_busy has
> been verifying that there are no submounts and that there are no
> extraneous references on the mount.
>
> For mounts that the unmount would propagate to propagate_mount_busy has
> been verifying that there are no extraneous references only if there
> are no submounts. Which is nonsense.
>
> Thefore rework the logic in propgate_mount_busy so that for each
> mount it examines it considers that mount busy if that mount has
> children or if there are extraneous references to that mount.
>
> While this check was incorrect we could leak mounts instead of simply
> failing umount.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: a05964f3917c ("[PATCH] shared mounts handling: umount")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> ---
>
> If you don't figure this fix is worth it after all of this time please
> let me know. This feels like the proper thing to do, and I don't expect
> it will break anyone to fix this.
>
> fs/pnode.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 06a793f4ae38..12fafa711114 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -344,7 +344,6 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
> {
> struct mount *m, *child;
> struct mount *parent = mnt->mnt_parent;
> - int ret = 0;
>
> if (mnt == parent)
> return do_refcount_check(mnt, refcnt);
> @@ -360,11 +359,13 @@ int propagate_mount_busy(struct mount *mnt, int refcnt)
> for (m = propagation_next(parent, parent); m;
> m = propagation_next(m, parent)) {
> child = __lookup_mnt_last(&m->mnt, mnt->mnt_mountpoint);
> - if (child && list_empty(&child->mnt_mounts) &&
> - (ret = do_refcount_check(child, 1)))
> - break;
> + if (!child)
> + continue;
> + if (!list_empty(&child->mnt_mounts) ||
> + do_refcount_check(child, 1))
> + return 1;
> }
> - return ret;
> + return 0;
> }
>
> /*
> --
> 2.10.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
