On Fri, 2020-01-31 at 18:15 +0100, Marc Lehmann wrote:
> On Fri, Jan 31, 2020 at 07:16:14PM +0800, Ian Kent <ik...@redhat.com>
> wrote:
> > It looks like this package has the change that adds handling of
> > symlinks to umount_multi() so this should not be calling umount()
> > at all.
>
> Ah, I tried to see if there was a custom symlink patch in debians
> version,
> but it seems to e integrated intot he source tree. I feel a bit
> guilty as I
> think I am the likely reason why debian kept some symlink changes.
>
> > > 5463 execve("/bin/umount", ["/bin/umount", "/fs/bp"],
> > > 0x5555555b23a0 /* 20 vars */) = 0
> >
> > Yeah, it may have done that before the symlink handling change
> > I mentioned. For a long time there wasn't sufficient attention
> > given to symlinks in favour of bind mounting.
>
> That's because the previous autofs maintainer, in true systemd
> fashioon,
> decided that nobody should use symlinks as bind mounts are the
> future(tm).
>From an upstream POV it was more neglect (on my part) of the
symlink code in favour of bind mounts.
But the autofs amd format map support needs them to work properly
so quite a bit of effort went into making them work as best as I
could.
>
> This seesm to be no longer the case, and I greatly appreciate the
> apparent
> change in policy to make symlinks viable - autofs would be completely
> useless to me without symlink support.
Right, apologies for that.
>
> > I definitely don't see the the target get umounted in the Fedora
> > package or the current development source and I've verified the
>
> I downlaoded autofs-5.1.5-10.fc30.src.rpm and indeed, this block is
> only in
> the debian version (didn't check any .patch files in the rpm though):
>
> /* If path is a mount it can't be a symlink */
> if (is_mounted(_PATH_MOUNTED, path, MNTS_ALL))
> goto real_mount;
>
> And this is exactly the code that causes this problem. is_mounted has
> different semantics depending on whether /dev/autofs exists.
Right, if /dev/autofs does not exist then is_mounted() will consult
the text based mount tables. That has terrible performance when the
mount table is large so not using the miscellaneous device isn't the
best idea in general.
>
> > I could stop the path walk following symlinks but I'd rather not
> > since the current code, including that of the Debian package (I'll
> > look a bit closer at the Debian package source), has that early
> > symlink handing I spoke about.
>
> Right. Even if this problem is fixed though, maybe some thought
> should
> be given to the fact that is_mounted is not useful for symlinks in
> the
> current version as the result is essentially random (practically,
> lstat
> vs. stat).
>
> MY hunch is that the kernel shouldn't follow symlinks when testing
> for
> mountpoints, but that probably can't be changed anymore.
I am thinking about it.
The is_mounted() function is used in numerous places and I'm pretty
sure there are cases where it needs to follow symlinks so that's
tricky.
Ian
