On Thu, Jan 17, 2019 at 10:40:11PM +0000, Al Viro wrote: <snip> Thanks for all the comments they seem really helpful. I'm currently travelling but I will try to address them all within the next week! Does that sound ok to you? Thanks! Christian > 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...
