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

Sage Weil
2014-01-31T00:14:28Z

On Thu, 30 Jan 2014, Linus Torvalds wrote:
> On Wed, Jan 29, 2014 at 11:54 PM, Christoph Hellwig <hch@infradead.org> 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