Menu
▾
▴
Re: [fuse-devel] [RFC v3 2/2] fuse: Add posix acl support
|
From: Seth F. <set...@ca...> - 2016-08-06 01:55:06
|
On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote:
> Seth Forshee <set...@ca...> 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
> >> <set...@ca...> 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. <set...@ca...>
> >> > +
> >> > + 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 can at least explain the response.
What I mean is that we could use the generic posix acl xattr handlers
and the get_acl/set_acl callbacks regardless of whether or not
default_permissions is used. When it's not used we get the id
translation even though the acls aren't being enforced.
The cost to that though is pointless caching. I was also saying that you
could theoretically have aliasing between the cache and the filesystem if
changes were made in the filesystem outside of the kernel's knowledge,
but I'm not convinced this is really a problem to be worried about.
What still doesn't work well in that scenario is passing though the acl
xattrs when CONFIG_FS_POSIX_ACL=n, as the ids would not get translated.
But returning -EOPNOTSUPP (or whatever error) in this case seems
reasonable to me.
> 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).
Without CONFIG_FS_POSIX_ACL=y we're missing definitions for some
symbols, off the top of my head the posix acl xattr handlers but there
may be others. That's one of those "hidden" options that's only enabled
when some other option selects it. Using a Kconfig option for the
filesystem like I did here is the usual way of handling it, and I don't
see a good reason to make fuse different in this respect.
> 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?
Yes, it makes sense. I was hoping we could do it without userspace
modifications but I guess that isn't going to happen.
What I'm not convinced of is that the userspace visible changes in
behavior won't break someone's software, even if they aren't really
getting acl enforcement.
Thanks,
Seth
|
