Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.bugs.dist > #1251480 > unrolled thread

Bug#1108534: e2fsprogs: getfattr returns different values for posix acls when using fuse2fs

Back to article view | Back to linux.debian.bugs.dist

Contents

  Bug#1108534: e2fsprogs: getfattr returns different values for posix acls when using fuse2fs Tim Woodall <debianbugs@woodall.me.uk> - 2025-06-30 20:30 +0200
    Bug#1108534: e2fsprogs: getfattr returns different values for posix acls when using fuse2fs "Theodore Ts'o" <tytso@mit.edu> - 2025-07-02 06:00 +0200
      Bug#1108534: e2fsprogs: getfattr returns different values for posix acls when using fuse2fs Tim Woodall <debianbugs@woodall.me.uk> - 2025-07-02 21:00 +0200
        Bug#1108534: e2fsprogs: getfattr returns different values for posix acls when using fuse2fs "Theodore Ts'o" <tytso@mit.edu> - 2025-07-02 23:20 +0200
          Bug#1108534: e2fsprogs: getfattr returns different values for posix acls when using fuse2fs Tim Woodall <tim@woodall.me.uk> - 2025-07-03 00:40 +0200

#1251480 — Bug#1108534: e2fsprogs: getfattr returns different values for posix acls when using fuse2fs

Package: e2fsprogs
Version: 1.47.0-2
Severity: minor

Dear Maintainer,

Without using fuse at all:
# touch x
# setfacl -m u:root:rw x
# getfattr -m - -d x
# file: x
system.posix_acl_access=0sAgAAAAEABgD/////AgAGAAAAAAAEAAQA/////xAABgD/////IAAEAP////8=


When done via fuse2fs:
# which fuse2fs
/usr/bin/fuse2fs
# mkdir mnt
# fuse2fs img mnt
# touch mnt/x
# setfacl -m u:root:rw mnt/x
# getfattr -m - -d mnt/x
# file: mnt/x
system.posix_acl_access=0sAgAAAAEABgAAAAAAAgAGAAAAAAAEAAQAAAAAABAABgAAAAAAIAAEAAAAAAA=

# umount mnt
# /tmp/fuse2fs img mnt
# getfattr -m - -d mnt/x
# file: mnt/x
system.posix_acl_access=0sAgAAAAEABgD/////AgAGAAAAAAAEAAQA/////xAABgD/////IAAEAP////8=

# umount mnt

Note that the one run with my patched libext2fs gets the same value as 
without using fuse without rewriting the attribute showing that this is 
a read issue only.


diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
index 7723d0f9..d09a13da 100644
--- a/lib/ext2fs/ext_attr.c
+++ b/lib/ext2fs/ext_attr.c
@@ -671,7 +671,7 @@ static errcode_t convert_disk_buffer_to_posix_acl(const void *value, size_t size
 			case ACL_GROUP_OBJ:
 			case ACL_MASK:
 			case ACL_OTHER:
-				entry->e_id = 0;
+				entry->e_id = -1;
 				cp += sizeof(ext4_acl_entry_short);
 				size -= sizeof(ext4_acl_entry_short);
 				break;



-- System Information:
Debian Release: 12.11
  APT prefers stable-security
  APT policy: (500, 'stable-security'), (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 6.1.0-37-amd64 (SMP w/4 CPU threads; PREEMPT)
Kernel taint flags: TAINT_WARN
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /usr/bin/dash
Init: sysvinit (via /sbin/init)

Versions of packages e2fsprogs depends on:
ii  libblkid1    2.38.1-5+deb12u3
ii  libc6        2.36-9+deb12u10
ii  libcom-err2  1.47.0-2
ii  libext2fs2   1.47.0-2
ii  libss2       1.47.0-2
ii  libuuid1     2.38.1-5+deb12u3
ii  logsave      1.47.0-2

Versions of packages e2fsprogs recommends:
pn  e2fsprogs-l10n  <none>

Versions of packages e2fsprogs suggests:
pn  e2fsck-static  <none>
ii  fuse2fs        1.47.0-2
pn  gpart          <none>
pn  parted         <none>

-- no debconf information

[toc] | [next] | [standalone]

#1251625

On Mon, Jun 30, 2025 at 07:08:58PM +0100, Tim Woodall wrote:
> Package: e2fsprogs
> Version: 1.47.0-2
> Severity: minor
> 
> Dear Maintainer,
> 
> Without using fuse at all:
> # touch x
> # setfacl -m u:root:rw x
> # getfattr -m - -d x
> # file: x
> system.posix_acl_access=0sAgAAAAEABgD/////AgAGAAAAAAAEAAQA/////xAABgD/////IAAEAP////8=
> 
> 
> When done via fuse2fs:
> # which fuse2fs
> /usr/bin/fuse2fs
> # mkdir mnt
> # fuse2fs img mnt
> # touch mnt/x
> # setfacl -m u:root:rw mnt/x
> # getfattr -m - -d mnt/x
> # file: mnt/x
> system.posix_acl_access=0sAgAAAAEABgAAAAAAAgAGAAAAAAAEAAQAAAAAABAABgAAAAAAIAAEAAAAAAA=
>

Hmm, I can't replicate this on e2fsprogs 1.47.3~rc2-1.  I believe this
was fixed by this commit:

commit 0111bdb70a9c460052387111414a2e2dc8c06822
Author: Darrick J. Wong <djwong@kernel.org>
Date:   Thu Apr 24 14:41:49 2025 -0700

    fuse2fs: remove posix acl translation
    
    Remove the POSIX ACL format translation since libext2fs takes care of
    that now.

						- Ted

[toc] | [prev] | [next] | [standalone]

#1251689

On Tue, 1 Jul 2025, Theodore Ts'o wrote:

> On Mon, Jun 30, 2025 at 07:08:58PM +0100, Tim Woodall wrote:
>> Package: e2fsprogs
>> Version: 1.47.0-2
>> Severity: minor
>>
>> Dear Maintainer,
>>
>> Without using fuse at all:
>> # touch x
>> # setfacl -m u:root:rw x
>> # getfattr -m - -d x
>> # file: x
>> system.posix_acl_access=0sAgAAAAEABgD/////AgAGAAAAAAAEAAQA/////xAABgD/////IAAEAP////8=
>>
>>
>> When done via fuse2fs:
>> # which fuse2fs
>> /usr/bin/fuse2fs
>> # mkdir mnt
>> # fuse2fs img mnt
>> # touch mnt/x
>> # setfacl -m u:root:rw mnt/x
>> # getfattr -m - -d mnt/x
>> # file: mnt/x
>> system.posix_acl_access=0sAgAAAAEABgAAAAAAAgAGAAAAAAAEAAQAAAAAABAABgAAAAAAIAAEAAAAAAA=
>>
>
> Hmm, I can't replicate this on e2fsprogs 1.47.3~rc2-1.  I believe this
> was fixed by this commit:
>
> commit 0111bdb70a9c460052387111414a2e2dc8c06822
> Author: Darrick J. Wong <djwong@kernel.org>
> Date:   Thu Apr 24 14:41:49 2025 -0700
>
>    fuse2fs: remove posix acl translation
>
>    Remove the POSIX ACL format translation since libext2fs takes care of
>    that now.
>

This is very strange.

I cannot reproduce it on a trixie machine
root@trixie17-build:/build# fuse2fs -V
fuse2fs 1.47.2 (1-Jan-2025)
FUSE library version 3.17.2
using FUSE kernel interface version 7.40
fusermount3 version: 3.17.2
root@trixie17-build:/build# apt-cache policy fuse2fs
fuse2fs:
   Installed: 1.47.2-3+b1

root@trixie17-build:~# uname -a
Linux trixie17-build.home.woodall.me.uk 6.12.27-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.12.27-1 (2025-05-06) x86_64 GNU/Linux

root@trixie17-build:~# setfacl -m u:root:rw mnt/x
root@trixie17-build:~# getfattr -m - -d mnt/x
# file: mnt/x
system.posix_acl_access=0sAgAAAAEABgD/////AgAGAAAAAAAEAAQA/////xAABgD/////IAAEAP////8=

But when I build the latest code from master on a bookworm machine I 
can:
root@dirac:/build/dump-sf/testing/scripts# /tmp/fuse2fs -V
fuse2fs 1.47.3-rc2 (12-Jun-2025)
FUSE library version 3.14.0
using FUSE kernel interface version 7.31
fusermount3 version: 3.14.0

root@dirac:/build/dump-sf/testing/scripts# uname -a
Linux dirac.home.woodall.me.uk 6.1.0-37-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.140-1 (2025-05-22) x86_64 GNU/Linux

root@dirac:/build/dump-sf/testing/scripts# getfattr -m - -d mnt/x
# file: mnt/x
system.posix_acl_access=0sAgAAAAEABgAAAAAAAgAGAAAAAAAEAAQAAAAAABAABgAAAAAAIAAEAAAAAAA=


And I've just realized that the trixie version is in bookworm-backports. 
Installing that and I get the same:

root@dirac:/build/dump-sf/testing/scripts# apt-cache policy fuse2fs
fuse2fs:
   Installed: 1.47.2-3~bpo12+1

root@dirac:/build/dump-sf/testing/scripts# fuse2fs -V
fuse2fs 1.47.2 (1-Jan-2025)
FUSE library version 3.14.0
using FUSE kernel interface version 7.31
fusermount3 version: 3.14.0

root@dirac:/build/dump-sf/testing/scripts# getfattr -m - -d mnt/x
# file: mnt/x
system.posix_acl_access=0sAgAAAAEABgAAAAAAAgAGAAAAAAAEAAQAAAAAABAABgAAAAAAIAAEAAAAAAA=


So it seems that my change is only needed on older kernels.

Tim.

[toc] | [prev] | [next] | [standalone]

#1251692

On Wed, Jul 02, 2025 at 07:44:24PM +0100, Tim Woodall wrote:
> 
> So it seems that my change is only needed on older kernels.
>

OK, thanks for the hint.  Using the same fuse2fs binary (built in a
bookworm-amd64 chroot), I can replicate what you're seeing on 6.1.142:

<tytso@trampoline> {/usr/projects/linux/ext4-6.1}   ((v6.1.142))
1477% kvm-xfstests shell
  ...
root@kvm-xfstests:~# /vtmp/fuse2fs  /vtmp/foo.img /mnt
FUSE2FS (foo.img): Warning: fuse2fs does not support using the journal.
There may be file system corruption or data loss if
the file system is not gracefully unmounted.
FUSE2FS (foo.img): Warning: Mounting unchecked fs, running e2fsck is recommended.
root@kvm-xfstests:~# getfattr -m - -d /mnt/x
getfattr: Removing leading '/' from absolute path names
# file: mnt/x
system.posix_acl_access=0sAgAAAAEABgAAAAAAAgAGAAAAAAAEAAQAAAAAABAABgAAAAAAIAAEAAAAAAA=

.... but not 6.6.95:

<tytso@trampoline> {/usr/projects/linux/ext4-6.6}   ((v6.6.95))
1479% kvm-xfstests shell
   ...
root@kvm-xfstests:~# /vtmp/fuse2fs  /vtmp/foo.img /mnt
FUSE2FS (foo.img): Warning: fuse2fs does not support using the journal.
There may be file system corruption or data loss if
the file system is not gracefully unmounted.
FUSE2FS (foo.img): Warning: Mounting unchecked fs, running e2fsck is recommended.
root@kvm-xfstests:~# getfattr -m - -d /mnt/x
getfattr: Removing leading '/' from absolute path names
# file: mnt/x
system.posix_acl_access=0sAgAAAAEABgD/////AgAGAAAAAAAEAAQA/////xAABgD/////IAAEAP////8=

So it looks like some kind of bug fix or kernel change which landed
sometime between 6.1 and 6.6, and which apparently was not backported
to the 6.1 LTS kernel.   

Hmm... digging a bit deeper to see what changed in the kernel's fuse
implementation between 6.1 and 6.6, it looks like there were some
*radical changes*.

% git log --reverse v6.1.. --grep acl -i fs/fuse

Turns up these two interesting patches:

commit cac2f8b8d8b50ef32b3e34f6dcbbf08937e4f616
Author: Christian Brauner <brauner@kernel.org>
Date:   Thu Sep 22 17:17:00 2022 +0200

    fs: rename current get acl method
    
    The current way of setting and getting posix acls through the generic
    xattr interface is error prone and type unsafe. The vfs needs to
    interpret and fixup posix acls before storing or reporting it to
    userspace. Various hacks exist to make this work. The code is hard to
    understand and difficult to maintain in it's current form. Instead of
    making this work by hacking posix acls through xattr handlers we are
    building a dedicated posix acl api around the get and set inode
    operations. This removes a lot of hackiness and makes the codepaths
    easier to maintain. A lot of background can be found in [1].
    
    The current inode operation for getting posix acls takes an inode
    argument but various filesystems (e.g., 9p, cifs, overlayfs) need access
    to the dentry. In contrast to the ->set_acl() inode operation we cannot
    simply extend ->get_acl() to take a dentry argument. The ->get_acl()
    inode operation is called from:
    
    acl_permission_check()
    -> check_acl()
       -> get_acl()
    
    which is part of generic_permission() which in turn is part of
    inode_permission(). Both generic_permission() and inode_permission() are
    called in the ->permission() handler of various filesystems (e.g.,
    overlayfs). So simply passing a dentry argument to ->get_acl() would
    amount to also having to pass a dentry argument to ->permission(). We
    should avoid this unnecessary change.
    
    So instead of extending the existing inode operation rename it from
    ->get_acl() to ->get_inode_acl() and add a ->get_acl() method later that
    passes a dentry argument and which filesystems that need access to the
    dentry can implement instead of ->get_inode_acl(). Filesystems like cifs
    which allow setting and getting posix acls but not using them for
    permission checking during lookup can simply not implement
    ->get_inode_acl().
    
    This is intended to be a non-functional change.
    
    Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
    Suggested-by/Inspired-by: Christoph Hellwig <hch@lst.de>
    Reviewed-by: Christoph Hellwig <hch@lst.de>

... and ...

commit 6a518afcc2066732e6c5c24281ce017bbbd85506
Merge: bd90741318ee d6fdf29f7b99
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Dec 12 18:46:39 2022 -0800

    Merge tag 'fs.acl.rework.v6.2' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping
    
    Pull VFS acl updates from Christian Brauner:
     "This contains the work that builds a dedicated vfs posix acl api.
    
      The origins of this work trace back to v5.19 but it took quite a while
      to understand the various filesystem specific implementations in
      sufficient detail and also come up with an acceptable solution.
    
      As we discussed and seen multiple times the current state of how posix
      acls are handled isn't nice and comes with a lot of problems: The
      current way of handling posix acls via the generic xattr api is error
      prone, hard to maintain, and type unsafe for the vfs until we call
      into the filesystem's dedicated get and set inode operations.
    
      It is already the case that posix acls are special-cased to death all
      the way through the vfs. There are an uncounted number of hacks that
      operate on the uapi posix acl struct instead of the dedicated vfs
      struct posix_acl. And the vfs must be involved in order to interpret
      and fixup posix acls before storing them to the backing store, caching
      them, reporting them to userspace, or for permission checking.
    
      Currently a range of hacks and duct tape exist to make this work. As
      with most things this is really no ones fault it's just something that
      happened over time. But the code is hard to understand and difficult
      to maintain and one is constantly at risk of introducing bugs and
      regressions when having to touch it.
    
      Instead of continuing to hack posix acls through the xattr handlers
      this series builds a dedicated posix acl api solely around the get and
      set inode operations.
    
      Going forward, the vfs_get_acl(), vfs_remove_acl(), and vfs_set_acl()
      helpers must be used in order to interact with posix acls. They
      operate directly on the vfs internal struct posix_acl instead of
      abusing the uapi posix acl struct as we currently do. In the end this
      removes all of the hackiness, makes the codepaths easier to maintain,
      and gets us type safety.
    
      (several more pagefuls of testing and implementation details removed)

The thing is, for the ACL types in question, the kernel shouldn't even
be looking at e_id; that field is undefined for ACL_USER_OBJ,
ACL_GROUP_OBJ, et.al, since the kernel would be using the inode's user
and group ownership, and for ACL_MASK and ACL_OTHER there is no id
number to be used.  So the kernel shouldn't even be *looking* at that
field.  So whether the value is 0 or -1 shouldn't make a difference.
And change that I don't understand always give me pause....

BTW, what caused you to think that -1 might fix things on a Bookworm
6.1 kernel?

						- Ted

[toc] | [prev] | [next] | [standalone]

#1251698

On Wed, 2 Jul 2025, Theodore Ts'o wrote:

> On Wed, Jul 02, 2025 at 07:44:24PM +0100, Tim Woodall wrote:
>>
>> So it seems that my change is only needed on older kernels.
>>
>
> OK, thanks for the hint.  Using the same fuse2fs binary (built in a
> bookworm-amd64 chroot), I can replicate what you're seeing on 6.1.142:
>

> The thing is, for the ACL types in question, the kernel shouldn't even
> be looking at e_id; that field is undefined for ACL_USER_OBJ,
> ACL_GROUP_OBJ, et.al, since the kernel would be using the inode's user
> and group ownership, and for ACL_MASK and ACL_OTHER there is no id
> number to be used.  So the kernel shouldn't even be *looking* at that
> field.  So whether the value is 0 or -1 shouldn't make a difference.
> And change that I don't understand always give me pause....
>
I agree. I tried digging through the kernel code but I couldn't work out 
where the -1 wad coming from when not using fuse. I thought zero seemed 
more likely! If I followed the right bits there's a kvzmalloc where it 
goes to userspace.  I gave up at this point, I don't know kernel 
programming and I was probably wasting my time.

> BTW, what caused you to think that -1 might fix things on a Bookworm
> 6.1 kernel?
>

Here is near identical code:

https://sourceforge.net/p/dump/code/ci/main/tree/restore/xattr.c#l284

and this is how I found it (by luck) when an EA failed to verify.

I patched this code (to match libext2fs) and then recently discovered 
that it wad now failing when not testing on fuse.

I don't think this 0/-1 value is used, it's just exposed via the 
getfattr interface to this EA.

Tim.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.bugs.dist

csiph-web