On Fri, Jan 13, 2017 at 12:31:59PM -0800, Andrei Vagin wrote:
> Hi Eric,
>
> Something wrong is in this patch. Pls, take a look at this script:
Actually, it works as expected. The reason of this error is what was
fixed in this patch. I'm sorry for this noise.
Tested-by: Andrei Vagin <avagin@xxxxxxxxxxxxx>
>
> [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
