On Sat, 23 Jul 2011 07:06:26 +0100, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Jul 23, 2011 at 05:31:20AM +0100, Al Viro wrote:
>
> > Heh... In addition to ocfs2 leak: 9p leaks nicely if v9fs_acl_mode() is
> > called with !S_ISDIR(mode). In that case acl reference is simply lost.
> > So yes, it's worth looking at.
>
> Damn... FWIW, that code is seriously broken and not only on failure exits.
> Guys, think what happens if we have a negative dentry and call e.g. mkdir().
> Dentry is hashed. Eventually we get to
> d_instantiate(dentry, inode);
> err = v9fs_fid_add(dentry, fid);
> in v9fs_create() (or v9fs_vfs_mkdir_dotl()). At the same time somebody does
> lookup *in* that directory. We do find that thing in dcache, it's (already)
> positive and we hit
> dfid = v9fs_fid_lookup(dentry->d_parent);
> at the same time. That calls v9fs_fid_lookup_with_uid() and then
> v9fs_fid_add(). Now, we have two tasks doing v9fs_fid_add() on the same
> dentry. Which is to say,
> dent = dentry->d_fsdata;
> if (!dent) {
> dent = kmalloc(sizeof(struct v9fs_dentry), GFP_KERNEL);
> if (!dent)
> return -ENOMEM;
>
> spin_lock_init(&dent->lock);
> INIT_LIST_HEAD(&dent->fidlist);
> dentry->d_fsdata = dent;
> }
> and that's an obvious leak. It's not easy to hit, but... Note that
> ->d_revalidate() does *not* help - v9fs_create() will force revaliation
> of parent, but the second process might be already past that point.
> Moreover, ->d_revalidate() would just talk to server and return 1, so
> even if the second process does hit it, nothing will change except for
> minor delay. And the first one might be sleeping in blocking allocation
> at that point...
>
> I don't like that; we certainly can protect v9fs_fid_add() in standard way
> (i.e. recheck ->d_fsdata after kmalloc(), if we have lost the race - kfree()
> and go on, otherwise set ->d_fsdata and make that check+assignment atomic,
> e.g. by use of ->d_lock). However, that looks like a kludge. Is there any
> saner way to do it? E.g. do we absolutely have to do that *after*
> d_instantiate()?
We should be able to move that v9fs_fid_add before d_instantiate. That
way we can be sure that positive dentries have fids attached.
v9fs_vfs_lookup does that already. I guess we should audit all the
v9fs_fid_add usage to make sure we do that consistently all other place.
-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
