[heads-up] binderfs bugs galore...

[Al Viro <viro@zeniv.linux.org.uk>]

	1) d_make_root() does iput() on failure.  IOW, if you hit that case,
you get double iput() in err_without_dentry.

	2) final dput() does iput() (unsurprisingly).  IOW, anyone hitting
err_with_dentry will get double iput() as well.

	3) iput(NULL) is (fortunately) a no-op.  And that is the only case
when iput() in there does not cause immediate trouble.

	4) failure in binderfs_fill_super() leads to a call of
deactivate_locked_super().  Which calls binderfs_kill_super(),
which does put_ipc_ns(info->ipc_ns).  IOW,
        put_ipc_ns(ipc_ns);
in failure exits of binderfs_fill_super() is also a double-put...

	5) ... if, that is, the memory that used to hold sb->s_fs_info
until the same failure exit has kfreed it has already been stomped onto,
in which case you get put_ipc_ns() done on arbitrary address that might've
never held a struct ipc_namespace instance.

	What I really don't get here is why do you go to such contortions
in the first place.  Every single thing done in err_without_dentry is wrong.
And the only thing done in err_with_dentry before it hits err_without_dentry
is pointless; deactivate_locked_super() will do that dput() just fine.
Whereas the simple return ret; (or return -ENOMEM in most of the cases)
would've worked correctly...

	Actually, looking at that a bit more,
	6) initially you have ->s_fs_info contain a pointer to
struct ipc_namespace.  Then binderfs_fill_super() tries to allocate
a struct binderfs_info instance and stick *that* in place of
->s_fs_info, moving the original value into info->ipc_ns.  That's
Not Nice(tm) towards the poor binderfs_kill_super(), which has no
fscking way to tell which kind of pointer is it going to find
there.  As it is, it assumes that replacement has been done and
drops ->ipc_ns it finds in there.  Guess what happens if you
fail on attempt to allocate struct binderfs_info in the first
place?

	7) binderfs_test_super() also assumes that all superblocks it
is asked to look at will have ->s_fs_info pointing to struct binderfs_info.
binderfs_set_super(), OTOH, sets it to struct ipc_namespace pointer it
(ultimately - sget_userns()) has been passed as 'data'.  So if two mounts
race, you'll get a false negative out of binderfs_test_super().

	8) binderfs_binder_ctl_create() is a no-op on subsequent
calls (if there had been any).  And the first call is done before
we unlock the superblock, so locking the root is completely pointless.

	9) the one thing ->s_fs_info can't be in the whole thing is NULL.
There are two assignments - one in binderfs_set_super(), where we set
it to the last argument of sget_userns() and another in binderfs_fill_super()
where we set it to 'info'.  In both cases we have that pointer dereferenced
a little bit earlier - while evaluating the next-to-last argument of the
same sget_userns() call or in setting info->root_uid in the other place.
Both would've obviously oopsed before storing NULL there.  IOW, checking
whether it's NULL is pointless both in binderfs_test_super() and in
binderfs_kill_super().

	10) your ->unlink() checks if the victim is ->control_dentry.
Your ->rename(), OTOH, does not, which renders the check in ->unlink()
trivial to bypass.

	11) speaking of the tests that are there in ->rename(),
        /* binderfs doesn't support directories. */
        if (d_is_dir(old_dentry))
                return -EPERM;
is... interesting.  If the comment is correct, the check is pointless.
And AFAICS the comment *is* correct...  Another check in there
        if (!simple_empty(new_dentry))
                return -ENOTEMPTY;
is equally pointless, for the same reason...

	12) what is the comment in binderfs_unlink() supposed to mean?
In ->unlink() we *already* have parent locked...