Fix refcounting problem in walk_symlink()

The old code would double-close 'symlink' if it ever got into the "walk
succeeded but we were still on a symlink" case.

Recall that walk_symlink() either walks all the way, or not at all.  If
the old code (deleted) failed, we were supposed to close symlink.
However, there was a case where walk_symlink would fail, but it would
also close the chan passed in: when the sub-walk succeeded, but
walk_symlink() still failed.  How could that fail?  With a symlink loop.

If that sounds confusing, it's because it is.  There are actually two
spots of recursion, which might not even have been clear to me when I
wrote this.  The main recursive path is walk -> walk_symlink -> walk ->
walk_symlink.  We normally never got to the "walk succeeded, then call
walk_symlink" (deleted).  We usually only called walk_symlink() from
*within* walk itself.

You could trigger this situation with a no_follow symlink
(rename/remove, flags Aremove, flags no_follow).  Syzkaller basically
did this:
	ln -s x x
	rename x/x whatever
So we were walking the first x, had no_follow set, so walk ->
walk_symlink for the first x.  Since there were more names in the
main/outer namec, we'd get past the first no_follow.  Then that second
walk() would be for "../x", where no_follow would kick in, since there
were no longer any names left.  That walk_symlink() would return the
syml passed in, which appeared to walk to be a success (which it was).

Anyway, this was calling the buggy post-successful-walk() part of
walk_symlink().  The first fix I had was to only cclose(symlink) when that
interior walk_symlink() succeeded.  Although that fixed the bug
(walk_symlink either succeeds xor closes, so don't close on success),
the real problem was having that code at all.

If walk() lands on a symlink, walk itself should try to deal with it (by
calling walk_symlink()).  If walk returns (success or failure), we
should consider the walking of that symlink to be done, one way or
another.  If it is no_follow, we might be on a symlink (if last in the
path).  If it was a mount_point, we could be on a symlink too.

That whole "we're still on a symlink, let's walk it again" was busted,
and it would break when we had no_follow set globally and tried to
follow a legit intermediate symlink that had more than one link to
follow (but not 8 - the max).  And it was confusing.  Hopefully this
code makes a little more sense - esp when realizing that walk() sorts
out symlinks by calling walk_symlink().  walk_symlink() shouldn't call
itself, but it can call walk().  Up to 8 times.

Reported-by: syzbot+9eec51df84779065d6de@syzkaller.appspotmail.com
Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>

kern/src/ns/chan.c

diff --git a/kern/src/ns/chan.c b/kern/src/ns/chan.c
index 3929813..02f11fd 100644
--- a/kern/src/ns/chan.c
+++ b/kern/src/ns/chan.c
@@ -1684,6 +1684,7 @@
 	struct dir *dir;
 	char *link_name, *link_store;
 	struct chan *from;
+	bool old_nofollow;
 	Elemlist e = {0};
 
 	/* mildly expensive: need to rlock the namespace */
@@ -1727,18 +1728,27 @@
 	kfree(link_store);
 
 	wh->nr_loops++;
+	/* no_follow applies to the outermost walk, i.e. the one that the
+	 * original namec performs.  At this point, we've decided that we're
+	 * going to try and follow a symlink: even if its no_follow, that only
+	 * applies to the last link in the original path.  Our sub-walks are not
+	 * no_follow.
+	 *
+	 * Note the other wh vars need to stay with the walk: nr_loops,
+	 * since its our method of detecting symlink loops, and can_mount, which
+	 * is a property of the overall namec() call. */
+	old_nofollow = wh->no_follow;
+	wh->no_follow = false;
 	if (walk(&from, e.elems, e.ARRAY_SIZEs, wh, NULL) < 0) {
 		cclose(from);
 		from = NULL;
 	} else {
+		/* We can still have a successful walk and have the new 'from'
+		 * be a symlink.  We'd need walk_symlink to return a symlink
+		 * chan, which happens if the symlink is a mount point. */
 		cclose(symlink);
-		if (from->qid.type & QTSYMLINK) {
-			symlink = from;
-			from = walk_symlink(symlink, wh, nr_names_left);
-			if (!from)
-				cclose(symlink);
-		}
 	}
+	wh->no_follow = old_nofollow;
 	wh->nr_loops--;
 
 	kfree(e.name);