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...
