Skip to content
Snippets Groups Projects
  • Linus Torvalds's avatar
    Merge tag 'fs.acl.rework.v6.2' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping · 6a518afc
    Linus Torvalds authored
    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.
    
      This series passes the LTP and xfstests suites without any
      regressions. For xfstests the following combinations were tested:
       - xfs
       - ext4
       - btrfs
       - overlayfs
       - overlayfs on top of idmapped mounts
       - orangefs
       - (limited) cifs
    
      There's more simplifications for posix acls that we can make in the
      future if the basic api has made it.
    
      A few implementation details:
    
       - The series makes sure to retain exactly the same security and
         integrity module permission checks. Especially for the integrity
         modules this api is a win because right now they convert the uapi
         posix acl struct passed to them via a void pointer into the vfs
         struct posix_acl format to perform permission checking on the mode.
    
         There's a new dedicated security hook for setting posix acls which
         passes the vfs struct posix_acl not a void pointer. Basing checking
         on the posix acl stored in the uapi format is really unreliable.
         The vfs currently hacks around directly in the uapi struct storing
         values that frankly the security and integrity modules can't
         correctly interpret as evidenced by bugs we reported and fixed in
         this area. It's not necessarily even their fault it's just that the
         format we provide to them is sub optimal.
    
       - Some filesystems like 9p and cifs need access to the dentry in
         order to get and set posix acls which is why they either only
         partially or not even at all implement get and set inode
         operations. For example, cifs allows setxattr() and getxattr()
         operations but doesn't allow permission checking based on posix
         acls because it can't implement a get acl inode operation.
    
         Thus, this patch series updates the set acl inode operation to take
         a dentry instead of an inode argument. However, for the get acl
         inode operation we can't do this as the old get acl method is
         called in e.g., generic_permission() and inode_permission(). These
         helpers in turn are called in various filesystem's permission inode
         operation. So passing a dentry argument to the old get acl inode
         operation would amount to passing a dentry to the permission inode
         operation which we shouldn't and probably can't do.
    
         So instead of extending the existing inode operation Christoph
         suggested to add a new one. He also requested to ensure that the
         get and set acl inode operation taking a dentry are consistently
         named. So for this version the old get acl operation is renamed to
         ->get_inode_acl() and a new ->get_acl() inode operation taking a
         dentry is added. With this we can give both 9p and cifs get and set
         acl inode operations and in turn remove their complex custom posix
         xattr handlers.
    
         In the future I hope to get rid of the inode method duplication but
         it isn't like we have never had this situation. Readdir is just one
         example. And frankly, the overall gain in type safety and the more
         pleasant api wise are simply too big of a benefit to not accept
         this duplication for a while.
    
       - We've done a full audit of every codepaths using variant of the
         current generic xattr api to get and set posix acls and
         surprisingly it isn't that many places. There's of course always a
         chance that we might have missed some and if so I'm sure we'll find
         them soon enough.
    
         The crucial codepaths to be converted are obviously stacking
         filesystems such as ecryptfs and overlayfs.
    
         For a list of all callers currently using generic xattr api helpers
         see [2] including comments whether they support posix acls or not.
    
       - The old vfs generic posix acl infrastructure doesn't obey the
         create and replace semantics promised on the setxattr(2) manpage.
         This patch series doesn't address this. It really is something we
         should revisit later though.
    
      The patches are roughly organized as follows:
    
       (1) Change existing set acl inode operation to take a dentry
           argument (Intended to be a non-functional change)
    
       (2) Rename existing get acl method (Intended to be a non-functional
           change)
    
       (3) Implement get and set acl inode operations for filesystems that
           couldn't implement one before because of the missing dentry.
           That's mostly 9p and cifs (Intended to be a non-functional
           change)
    
       (4) Build posix acl api, i.e., add vfs_get_acl(), vfs_remove_acl(),
           and vfs_set_acl() including security and integrity hooks
           (Intended to be a non-functional change)
    
       (5) Implement get and set acl inode operations for stacking
           filesystems (Intended to be a non-functional change)
    
       (6) Switch posix acl handling in stacking filesystems to new posix
           acl api now that all filesystems it can stack upon support it.
    
       (7) Switch vfs to new posix acl api (semantical change)
    
       (8) Remove all now unused helpers
    
       (9) Additional regression fixes reported after we merged this into
           linux-next
    
      Thanks to Seth for a lot of good discussion around this and
      encouragement and input from Christoph"
    
    * tag 'fs.acl.rework.v6.2' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping: (36 commits)
      posix_acl: Fix the type of sentinel in get_acl
      orangefs: fix mode handling
      ovl: call posix_acl_release() after error checking
      evm: remove dead code in evm_inode_set_acl()
      cifs: check whether acl is valid early
      acl: make vfs_posix_acl_to_xattr() static
      acl: remove a slew of now unused helpers
      9p: use stub posix acl handlers
      cifs: use stub posix acl handlers
      ovl: use stub posix acl handlers
      ecryptfs: use stub posix acl handlers
      evm: remove evm_xattr_acl_change()
      xattr: use posix acl api
      ovl: use posix acl api
      ovl: implement set acl method
      ovl: implement get acl method
      ecryptfs: implement set acl method
      ecryptfs: implement get acl method
      ksmbd: use vfs_remove_acl()
      acl: add vfs_remove_acl()
      ...
    6a518afc