Looking at some of the comments it seems you have been looking at the
code that is currently in git master.
Some of the bugs you're pointing at are already fixed and patches for it
are sitting in Greg's tree. :)
The most crucial source of bugs binderfs_mount() is completely gone in
Greg's tree already because I gutted it to just do:
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
int flags, const char *dev_name,
void *data)
{
return mount_nodev(fs_type, flags, data, binderfs_fill_super);
}
since we want to have each binderfs mount to be a new instance.
That means binderfs_set_super() and binderfs_test_super are gone.
So there's no weird mucking with sget_userns() and s_fs_info anymore
going on in binderfs_mount().
That means your points from 5) to 9) should be taken care of.
I have already a patch that remove unnecessary checks and an invalid
comment that you pointed out int 10) to 12).
I'm currently fixing the double put issues 1) to 5) based on your
suggestions from your private mail. Thanks for that!
Thanks, this was overall really helpful.
Christian
On Thu, Jan 17, 2019 at 10:40:11PM +0000, Al Viro wrote:
> 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.
Thanks for catching that. I will prevent the rename for control_dentry
too.
>
> 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...
Thanks. The checks are removed now.
>
> 12) what is the comment in binderfs_unlink() supposed to mean?
> In ->unlink() we *already* have parent locked...
Comment is removed and leftover from when this did something more
"interesting". I'll remove it.
