Re: [RFC v3 2/2] fuse: Add posix acl support

[ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)]

Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:

> On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote:
>> On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
>> <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
>> > Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y
>> > and default_permissions is used for the filesystem. When
>> > default_permissions is not used fuse cannot meaninfully support
>> > cals, as fuse_permission() only sends FUSE_PERMISSION from the
>> > access, faccessat, chdir, fchdir, and chroot system calls.
>> > Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or
>> > default_permissions is not used fuse will return -EOPNOTSUPP
>> > whenever posix acl xattrs are read or written.
>> 
>> Which is breaking backward compatibilty.  Please don't do this without
>> good reason.
>> 
>> Even having "default_permissions" change behavior might cause
>> problems.  I'd suggest adding an INIT flag to indicate whether the
>> filesystem wants acl checking or not.  Feature negotiation with the
>> INIT flag is good for another reason:  the filesystem can detect
>> whether this feature is available and act differently based on that.
>
> This backwards compatibility is a bit of a mystery to me. I've seen
> claims of fuse filesystems supporting acls internally, but in practice I
> can't see how this can possibly work since fuse_permission() only sends
> a FUSE_ACCESS request if !default_permissions and one of MAY_ACCESS or
> MAY_CHDIR is set.
>
> Of course it is also a change to not pass through the xattr even if it
> isn't being enforced. I'm going to defer to Eric on this point, as he
> has some broader-reaching ideas about this.

Fuse is weird in dealing with the special xattrs.

A typical filesystem will only support the special xattrs (aka
security.* system.*) when the underlying filesystem supports them.  For
fuse that would be an INIT flag.

The reason this comes up is all of this starts with allowing
unprivileged mounts of fuse filesystems.  A fully unprivileged mount
implies your own private user namespace and your own private mount
namespace.

As posix acls have embedded uids and gids their meaning depends on
which user namespace they are interpreted in.  When a filesystem is
mounted outside of the initial user namespace this becomes interesting.

The sanest approach I can see internal to the kernel is to require all
filesystems that care about posix acls to implement get_acl and set_acl
methods and not let the filesystems have access to the raw posix acl
data.  That moves the communication from the vfs to the filesystem
into kuids and kgids (like all other uid and gid data).


For most filesystems that opt into receiving the posix acl xattrs only
when posix acls are implemented this is not a problem.  For fuse this
is interesting.

It is my intention to clean all of this up early November when I get
back from vacation.  At which point the plan is to simply have
the vfs return a not supported error if passed or asked for a posix acl
xattr.

My hope is that all of this is far enough along that fuse can do
something compatible in parallel such as return -EOPNOTSUPP or -ENOTSUP
when presented with posix acls xattrs if fuse posix acl support is not
enabled.

Which leads me to this chunk of the code being reviewed:

>> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
>> > new file mode 100644
>> > index 000000000000..c0f412dfe9a7
>> > --- /dev/null
>> > +++ b/fs/fuse/acl.c
>> > @@ -0,0 +1,200 @@
>> > +/*
>> > +  FUSE: Filesystem in Userspace
>> > +  Copyright (C) 2016 Canonical Ltd. <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> > +
>> > +  This program can be distributed under the terms of the GNU GPL.
>> > +  See the file COPYING.
>> > +*/
>> > +
>> > +#include "fuse_i.h"
>> > +
>> > +#include <linux/posix_acl.h>
>> > +#include <linux/posix_acl_xattr.h>
>> > +
>> > +#ifdef CONFIG_FUSE_FS_POSIX_ACL
>> > +
>> > +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
>> > +                             struct dentry *dentry, struct inode *inode,
>> > +                             const char *name, void *value, size_t size)
>> > +{
>> > +       struct fuse_conn *fc = get_fuse_conn(inode);
>> > +
>> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
>> > +               if (handler->flags == ACL_TYPE_ACCESS)
>> > +                       return posix_acl_access_xattr_handler.get(
>> > +                                       &posix_acl_access_xattr_handler,
>> > +                                       dentry, inode, name, value, size);
>> > +               if (handler->flags == ACL_TYPE_DEFAULT)
>> > +                       return posix_acl_default_xattr_handler.get(
>> > +                                       &posix_acl_default_xattr_handler,
>> > +                                       dentry, inode, name, value, size);
>> > +       }
>> > +       return -EOPNOTSUPP;
>> > +}
>> > +
>> > +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
>> > +                               struct dentry *dentry, struct inode *inode,
>> > +                               const char *name, const void *value, size_t size,
>> > +                               int flags)
>> > +{
>> > +       struct fuse_conn *fc = get_fuse_conn(inode);
>> > +
>> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
>> > +               if (handler->flags == ACL_TYPE_ACCESS)
>> > +                       return posix_acl_access_xattr_handler.set(
>> > +                                       &posix_acl_access_xattr_handler,
>> > +                                       dentry, inode, name, value, size,
>> > +                                       flags);
>> > +               if (handler->flags == ACL_TYPE_DEFAULT)
>> > +                       return posix_acl_default_xattr_handler.set(
>> > +                                       &posix_acl_default_xattr_handler,
>> > +                                       dentry, inode, name, value, size,
>> > +                                       flags);
>> > +       }
>> > +       return -EOPNOTSUPP;
>> > +}
>> 
>> The above should go away.  As I said, getxattr() and setxattr()
>> shouldn't behave differently depending on whether
>> "default_permissions" is set or not.
>
> So use the posix xattr handlers in all cases then? I'm okay with that,
> my only reason for not doing it was that when default_permissions is not
> set the acl xattr reads can come from the cache rather than the
> filesystem.

I don't quite understand this feedback, or the response.

I think what is desired here is:

- In fuse_send_init to send a new flag FUSE_POSIX_ACL,
  if the mount option "default_permissions" is set. ( Supporting posix
  acls don't if fuse is not going to support them).

- In process_init_reply test if FUSE_POSIX_ACL was returned
  and set a boolean fc->posix_acl.

- In the code above test fc->posix_acl instead of
  FUSE_DEFAULT_PERMISSIONS.  As having get_acl and set_acl succeeded
  implies that   that the filesystem implements posix acls.
  
- Don't bother implementing a posix acl Kconfig option (after the review
  comments there is so little code in the kernel it shouldn't matter).

Which will result in the posix acl xattr not being transmitted to the
filesystem unless the filesystem has enabled posix acl support.

That seems simple and an implementation that won't need to change when I
clean up the kernel posix acl support.

Seth does that make sense to you?

Eric

------------------------------------------------------------------------------
-- 
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel