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 from 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?
>
> 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.
Thanks,
Miklos
