| Message ID | CA+55aFwTuBDHgQoTymr8OtE-+k=+pX0T_eb+fff0K+Pa3C-g-Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 29, 2014 at 11:09:18AM -0800, Linus Torvalds wrote: > So attached is the incremental diff of the patch by Sage and Ilya, and > I'll apply it (delayed a bit to see if I can get the sign-off from > Ilya), but I also think we should fix the (non-cached) ACL functions > that call down to the filesystem layer to also get the dentry. 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. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Linus -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 30, 2014 at 02:01:38PM -0800, Linus Torvalds wrote: > 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. It's not just ceph. 9p fundamentally needs it and I really want to convert 9p to the new code so that we can get rid of the lower level interfaces entirely and eventually move ACL dispatching entirely into the VFS. The same d_find_alias hack should work for 9p as well, although spreading this even more gets uglier and uglier. Similarly for CIFS which pretends to understand the Posix ACL xattrs, but doesn't use any of the infrastructure as it seems to rely on server side enforcement. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 03, 2014 at 02:29:43AM -0800, Christoph Hellwig wrote: > On Thu, Jan 30, 2014 at 02:01:38PM -0800, Linus Torvalds wrote: > > 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. > > It's not just ceph. 9p fundamentally needs it and I really want to > convert 9p to the new code so that we can get rid of the lower level > interfaces entirely and eventually move ACL dispatching entirely > into the VFS. The same d_find_alias hack should work for 9p as well, > although spreading this even more gets uglier and uglier. Similarly > for CIFS which pretends to understand the Posix ACL xattrs, but doesn't > use any of the infrastructure as it seems to rely on server side > enforcement. 9P is going to be fun to deal with; that's why I've ended up abandoning vfs.git#experimental-xattr last year. We probably want to move FIDs from dentries to inodes there, and rely in ->getxattr() et.al. upon having already done ->d_revalidate() on some dentry for that inode. Another pile of fun is fsnotify_xattr() call in __vfs_setxattr_noperm() and the whole misbegotten IMA/EVM mess ;-/ See #experimental-xattr - a lot of stuff in that direction is sitting there; might turn out to be useful. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c index f6911284c9bd..66d377a12f7c 100644 --- a/fs/ceph/acl.c +++ b/fs/ceph/acl.c @@ -107,14 +107,14 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) return acl; } -static int ceph_set_acl(struct dentry *dentry, struct inode *inode, - struct posix_acl *acl, int type) +int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) { int ret = 0, size = 0; const char *name = NULL; char *value = NULL; struct iattr newattrs; umode_t new_mode = inode->i_mode, old_mode = inode->i_mode; + struct dentry *dentry = d_find_alias(inode); if (acl) { ret = posix_acl_valid(acl); @@ -208,8 +208,7 @@ int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir) if (IS_POSIXACL(dir) && acl) { if (S_ISDIR(inode->i_mode)) { - ret = ceph_set_acl(dentry, inode, acl, - ACL_TYPE_DEFAULT); + ret = ceph_set_acl(inode, acl, ACL_TYPE_DEFAULT); if (ret) goto out_release; } @@ -217,7 +216,7 @@ int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir) if (ret < 0) goto out; else if (ret > 0) - ret = ceph_set_acl(dentry, inode, acl, ACL_TYPE_ACCESS); + ret = ceph_set_acl(inode, acl, ACL_TYPE_ACCESS); else cache_no_acl(inode); } else { diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 619616d585b0..6da4df84ba30 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1303,6 +1303,7 @@ const struct inode_operations ceph_dir_iops = { .listxattr = ceph_listxattr, .removexattr = ceph_removexattr, .get_acl = ceph_get_acl, + .set_acl = ceph_set_acl, .mknod = ceph_mknod, .symlink = ceph_symlink, .mkdir = ceph_mkdir, diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 8b8b506636cc..32d519d8a2e2 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -97,6 +97,7 @@ const struct inode_operations ceph_file_iops = { .listxattr = ceph_listxattr, .removexattr = ceph_removexattr, .get_acl = ceph_get_acl, + .set_acl = ceph_set_acl, }; @@ -1616,6 +1617,7 @@ static const struct inode_operations ceph_symlink_iops = { .listxattr = ceph_listxattr, .removexattr = ceph_removexattr, .get_acl = ceph_get_acl, + .set_acl = ceph_set_acl, }; /* diff --git a/fs/ceph/super.h b/fs/ceph/super.h index 345933948b6e..aa260590f615 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -718,6 +718,7 @@ extern void ceph_queue_writeback(struct inode *inode); extern int ceph_do_getattr(struct inode *inode, int mask); extern int ceph_permission(struct inode *inode, int mask); extern int ceph_setattr(struct dentry *dentry, struct iattr *attr); +extern int ceph_setattr(struct dentry *dentry, struct iattr *attr); extern int ceph_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat); @@ -741,12 +742,14 @@ extern const struct xattr_handler *ceph_xattr_handlers[]; #ifdef CONFIG_CEPH_FS_POSIX_ACL struct posix_acl *ceph_get_acl(struct inode *, int); +int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type); int ceph_init_acl(struct dentry *, struct inode *, struct inode *); void ceph_forget_all_cached_acls(struct inode *inode); #else #define ceph_get_acl NULL +#define ceph_set_acl NULL static inline int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir)