On Mon, Dec 30, 2019 at 06:29:59PM +1100, Aleksa Sarai wrote:

On 2019-12-30, Aleksa Sarai [off-list ref] wrote:

quoted

On 2019-12-30, Al Viro [off-list ref] wrote:

quoted

On Mon, Dec 30, 2019 at 04:20:35PM +1100, Aleksa Sarai wrote:

quoted

A reasonably detailed explanation of the issues is provided in the patch
itself, but the full traces produced by both the oopses and deadlocks is
included below (it makes little sense to include them in the commit since we
are disabling this feature, not directly fixing the bugs themselves).

I've posted this as an RFC on whether this feature should be allowed at
all (and if anyone knows of legitimate uses for it), or if we should
work on fixing these other kernel bugs that it exposes.

Umm...  Are all of those traces
	a) reproducible on mainline and

This was on viro/for-next, I'll retry it on v5.5-rc4.

The NULL deref oops is reproducible on v5.5-rc4. Strangely it seems
harder to reproduce than on viro/for-next (I kept reproducing it there
by accident), but I'll double-check if that really is the case.

The simplest reproducer is (using the attached programs and .config):

  ln -s . link
  sudo ./umount_symlink link

FWIW, the problem with that reproducer is that we *CAN'T* resolve that
path.  Look: you have /proc/self/fd/3 resolve to ./link.  OK, you've
asked to follow that.  Got ./link, which is a symlink, so we need to
follow it further.  Relative to what, though?

The meaning of symlink is dependent upon the directory you find it in.
And we don't have any here.

The bug is in mountpoint_last() - we have
        if (unlikely(nd->last_type != LAST_NORM)) {
                error = handle_dots(nd, nd->last_type);
                if (error)
                        return error;
                path.dentry = dget(nd->path.dentry);
        } else {
                path.dentry = d_lookup(dir, &nd->last);
                if (!path.dentry) {
                        /*
                         * No cached dentry. Mounted dentries are pinned in the
                         * cache, so that means that this dentry is probably
                         * a symlink or the path doesn't actually point
                         * to a mounted dentry.
                         */
                        path.dentry = lookup_slow(&nd->last, dir,
                                             nd->flags | LOOKUP_NO_REVAL);
                        if (IS_ERR(path.dentry))
                                return PTR_ERR(path.dentry);
                }
        }
        if (d_flags_negative(smp_load_acquire(&path.dentry->d_flags))) {
                dput(path.dentry);
                return -ENOENT;
        }
        path.mnt = nd->path.mnt;
        return step_into(nd, &path, 0, d_backing_inode(path.dentry), 0);
in there, and that ends up with step_into() called in case of LAST_DOT/LAST_DOTDOT
(where it's harmless) *AND* in case of LAST_BIND.  Where it very much isn't.

I'm not sure if you have caught anything else, but we really, really should *NOT*
consider the LAST_BIND as "maybe we should follow the result" material.  So
at least the following is needed; could you check if anything else remains
with that applied?

diff --git a/fs/namei.c b/fs/namei.c
index d6c91d1e88cb..d4fbbda8a7ff 100644
--- a/fs/namei.c
+++ b/fs/namei.c

@@ -2656,10 +2656,7 @@ mountpoint_last(struct nameidata *nd)
 	nd->flags &= ~LOOKUP_PARENT;
 
 	if (unlikely(nd->last_type != LAST_NORM)) {
-		error = handle_dots(nd, nd->last_type);
-		if (error)
-			return error;
-		path.dentry = dget(nd->path.dentry);
+		return handle_dots(nd, nd->last_type);
 	} else {
 		path.dentry = d_lookup(dir, &nd->last);
 		if (!path.dentry) {