On Tue, Jan 24, 2023 at 03:07:18PM +0100, Miklos Szeredi wrote:
> On Fri, 20 Jan 2023 at 12:55, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > This cycle we ported all filesystems to the new posix acl api. While
> > looking at further simplifications in this area to remove the last
> > remnants of the generic dummy posix acl handlers we realized that we
> > regressed fuse daemons that don't set FUSE_POSIX_ACL but still make use
> > of posix acls.
> >
> > With the change to a dedicated posix acl api interacting with posix acls
> > doesn't go through the old xattr codepaths anymore and instead only
> > relies the get acl and set acl inode operations.
> >
> > Before this change fuse daemons that don't set FUSE_POSIX_ACL were able
> > to get and set posix acl albeit with two caveats. First, that posix acls
> > aren't cached. And second, that they aren't used for permission checking
> > in the vfs.
> >
> > We regressed that use-case as we currently refuse to retrieve any posix
> > acls if they aren't enabled via FUSE_POSIX_ACL. So older fuse daemons
> > would see a change in behavior.
>
> This originates commit e45b2546e23c ("fuse: Ensure posix acls are
> translated outside of init_user_ns") which, disables set/get acl in
> the problematic case instead of translating them.
>
> Not sure if there's a real use case or it's completely theoretical.
> Does anyone know?
After 4+ years without anyone screaming for non-FUSE_POSIX_ACL daemons
to be able set/get posix acls without permission checking in the vfs
inside a userns we can continue not enabling this. Even if we now
actually can.
>
> >
> > We can restore the old behavior in multiple ways. We could change the
> > new posix acl api and look for a dedicated xattr handler and if we find
> > one prefer that over the dedicated posix acl api. That would break the
> > consistency of the new posix acl api so we would very much prefer not to
> > do that.
> >
> > We could introduce a new ACL_*_CACHE sentinel that would instruct the
> > vfs permission checking codepath to not call into the filesystem and
> > ignore acls.
> >
> > But a more straightforward fix for v6.2 is to do the same thing that
> > Overlayfs does and give fuse a separate get acl method for permission
> > checking. Overlayfs uses this to express different needs for vfs
> > permission lookup and acl based retrieval via the regular system call
> > path as well. Let fuse do the same for now. This way fuse can continue
> > to refuse to retrieve posix acls for daemons that don't set
> > FUSE_POSXI_ACL for permission checking while allowing a fuse server to
> > retrieve it via the usual system calls.
> >
> > In the future, we could extend the get acl inode operation to not just
> > pass a simple boolean to indicate rcu lookup but instead make it a flag
> > argument. Then in addition to passing the information that this is an
> > rcu lookup to the filesystem we could also introduce a flag that tells
> > the filesystem that this is a request from the vfs to use these acls for
> > permission checking. Then fuse could refuse the get acl request for
> > permission checking when the daemon doesn't have FUSE_POSIX_ACL set in
> > the same get acl method. This would also help Overlayfs and allow us to
> > remove the second method for it as well.
> >
> > But since that change is more invasive as we need to update the get acl
> > inode operation for multiple filesystems we should not do this as a fix
> > for v6.2. Instead we will do this for the v6.3 merge window.
> >
> > Fwiw, since posix acls are now always correctly translated in the new
> > posix acl api we could also allow them to be used for daemons without
> > FUSE_POSIX_ACL that are not mounted on the host. But this is behavioral
> > change and again if dones should be done for v6.3. For now, let's just
> > restore the original behavior.
>
> Agreed.
>
> >
> > A nice side-effect of this change is that for fuse daemons with and
> > without FUSE_POSIX_ACL the same code is used for posix acls in a
> > backwards compatible way. This also means we can remove the legacy xattr
> > handlers completely. We've also added comments to explain the expected
> > behavior for daemons without FUSE_POSIX_ACL into the code.
> >
> > Fixes: 318e66856dde ("xattr: use posix acl api")
> > Signed-off-by: Seth Forshee (Digital Ocean) <sforshee@xxxxxxxxxx>
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
>
> Reviewed-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
>
>
> > If you're fine with this approach then could you please route this to
> > Linus during v6.2 still? If you prefer I do it I'm happy to as well.
>
> I don't think I have anything pending for v6.2 in fuse, but if you
> don't either I can handle the routing.
I don't but if you'd be fine with me taking it it would make my life
easier for another series.
Thanks!
Christian
