[v6,4/5] fuse: Ensure posix acls are translated outside of init_user_ns

Submitted by Eric W. Biederman on Feb. 21, 2018, 8:29 p.m.

Details

Message ID 20180221202908.17258-4-ebiederm@xmission.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Eric W. Biederman Feb. 21, 2018, 8:29 p.m.
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(+)

Patch hide | download patch | download mbox

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
+};

Comments

Miklos Szeredi Feb. 22, 2018, 11:40 a.m.
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
>
Eric W. Biederman Feb. 22, 2018, 7:18 p.m.
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
Eric W. Biederman Feb. 22, 2018, 10:50 p.m.
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
Miklos Szeredi Feb. 26, 2018, 7:47 a.m.
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
Eric W. Biederman Feb. 26, 2018, 4:35 p.m.
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
Eric W. Biederman Feb. 26, 2018, 9:51 p.m.
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