tree 3e613d45f7a706cf5bfcf4ee0a254c8b83102614
parent 26e4d0b9a3ef64c609f7ac331984aaea9f9b799f
author Barret Rhoden <brho@cs.berkeley.edu> 1554729556 -0400
committer Barret Rhoden <brho@cs.berkeley.edu> 1554731316 -0400

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>