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

[Sage Weil]

On Thu, 30 Jan 2014, Linus Torvalds wrote:
> On Wed, Jan 29, 2014 at 11:54 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> >
> > For ->set_acl that's fairly easily doable and I actually had a version
> > doing that to be able to convert 9p.  But for ->get_acl the path walking
> > caller didn't seem easily feasible.  ->get_acl actually is an invention
> > of yours, so if you got a good idea to get the dentry to it I'd love
> > to be able to pass it.
> 
> Yeah, that's pretty annoying, largely because that path is also
> RCU-walk aware, which does *not* need this all (because it will never
> call down into the filesystem - if the acl isn't found in the cached
> acl's, we just abort).
> 
> And we're going through that very common "generic_permission()" thing
> that in turn is also often called from the low-level filesystens, and
> it's all fairly tightly integrated with __inode_permission() etc.
> 
> In the end, all the original call-sites should have a dentry, and none
> of this is "fundamental". But you're right, it looks like an absolute
> nightmare to add the dentry pointer through the whole chain. Damn.
> 
> So I'm not thrilled about it, but maybe that "d_find_alias(inode)" to
> find the dentry is good enough in practice. It feels very much
> incorrect (it could find a dentry with a path that you cannot actually
> access on the server, and result in user-visible errors), but I
> definitely see your argument. It may just not be worth the pain for
> this odd ceph case.
> 
> That said, if the ceph people decide to try to bite the bullet and do
> the required conversions to pass the dentry to the permissions
> functions, I think I'd take it unless it ends up being *too* horribly
> messy.

FWIW the dentry isn't useful in the get case; it's only on put that it is 
currently used.  And now that I look closely, it is only being used by 
ceph_setattr to associate the update with the parent directory for the 
purposes of fsync(dirfd)... which is, I think, incorrect anyway (that 
should only flush out/wait for namespace modifications, not inode attr 
updates).

So I think it's fine as is, and we'll clean this up later.

I do have a couple patches on top of what's in your tree, though, that 
clean up a couple duplicated lines in your fix and apply Christoph's 
cleanup:

 git://git.kernel.org/pub/scm/linux/kernel/git/sage/ceph-client.git for-linus

Thanks!
sage
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/