Re: [PATCH v2] ceph: fix posix ACL hooks

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

On Mon, Feb 03, 2014 at 02:12:00PM -0800, Linus Torvalds wrote:

> >> -int afs_permission(struct inode *inode, int mask)
> >> +int afs_permission(struct dentry *dentry, struct inode *inode, int mask)
> >
> > Oh, _lovely_.  So not only do we pass dentry, the arguments are redundant
> > as well.
> 
> Note that *not* passing in inode would make the patch much bigger,
> because now every filesystem would have to add the
> 
>        struct inode *inode = dentry->d_inode;
> 
> at the top.
> 
> Also, I'm not actually convinced it is redundant at all. Remember the
> RCU lookup case? dentry->d_inode is not safe.

Umm...  Point, but that actually means that we get an extra pitfall for
filesystem writers here.  foo_permission() passes dentry (now that it
has one) to foo_wank_a_lot(), with the latter using dentry->d_inode at
some point...

> >> +static int gfs2_vfs_permission(struct dentry *dentry, struct inode *inode, int mask)
> >> +{
> >> +     return gfs2_permission(inode, mask);
> >> +}
> >
> > Er...  You do realize that callers of gfs2_permission() tend to have
> > the dentry in question, either directly or as ->d_parent of something
> > they have?
> 
> Not true. Look closer.
> 
> Look at gfs2_lookupi() in particular, and check how it is called.

Yeowch...  gfs2_ok_to_move() is particulary nasty...  WTF do we need
it for and why is it not racy as hell?