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

Al Viro
2014-02-03T22:40:42Z

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?