do not archive ACLs that are equivalent to the mode bits (and fix FreeBSD default ACL loss) - #10040
Draft
ThomasWaldmann wants to merge 2 commits into
Draft
do not archive ACLs that are equivalent to the mode bits (and fix FreeBSD default ACL loss)#10040ThomasWaldmann wants to merge 2 commits into
ThomasWaldmann wants to merge 2 commits into
Conversation
A directory that only has a default ACL still counts as "extended" for acl_extended_file_nofollow/acl_extended_fd, so borg archived its access ACL, although that ACL just mirrors the traditional permission bits - the kernel does not keep a system.posix_acl_access xattr for it either. "borg mount" then offered such a bogus xattr. Same the other way round: a directory without a default ACL got an empty acl_default archived. Now only archive the access ACL if it is not equivalent to the mode bits (acl_equiv_mode) and the default ACL if it has any entries (acl_entries). The FUSE mounts additionally skip trivial/empty ACLs (acl_is_extended), so archives created by older borg versions do not expose these xattrs either.
acl_extended_link_np() only inspects the access (resp. NFSv4) ACL - see _acl_extended_file() in lib/libc/posix1e/acl_extended_file_np.c, which runs acl_is_trivial_np() on ACL_TYPE_ACCESS. So a directory that has only a default ACL does not count as "extended", borg returned early and never archived that default ACL at all - restoring it silently lost the inheritance policy. Now acl_extended_link_np() only gates the access / NFSv4 ACL, and for a directory on a POSIX.1e filesystem (checked via _PC_ACL_EXTENDED, just like acl_set does already) the default ACL is looked at in any case. Also, do not archive empty ACLs: for a directory without a default ACL the kernel returns success and an empty ACL (see ufs_getacl_posix1e()), so borg stored acl_default = b'' for those.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10040 +/- ##
==========================================
+ Coverage 86.18% 86.19% +0.01%
==========================================
Files 96 96
Lines 17434 17449 +15
Branches 2665 2671 +6
==========================================
+ Hits 15025 15040 +15
Misses 1668 1668
Partials 741 741 ☔ View full report in Codecov by Harness. |
ThomasWaldmann
marked this pull request as draft
August 5, 2026 11:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #9954: a directory that has only a default ACL got a
system.posix_acl_accessxattr archived and exposed byborg mount, although neither the source filesystem nor the kernel has such an xattr for it. Looking into it turned up the same class of problem on FreeBSD, where it is worse. One commit per platform.1. Linux: do not archive ACLs that are equivalent to the mode bits
acl_extended_file_nofollow()/acl_extended_fd()also consider the default ACL of a directory, so they return 1 for a directory that only has a default ACL.acl_get()took that as "there is an extended ACL" and archived the access ACL too - which for such a directory is just the traditional permission bits:The kernel keeps no
system.posix_acl_accessxattr for a mode-equivalent ACL, so the mount was offering an xattr that does not exist on the source. The mirror image of the same wart: a directory without a default ACL gotacl_default = b''archived (acl_to_any_text()of an empty ACL).Now
acl_accessis only archived ifacl_equiv_mode(acl, NULL) != 0(-1= error still archives, so an odd ACL is never silently dropped) andacl_defaultonly ifacl_entries(acl) > 0.Extraction is unaffected: a skipped mode-equivalent access ACL is exactly the mode bits, and a skipped empty default ACL is what
acl_set_file(ACL_TYPE_DEFAULT, <empty>)produced anyway.Additionally, the FUSE mounts now skip trivial/empty ACLs (new generic
acl_is_extended()inplatform/base.py, used byfuse.pyandhlfuse.py), so archives created by older borg versions stop exposing these xattrs too.2. FreeBSD: do not lose the default ACL of a directory
The same starting point, but FreeBSD's libc behaves the other way round - and the result is data loss rather than noise.
acl_extended_link_np()inspects only the access (resp. NFSv4) ACL; see_acl_extended_file()inlib/libc/posix1e/acl_extended_file_np.c, which runsacl_is_trivial_np()onACL_TYPE_ACCESS. So a directory that only has a default ACL does not count as "extended",acl_get()returned early, and the default ACL was never archived at all - restoring such a directory silently loses its inheritance policy.That call now only gates the access / NFSv4 ACL. For a directory on a POSIX.1e filesystem the default ACL is looked at in any case, using
_PC_ACL_EXTENDED- the very probeacl_set()already uses on the restore side, so both directions now ask the same question.Also, empty ACLs are no longer archived: for a directory without a default ACL the kernel returns success and an empty ACL (see
ufs_getacl_posix1e()insys/ufs/ufs/ufs_acl.c), which borg stored asacl_default = b''.Cost: on an
acls-mounted UFS every directory now gets one default-ACL fetch, and on ZFS every directory gets one extralpathconf- unavoidable if we must not rely on a call that cannot see default ACLs.Behaviour per case (FreeBSD): file without ACLs still returns early; file/dir with an access ACL unchanged; ZFS/NFSv4 still takes the NFS4 branch (only reachable when extended, so no change); dir with only a default ACL now archives it; dir with no ACLs still yields
{}. On a filesystem without ACL supportacl_extended_link_np()still fails withEOPNOTSUPPbefore any of this, whicharchive.pyswallows as before.Other platforms
acl_get()only deals withACL_TYPE_EXTENDED(no access/default split) and macOS returnsNULL/ENOENTwhen there is no ACL, whichdarwin.pyxalready handles. Checked on a Mac: plain dir ->{}, dir with an ACL -> only the real entry.base.pyno-opacl_get/acl_set, nothing to do.ACL_XATTRS = {...} if is_linux else {}).Tests
platform/linux_test.py:test_default_acl_only,test_access_acl_only_no_empty_default,test_acl_is_extended.platform/freebsd_test.py: the two analogous ACL tests.archiver/mount_cmds_test.py::test_fuse_acls: extended with adir2that has only a default ACL - asserts the access xattr is neither listed nor readable through the mount, that the default xattr matches the source byte for byte, and thatacl_get()through the mount sees no access ACL.Both new Linux platform tests fail on master with exactly the two bogus values above, and pass with the fix.
Verification status
Linux is fully verified: the platform tests and the full
test_fuse_acls(including the ACL-xattr passthrough assertions, which need a container outside a user namespace - inside one the kernel refusesgetxattrofsystem.posix_acl_*on FUSE mounts, as noted in #9954) pass, along withplatform/,mount_cmds,fuse,create,extract,diffanditem.The FreeBSD commit is untested on real FreeBSD - I had no FreeBSD system at hand. It is derived from the freebsd-src sources quoted above and the
.pyxwas Cython-translated cleanly, but that is a syntax check, not a behaviour check. Note also that the FreeBSDvm_testsjob iscontinue-on-errorand the ACL tests areskipif_acls_not_working, so they will skip unless that VM's filesystem is mounted withacls. Please give it a run on a FreeBSD box withmount -o aclsbefore trusting that commit.