[PATCH v2 0/8] acl: remove generic posix acl handlers from all xattr handlers

[PATCH v2 0/8] acl: remove generic posix acl handlers from all xattr handlers

[Christian Brauner]

Hey everyone,

after we finished the introduction of the new posix acl api last cycle
we still left the generic POSIX ACL xattr handlers around in the
filesystems xattr handler registered at sb->s_xattr for two reasons.
First, because a few filesystems rely on the ->list() method of the
generic POSIX ACL xattr handlers in their ->listxattr() inode operation.
Second, during inode initalization in inode_init_always() the registered
xattr handlers in sb->s_xattr are used to raise IOP_XATTR in
inode->i_opflags.

This series adresses both issues and makes it possible to remove the
generic POSIX ACL xattr handlers from the sb->s_xattr list of all
filesystems. This is a crucial step as the generic POSIX ACL xattr
handlers aren't used for POSIX ACLs anymore. They are never used apart
from the two cases above.

To fix this we make POSIX ACLs independent of IOP_XATTR. For filesystems
like btrfs or reiserfs that want to disable xattrs and POSIX ACLs for
some inodes we give them a dedicated IOP_NOACL flag they can raise as
discussed in the previous iteration.

The series also simplifies a the ->listxattr() implementation for all
filesystems that rely on the ->list() method of the xattr handlers.

After this we've removed the generic POSIX ACL xattr handlers completely
from sb->s_xattr.

All filesystems with reasonable integration into xfstests have been
tested with:

        ./check -g acl,attr,cap,idmapped,io_uring,perms,unlink

All tests pass without regression on xfstests for-next branch.

Since erofs doesn't have integration into xfstests yet afaict I have
tested it with the testuite available in erofs-utils. They also all pass
without any regressions.

Thanks!
Christian

[1]: https://lore.kernel.org/lkml/20230125100040.374709-1-brauner@kernel.org
[2]: ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git tags/fs.acl.remove.generic.xattr.handlers.v1

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
Changes in v2:
- Please see changelogs of the individual patches.
- Christoph & Christian:
  Remove SB_I_XATTR and instead introduce IOP_NOACL so filesystems can
  opt out of POSIX ACLs for specific inodes. Decouple POSIX ACLs from
  IOP_XATTR.
- Keep generic posix acl xattr handlers so filesystems that use array
  based indexing on xattr handlers can continue to do so.
- Minor fixes.
- Link to v1: https://lore.kernel.org/r/20230125-fs-acl-remove-generic-xattr-handlers-v1-0-6cf155b492b6@kernel.org

---
Christian Brauner (8):
      fs: don't use IOP_XATTR for posix acls
      xattr: simplify listxattr helpers
      xattr: add listxattr helper
      xattr: remove unused argument
      fs: drop unused posix acl handlers
      fs: simplify ->listxattr() implementation
      reiserfs: rework ->listxattr() implementation
      fs: rename generic posix acl handlers

 fs/9p/xattr.c                   |   4 --
 fs/bad_inode.c                  |   3 +-
 fs/btrfs/inode.c                |   2 +-
 fs/btrfs/xattr.c                |   4 --
 fs/ceph/xattr.c                 |   4 --
 fs/cifs/xattr.c                 |   4 --
 fs/ecryptfs/inode.c             |   4 --
 fs/erofs/xattr.c                |  12 +----
 fs/erofs/xattr.h                |  20 +++++---
 fs/ext2/xattr.c                 |  25 +++++-----
 fs/ext4/xattr.c                 |  25 +++++-----
 fs/f2fs/xattr.c                 |  24 +++++-----
 fs/gfs2/xattr.c                 |   2 -
 fs/jffs2/xattr.c                |  29 ++++++------
 fs/jfs/xattr.c                  |   4 --
 fs/libfs.c                      |   3 +-
 fs/nfs/nfs3_fs.h                |   1 -
 fs/nfs/nfs3acl.c                |   6 ---
 fs/nfs/nfs3super.c              |   3 --
 fs/nfsd/nfs4xdr.c               |   3 +-
 fs/ntfs3/xattr.c                |   4 --
 fs/ocfs2/xattr.c                |  14 ++----
 fs/orangefs/xattr.c             |   2 -
 fs/overlayfs/copy_up.c          |   4 +-
 fs/overlayfs/super.c            |   8 ----
 fs/posix_acl.c                  |  53 +++++++++++++++++----
 fs/reiserfs/inode.c             |   2 +-
 fs/reiserfs/namei.c             |   4 +-
 fs/reiserfs/reiserfs.h          |   2 +-
 fs/reiserfs/xattr.c             |  74 +++++++++++++++--------------
 fs/xattr.c                      | 101 +++++++++++++++-------------------------
 fs/xfs/xfs_xattr.c              |   4 --
 include/linux/fs.h              |   1 +
 include/linux/posix_acl.h       |   7 +++
 include/linux/posix_acl_xattr.h |   5 +-
 include/linux/xattr.h           |  30 +++++++++++-
 mm/shmem.c                      |   4 --
 37 files changed, 245 insertions(+), 256 deletions(-)
---
base-commit: ab072681eabe1ce0a9a32d4baa1a27a2d046bc4a
change-id: 20230125-fs-acl-remove-generic-xattr-handlers-4cfed8558d88

[PATCH v2 7/8] reiserfs: rework ->listxattr() implementation

[Christian Brauner]

Rework reiserfs so it doesn't have to rely on the dummy xattr handlers
in its s_xattr list anymore as this is completely unused for setting and
getting posix acls. Only leave them around for the sake of ->listxattr().

This is somewhat clumsy but reiserfs is marked deprecated and will be
removed in the future anway so I don't feel too bad about it.

Cc: reiserfs-devel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
Changes in v2:
- Rework patch to account for reiserfs quirks.
---
 fs/reiserfs/reiserfs.h |  2 +-
 fs/reiserfs/xattr.c    | 70 +++++++++++++++++++++++++++-----------------------
 2 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/fs/reiserfs/reiserfs.h b/fs/reiserfs/reiserfs.h
index 3aa928ec527a..14726fd353c4 100644
--- a/fs/reiserfs/reiserfs.h
+++ b/fs/reiserfs/reiserfs.h
@@ -1166,7 +1166,7 @@ static inline int bmap_would_wrap(unsigned bmap_nr)
 	return bmap_nr > ((1LL << 16) - 1);
 }
 
-extern const struct xattr_handler *reiserfs_xattr_handlers[];
+extern const struct xattr_handler **reiserfs_xattr_handlers;
 
 /*
  * this says about version of key of all items (but stat data) the
diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
index 2c326b57d4bc..66574fcbe7a9 100644
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -52,6 +52,7 @@
 #include <linux/quotaops.h>
 #include <linux/security.h>
 #include <linux/posix_acl_xattr.h>
+#include <linux/xattr.h>
 
 #define PRIVROOT_NAME ".reiserfs_priv"
 #define XAROOT_NAME   "xattrs"
@@ -770,23 +771,49 @@ reiserfs_xattr_get(struct inode *inode, const char *name, void *buffer,
 			(handler) != NULL;			\
 			(handler) = *(handlers)++)
 
+static const struct xattr_handler *reiserfs_xattr_handlers_max[] = {
+#ifdef CONFIG_REISERFS_FS_POSIX_ACL
+	&posix_acl_access_xattr_handler,
+	&posix_acl_default_xattr_handler,
+#endif
+#ifdef CONFIG_REISERFS_FS_XATTR
+	&reiserfs_xattr_user_handler,
+	&reiserfs_xattr_trusted_handler,
+#endif
+#ifdef CONFIG_REISERFS_FS_SECURITY
+	&reiserfs_xattr_security_handler,
+#endif
+	NULL
+};
+
+/* Actual operations that are exported to VFS-land */
+#ifdef CONFIG_REISERFS_FS_POSIX_ACL
+const struct xattr_handler **reiserfs_xattr_handlers =
+	reiserfs_xattr_handlers_max + 2;
+#else
+const struct xattr_handler **reiserfs_xattr_handlers =
+	reiserfs_xattr_handlers_max;
+#endif
+
 /* This is the implementation for the xattr plugin infrastructure */
-static inline const struct xattr_handler *
-find_xattr_handler_prefix(const struct xattr_handler **handlers,
-			   const char *name)
+static inline bool reiserfs_xattr_list(const char *name, struct dentry *dentry)
 {
-	const struct xattr_handler *xah;
-
-	if (!handlers)
-		return NULL;
+	const struct xattr_handler *xah = NULL;
+	const struct xattr_handler **handlers = reiserfs_xattr_handlers_max;
 
 	for_each_xattr_handler(handlers, xah) {
 		const char *prefix = xattr_prefix(xah);
-		if (strncmp(prefix, name, strlen(prefix)) == 0)
-			break;
+
+		if (strncmp(prefix, name, strlen(prefix)))
+			continue;
+
+		if (!xattr_handler_can_list(xah, dentry))
+			return false;
+
+		return true;
 	}
 
-	return xah;
+	return false;
 }
 
 struct listxattr_buf {
@@ -807,12 +834,7 @@ static bool listxattr_filler(struct dir_context *ctx, const char *name,
 
 	if (name[0] != '.' ||
 	    (namelen != 1 && (name[1] != '.' || namelen != 2))) {
-		const struct xattr_handler *handler;
-
-		handler = find_xattr_handler_prefix(b->dentry->d_sb->s_xattr,
-						    name);
-		if (!handler /* Unsupported xattr name */ ||
-		    (handler->list && !handler->list(b->dentry)))
+		if (!reiserfs_xattr_list(name, b->dentry))
 			return true;
 		size = namelen + 1;
 		if (b->buf) {
@@ -902,22 +924,6 @@ void reiserfs_xattr_unregister_handlers(void) {}
 static int create_privroot(struct dentry *dentry) { return 0; }
 #endif
 
-/* Actual operations that are exported to VFS-land */
-const struct xattr_handler *reiserfs_xattr_handlers[] = {
-#ifdef CONFIG_REISERFS_FS_XATTR
-	&reiserfs_xattr_user_handler,
-	&reiserfs_xattr_trusted_handler,
-#endif
-#ifdef CONFIG_REISERFS_FS_SECURITY
-	&reiserfs_xattr_security_handler,
-#endif
-#ifdef CONFIG_REISERFS_FS_POSIX_ACL
-	&posix_acl_access_xattr_handler,
-	&posix_acl_default_xattr_handler,
-#endif
-	NULL
-};
-
 static int xattr_mount_check(struct super_block *s)
 {
 	/*

-- 
2.34.1

Re: [PATCH v2 1/8] fs: don't use IOP_XATTR for posix acls

[Christoph Hellwig]

Sorry for not keeping up with your flow of ideas, so chiming in now:

> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index c7d1fa526dea..e293eaaed185 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -2090,6 +2090,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th,
>  	if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) {
>  		inode->i_flags |= S_PRIVATE;
>  		inode->i_opflags &= ~IOP_XATTR;
> +		inode->i_op = &reiserfs_privdir_inode_operations;

I wonder if there is any way to set this where reiserfs assigns
the other ops. 

> +const struct inode_operations reiserfs_privdir_inode_operations = {
> +	.create = reiserfs_create,
> +	.lookup = reiserfs_lookup,
> +	.link = reiserfs_link,
> +	.unlink = reiserfs_unlink,
> +	.symlink = reiserfs_symlink,
> +	.mkdir = reiserfs_mkdir,
> +	.rmdir = reiserfs_rmdir,
> +	.mknod = reiserfs_mknod,
> +	.rename = reiserfs_rename,
> +	.setattr = reiserfs_setattr,
> +	.permission = reiserfs_permission,
> +	.fileattr_get = reiserfs_fileattr_get,
> +	.fileattr_set = reiserfs_fileattr_set,
> +};

I suspect many other ops aren't need either, but I really need input
from people that known reiserfs better.

> +	if (likely(!is_bad_inode(inode)))
>  		error = set_posix_acl(mnt_userns, dentry, acl_type, kacl);
>  	else
> +		error = -EIO;

I wonder if the is_bad_inode check should simplify move into
get/set_posix_acl.  But otherwise this patch looks good.

Re: [f2fs-dev] [PATCH v2 0/8] acl: remove generic posix acl handlers from all xattr handlers

[patchwork-bot+f2fs]

Hello:

This patch was applied to jaegeuk/f2fs.git (dev)
by Christian Brauner (Microsoft) <brauner@kernel.org>:

On Mon, 30 Jan 2023 17:41:56 +0100 you wrote:
> Hey everyone,
> 
> after we finished the introduction of the new posix acl api last cycle
> we still left the generic POSIX ACL xattr handlers around in the
> filesystems xattr handler registered at sb->s_xattr for two reasons.
> First, because a few filesystems rely on the ->list() method of the
> generic POSIX ACL xattr handlers in their ->listxattr() inode operation.
> Second, during inode initalization in inode_init_always() the registered
> xattr handlers in sb->s_xattr are used to raise IOP_XATTR in
> inode->i_opflags.
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,v2,6/8] fs: simplify ->listxattr() implementation
    https://git.kernel.org/jaegeuk/f2fs/c/a5488f29835c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html