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

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

On Sat, 23 Jul 2011 07:06:26 +0100, Al Viro <viro@ZenIV.linux.org.uk> 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