Re: [PATCH] vfs: move ACL cache lookup into generic code

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

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()?