| Message ID | 20180221202908.17258-4-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 21, 2018 at 9:29 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Ensure the translation happens by failing to read or write > posix acls when the filesystem has not indicated it supports > posix acls. For the first iteration this is fine, but we could convert the raw xattrs as well, if we later want to, right? Thanks, Miklos > > This ensures that modern cached posix acl support is available > and used when dealing with posix acls. This is important > because only that path has the code to convernt the uids and > gids in posix acls into the user namespace of a fuse filesystem. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > fs/fuse/fuse_i.h | 1 + > fs/fuse/inode.c | 7 +++++++ > fs/fuse/xattr.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 51 insertions(+) > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index 7772e2b4057e..986fa2b043ab 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -979,6 +979,7 @@ ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size); > int fuse_removexattr(struct inode *inode, const char *name); > extern const struct xattr_handler *fuse_xattr_handlers[]; > extern const struct xattr_handler *fuse_acl_xattr_handlers[]; > +extern const struct xattr_handler *fuse_no_acl_xattr_handlers[]; > > struct posix_acl; > struct posix_acl *fuse_get_acl(struct inode *inode, int type); > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index e018dc3999f4..a52cf2019a58 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -1097,6 +1097,13 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent) > file->f_cred->user_ns != sb->s_user_ns) > goto err_fput; > > + /* > + * If we are not in the initial user namespace posix > + * acls must be translated. > + */ > + if (sb->s_user_ns != &init_user_ns) > + sb->s_xattr = fuse_no_acl_xattr_handlers; > + > fc = kmalloc(sizeof(*fc), GFP_KERNEL); > err = -ENOMEM; > if (!fc) > diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c > index 3caac46b08b0..433717640f78 100644 > --- a/fs/fuse/xattr.c > +++ b/fs/fuse/xattr.c > @@ -192,6 +192,26 @@ static int fuse_xattr_set(const struct xattr_handler *handler, > return fuse_setxattr(inode, name, value, size, flags); > } > > +static bool no_xattr_list(struct dentry *dentry) > +{ > + return false; > +} > + > +static int no_xattr_get(const struct xattr_handler *handler, > + struct dentry *dentry, struct inode *inode, > + const char *name, void *value, size_t size) > +{ > + return -EOPNOTSUPP; > +} > + > +static int no_xattr_set(const struct xattr_handler *handler, > + struct dentry *dentry, struct inode *nodee, > + const char *name, const void *value, > + size_t size, int flags) > +{ > + return -EOPNOTSUPP; > +} > + > static const struct xattr_handler fuse_xattr_handler = { > .prefix = "", > .get = fuse_xattr_get, > @@ -209,3 +229,26 @@ const struct xattr_handler *fuse_acl_xattr_handlers[] = { > &fuse_xattr_handler, > NULL > }; > + > +static const struct xattr_handler fuse_no_acl_access_xattr_handler = { > + .name = XATTR_NAME_POSIX_ACL_ACCESS, > + .flags = ACL_TYPE_ACCESS, > + .list = no_xattr_list, > + .get = no_xattr_get, > + .set = no_xattr_set, > +}; > + > +static const struct xattr_handler fuse_no_acl_default_xattr_handler = { > + .name = XATTR_NAME_POSIX_ACL_DEFAULT, > + .flags = ACL_TYPE_ACCESS, > + .list = no_xattr_list, > + .get = no_xattr_get, > + .set = no_xattr_set, > +}; > + > +const struct xattr_handler *fuse_no_acl_xattr_handlers[] = { > + &fuse_no_acl_access_xattr_handler, > + &fuse_no_acl_default_xattr_handler, > + &fuse_xattr_handler, > + NULL > +}; > -- > 2.14.1 >
Miklos Szeredi <mszeredi@redhat.com> writes: > On Wed, Feb 21, 2018 at 9:29 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> Ensure the translation happens by failing to read or write >> posix acls when the filesystem has not indicated it supports >> posix acls. > > For the first iteration this is fine, but we could convert the raw > xattrs as well, if we later want to, right? I will say maybe. This is tricky. The code would not be too hard, and the function to do the work posix_acl_fix_xattr_userns already exists in fs/posix_acl.c I don't actually expect that to work longterm. I expect the direction the kernel internals are moving is that all filesystems that implement posix acls will be expected to implement .get_acl and .set_acl. I would have to reread the old thread that got us to this point with posix acls before I could really understand the backwards compatible fuse use case, and I would have to reread the rest of the acl processing in the kernel before I could recall exactly what makes sense. If there was an obvious way to whitelist xattrs that fuse can support for user namespaces I think I would go for that. Just to avoid future problems with future xattrs. Eric
ebiederm@xmission.com (Eric W. Biederman) writes: > Miklos Szeredi <mszeredi@redhat.com> writes: > >> On Wed, Feb 21, 2018 at 9:29 PM, Eric W. Biederman >> <ebiederm@xmission.com> wrote: >>> Ensure the translation happens by failing to read or write >>> posix acls when the filesystem has not indicated it supports >>> posix acls. >> >> For the first iteration this is fine, but we could convert the raw >> xattrs as well, if we later want to, right? > > I will say maybe. This is tricky. The code would not be too hard, > and the function to do the work posix_acl_fix_xattr_userns already > exists in fs/posix_acl.c > > I don't actually expect that to work longterm. I expect the direction > the kernel internals are moving is that all filesystems that implement > posix acls will be expected to implement .get_acl and .set_acl. > > I would have to reread the old thread that got us to this point with > posix acls before I could really understand the backwards compatible > fuse use case, and I would have to reread the rest of the acl processing > in the kernel before I could recall exactly what makes sense. > > If there was an obvious way to whitelist xattrs that fuse can support > for user namespaces I think I would go for that. Just to avoid future > problems with future xattrs. I am remembering why this is such a sticky issue. Today when a posix acl is read from user space the code does: posix_acl_to_xattr(&init_user_ns, ...) in posix_acl_xattr_get posix_acl_fix_xattr_to_user() in getxattr Similary when a posix acl is written from user space the code does: posix_acl_fix_xattr_from_user() in setxattr posix_acl_from_xattr(&init_user_us, ...) in posix_acl_xattr_set If every posix acl supporting filesystem in the kernel would use posix_acl_access_xattr_handler and posix_acl_default_xattr_handler the function posix_acl_fix_xattr_to_user and posix_acl_fix_xattr_from_user and posix_acl_fix_xattr_userns could all be removed and the posix acl handling could be that little bit simpler and faster. So if we could figure out how to use the generic acl support for the old brand of fuse filesystems that don't set FUSE_POSIX_ACL it would be much easier to support them long term. Eric
On Thu, Feb 22, 2018 at 11:50 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > So if we could figure out how to use the generic acl support for the old > brand of fuse filesystems that don't set FUSE_POSIX_ACL it would be much > easier to support them long term. Simplest and most robust way seems to be to do everything the same (as with FUSE_POSIX_ACL) but tell the vfs not to cache the acl. Thanks, Miklos
Miklos Szeredi <mszeredi@redhat.com> writes: > On Thu, Feb 22, 2018 at 11:50 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: > >> So if we could figure out how to use the generic acl support for the old >> brand of fuse filesystems that don't set FUSE_POSIX_ACL it would be much >> easier to support them long term. > > Simplest and most robust way seems to be to do everything the same (as > with FUSE_POSIX_ACL) but tell the vfs not to cache the acl. Good point. That sounds like for the !fc->posix_acl case we just need a careful use of "forget_all_cached_acls(inode)". I will take a quick look at that, and see if that is easy/sufficient to cover the legacy fuse case. Otherwise I will go with what I already have here. That feels like a better path. And internally I would call what is today fc->posix_acl fc->cached_posix_acl. To better convey the intent. Fingers crossed. Eric
ebiederm@xmission.com (Eric W. Biederman) writes: > Miklos Szeredi <mszeredi@redhat.com> writes: > >> On Thu, Feb 22, 2018 at 11:50 PM, Eric W. Biederman >> <ebiederm@xmission.com> wrote: >> >>> So if we could figure out how to use the generic acl support for the old >>> brand of fuse filesystems that don't set FUSE_POSIX_ACL it would be much >>> easier to support them long term. >> >> Simplest and most robust way seems to be to do everything the same (as >> with FUSE_POSIX_ACL) but tell the vfs not to cache the acl. > > Good point. That sounds like for the !fc->posix_acl case we just > need a careful use of "forget_all_cached_acls(inode)". > > I will take a quick look at that, and see if that is easy/sufficient to > cover the legacy fuse case. Otherwise I will go with what I already > have here. > > That feels like a better path. And internally I would call what is > today fc->posix_acl fc->cached_posix_acl. To better convey the intent. > Fingers crossed. It looks like simply setting "inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE;" is the secret sauce needed to disable caching in the legacy case and make everything work. I had to tweak the calls to forget_all_cached_acls so that won't clear the ACL_DONT_CACHE status but otherwise that was an absolutely trivial change to combine those two code paths. I will post my updated patches shortly. Eric
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 7772e2b4057e..986fa2b043ab 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -979,6 +979,7 @@ ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size); int fuse_removexattr(struct inode *inode, const char *name); extern const struct xattr_handler *fuse_xattr_handlers[]; extern const struct xattr_handler *fuse_acl_xattr_handlers[]; +extern const struct xattr_handler *fuse_no_acl_xattr_handlers[]; struct posix_acl; struct posix_acl *fuse_get_acl(struct inode *inode, int type); diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index e018dc3999f4..a52cf2019a58 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -1097,6 +1097,13 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent) file->f_cred->user_ns != sb->s_user_ns) goto err_fput; + /* + * If we are not in the initial user namespace posix + * acls must be translated. + */ + if (sb->s_user_ns != &init_user_ns) + sb->s_xattr = fuse_no_acl_xattr_handlers; + fc = kmalloc(sizeof(*fc), GFP_KERNEL); err = -ENOMEM; if (!fc) diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c index 3caac46b08b0..433717640f78 100644 --- a/fs/fuse/xattr.c +++ b/fs/fuse/xattr.c @@ -192,6 +192,26 @@ static int fuse_xattr_set(const struct xattr_handler *handler, return fuse_setxattr(inode, name, value, size, flags); } +static bool no_xattr_list(struct dentry *dentry) +{ + return false; +} + +static int no_xattr_get(const struct xattr_handler *handler, + struct dentry *dentry, struct inode *inode, + const char *name, void *value, size_t size) +{ + return -EOPNOTSUPP; +} + +static int no_xattr_set(const struct xattr_handler *handler, + struct dentry *dentry, struct inode *nodee, + const char *name, const void *value, + size_t size, int flags) +{ + return -EOPNOTSUPP; +} + static const struct xattr_handler fuse_xattr_handler = { .prefix = "", .get = fuse_xattr_get, @@ -209,3 +229,26 @@ const struct xattr_handler *fuse_acl_xattr_handlers[] = { &fuse_xattr_handler, NULL }; + +static const struct xattr_handler fuse_no_acl_access_xattr_handler = { + .name = XATTR_NAME_POSIX_ACL_ACCESS, + .flags = ACL_TYPE_ACCESS, + .list = no_xattr_list, + .get = no_xattr_get, + .set = no_xattr_set, +}; + +static const struct xattr_handler fuse_no_acl_default_xattr_handler = { + .name = XATTR_NAME_POSIX_ACL_DEFAULT, + .flags = ACL_TYPE_ACCESS, + .list = no_xattr_list, + .get = no_xattr_get, + .set = no_xattr_set, +}; + +const struct xattr_handler *fuse_no_acl_xattr_handlers[] = { + &fuse_no_acl_access_xattr_handler, + &fuse_no_acl_default_xattr_handler, + &fuse_xattr_handler, + NULL +};
Ensure the translation happens by failing to read or write posix acls when the filesystem has not indicated it supports posix acls. This ensures that modern cached posix acl support is available and used when dealing with posix acls. This is important because only that path has the code to convernt the uids and gids in posix acls into the user namespace of a fuse filesystem. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/fuse/fuse_i.h | 1 + fs/fuse/inode.c | 7 +++++++ fs/fuse/xattr.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+)