On Thu, Jan 02, 2020 at 02:59:20PM +1100, Aleksa Sarai wrote: > On 2020-01-01, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > On Thu, Jan 02, 2020 at 01:44:07AM +1100, Aleksa Sarai wrote: > > > > > Thanks, this fixes the issue for me (and also fixes another reproducer I > > > found -- mounting a symlink on top of itself then trying to umount it). > > > > > > Reported-by: Aleksa Sarai <cyphar@xxxxxxxxxx> > > > Tested-by: Aleksa Sarai <cyphar@xxxxxxxxxx> > > > > Pushed into #fixes. > > Thanks. One other thing I noticed is that umount applies to the > underlying symlink rather than the mountpoint on top. So, for example > (using the same scripts I posted in the thread): > > # ln -s /tmp/foo link > # ./mount_to_symlink /etc/passwd link > # umount -l link # will attempt to unmount "/tmp/foo" > > Is that intentional? It's a mess, again in mountpoint_last(). FWIW, at some point I proposed to have nd_jump_link() to fail with -ELOOP if the target was a symlink; Linus asked for reasons deeper than my dislike of the semantics, I looked around and hadn't spotted anything. And there hadn't been at the time, but when four months later umount_lookup_last() went in I failed to look for that source of potential problems in it ;-/ I've looked at that area again now. Aside of usual cursing at do_last() horrors (yes, its control flow is a horror; yes, it needs serious massage; no, it's not a good idea to get sidetracked into that right now), there are several fun questions: * d_manage() and d_automount(). We almost certainly don't want those for autofs on the final component of pathname in umount, including the trailing symlinks. But do we want those on usual access via /proc/*/fd/*? I.e. suppose somebody does open() (O_PATH or not) in autofs; do we want ->d_manage()/->d_automount() called when resolving /proc/self/fd/<whatever>/foo/bar? We do not; is that correct from autofs point of view? I suspect that refusing to do ->d_automount() is correct, but I don't understand ->d_manage() purpose well enough to tell. * I really hope that the weird "trailing / forces automount even in cases when we normally wouldn't trigger it" (stat /mnt/foo vs. stat /mnt/foo/) is not meant to extend to umount. I'd like Ian's confirmation, though. * do we want ->d_manage() on following .. into overmounted directory? Again, autofs question... The minimal fix to mountpoint_last() would be to have follow_mount() done in LAST_NORM case. However, I'd like to understand (and hopefully regularize) the rules for follow_mount()/follow_managed(). Additional scary question is nfsd iterplay with automount. For nfs4 exports it's potentially interesting... Ian, could you comment on the autofs questions above? I'd rather avoid doing changes in that area without your input - it's subtle and breakage in automount-related behaviour can be mysterious as hell.
