mbox series

[00/12] acl: remove remaining posix acl handlers

Message ID 20230125-fs-acl-remove-generic-xattr-handlers-v1-0-6cf155b492b6@kernel.org (mailing list archive)
Headers show
Series acl: remove remaining posix acl handlers | expand

Message

Christian Brauner Jan. 25, 2023, 11:28 a.m. UTC
Hey everyone,

after we finished the introduction of the new posix acl api last cycle
we still left the generic POSIX ACL xattr handler around for two
reasons. First, because a few filesystems relied 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.

With the removal of the legacy POSIX ACL handlers it is at least
possible for a filesystem to only implement POSIX ACLs but no other
xattrs. If that were to happen we would miss to raise IOP_XATTR because
sb->s_xattr would be NULL.

Fix these things and then get rid of the misleading and effectively
already unused generic POSIX ACL handlers.

For most filesystems it is a trivial removal of the generic POSIX ACL
handlers. Only for erofs, ext2, ext4, f2fs, jffs2, reiserfs, oc2fs the
handler is used but rather easy to fix.

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.

This branch depends on [1] which hopefully should be merged soon and can
be pulled from [2] which already includes [1] so it's easy to test and
compile.

With this all remnants of the old POSIX ACL xattr handling will be gone.

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>
---
Christian Brauner (12):
      xattr: simplify listxattr helpers
      xattr, posix acl: add listxattr helpers
      xattr: remove unused argument
      fs: drop unused posix acl handlers
      erofs: drop posix acl handlers
      ext2: drop posix acl handlers
      ext4: drop posix acl handlers
      f2fs: drop posix acl handlers
      jffs2: drop posix acl handlers
      ocfs2: drop posix acl handlers
      reiserfs: drop posix acl handlers
      acl: remove posix acl handlers

 fs/9p/xattr.c                   |   4 --
 fs/btrfs/xattr.c                |   4 --
 fs/ceph/xattr.c                 |   4 --
 fs/cifs/xattr.c                 |   4 --
 fs/ecryptfs/inode.c             |   4 --
 fs/erofs/xattr.c                |  49 ++++++++++++----
 fs/erofs/xattr.h                |  21 -------
 fs/ext2/xattr.c                 |  60 +++++++++++--------
 fs/ext4/xattr.c                 |  71 +++++++++++++----------
 fs/f2fs/xattr.c                 |  63 ++++++++++++--------
 fs/gfs2/xattr.c                 |   2 -
 fs/jffs2/xattr.c                |  42 +++++++-------
 fs/jfs/xattr.c                  |   4 --
 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                |  41 +++++++------
 fs/orangefs/xattr.c             |   2 -
 fs/overlayfs/super.c            |   8 ---
 fs/posix_acl.c                  |  20 -------
 fs/reiserfs/xattr.c             |  38 ++++++------
 fs/xattr.c                      | 124 ++++++++++++++++++++--------------------
 fs/xfs/xfs_xattr.c              |   4 --
 include/linux/posix_acl_xattr.h |   6 +-
 include/linux/xattr.h           |   8 ++-
 mm/shmem.c                      |   4 --
 28 files changed, 290 insertions(+), 314 deletions(-)
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230125-fs-acl-remove-generic-xattr-handlers-4cfed8558d88

Comments

Christian Brauner Jan. 30, 2023, 9:10 a.m. UTC | #1
On Wed, Jan 25, 2023 at 12:28:45PM +0100, Christian Brauner wrote:
> Hey everyone,
> 
> after we finished the introduction of the new posix acl api last cycle
> we still left the generic POSIX ACL xattr handler around for two
> reasons. First, because a few filesystems relied 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.
> 
> With the removal of the legacy POSIX ACL handlers it is at least
> possible for a filesystem to only implement POSIX ACLs but no other
> xattrs. If that were to happen we would miss to raise IOP_XATTR because
> sb->s_xattr would be NULL.
> 
> Fix these things and then get rid of the misleading and effectively
> already unused generic POSIX ACL handlers.
> 
> For most filesystems it is a trivial removal of the generic POSIX ACL
> handlers. Only for erofs, ext2, ext4, f2fs, jffs2, reiserfs, oc2fs the
> handler is used but rather easy to fix.
> 
> 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.
> 
> This branch depends on [1] which hopefully should be merged soon and can
> be pulled from [2] which already includes [1] so it's easy to test and
> compile.
> 
> With this all remnants of the old POSIX ACL xattr handling will be gone.
> 
> 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>
> ---
> Christian Brauner (12):
>       xattr: simplify listxattr helpers
>       xattr, posix acl: add listxattr helpers
>       xattr: remove unused argument
>       fs: drop unused posix acl handlers
>       erofs: drop posix acl handlers
>       ext2: drop posix acl handlers
>       ext4: drop posix acl handlers
>       f2fs: drop posix acl handlers
>       jffs2: drop posix acl handlers
>       ocfs2: drop posix acl handlers
>       reiserfs: drop posix acl handlers
>       acl: remove posix acl handlers

I just realized that b4 dropped a patch when I created a series to
track. So this is missing an important bit. Christoph, Al, the
SB_I_XATTR bit is the best thing that I came up with so far. It works
well enough and I find the handler based logic weird anyway. But I'm
open to other ideas.

From e661c29b999f3d5d6293046ea1fe9e7d40cfa453 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Wed, 18 Jan 2023 10:00:00 +0100
Subject: [PATCH 01/14] fs: add SB_I_XATTR

Last cycle we introduced a dedicated POSIX ACLs api which only relies on
the associated inode operations. It does not depend on the xattr handler
infrastructure anymore.

However, a few filesystems still rely on the ->list() method of the
generix POSIX ACL xattr handlers in their ->listxattr() inode operation.
This is a very limited set of filesystems. For most of them there is no
dependence on the generic POSIX ACL xattr handler in any way.

In addition, 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.

With the incoming removal of the legacy POSIX ACL handlers it is at
least possible for a filesystem to only implement POSIX ACLs but no
other xattrs. If that were to happen we would miss to raise IOP_XATTR
because sb->s_xattr would be NULL. While there currently is no such
filesystem we should still make sure that this just works should it ever
happen in the future.

We could try and use raise IOP_XATTR if we detect that the filesystems
implements the associated inode operations. But this doesn't work at the
time inode_init_always() is called because the inode->i_op field is not
yet initialized with the correct filesystem inode operations.

We could of course make the filesystems responsible for raising IOP_XATR
but this is messy as quite a few filesystems initialize inode->i_op in
multiple locations meaning we would have to scatter the checks to raise
IOP_XATTR across the filesystems. This is doable but ugly.

Another possibility would be to just check for SB_POSIXACL in
sb->s_flags and then raise IOP_XATTR. However, this doesn't work as for
some filesystems this flag can be turned on or off. For example, both
btrfs and ext4 support the "acl" and "noacl" mount option to turn
SB_POSIXACL on or off.

So if the filesystem is mounted with "noacl" then we cannot use
SB_POSIXACL as it might not be raised because the filesystem temporarily
disabled POSIX ACLs.

We could try and get rid of IOP_XATTR but it is used in subtle ways to
communicate that a given inode doesn't support xattrs even though the
superblock does in general and is especially prevalent in the lsm world.
This could potentially be solved by having each inode have a separate
xattr handler field instead of the superblock but would bloat each inode
and would be a more substantial change.

The option that chosen here is to introduce SB_I_XATTR which is raised
in sb->s_iflags whenever the superblock supports xattrs. That
encompasses POSIX ACLs and handler based xattrs. The flag will become
even more useful once fscaps have a dedicated inode operation and we're
really not short of flags anyway.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/9p/vfs_super.c    | 2 ++
 fs/afs/super.c       | 4 +++-
 fs/btrfs/super.c     | 1 +
 fs/ceph/super.c      | 1 +
 fs/cifs/cifsfs.c     | 1 +
 fs/ecryptfs/main.c   | 1 +
 fs/erofs/super.c     | 1 +
 fs/ext2/super.c      | 1 +
 fs/ext4/super.c      | 1 +
 fs/f2fs/super.c      | 1 +
 fs/fuse/inode.c      | 4 ++++
 fs/gfs2/ops_fstype.c | 1 +
 fs/hfs/super.c       | 1 +
 fs/hfsplus/super.c   | 1 +
 fs/inode.c           | 2 +-
 fs/jffs2/super.c     | 1 +
 fs/jfs/super.c       | 1 +
 fs/kernfs/mount.c    | 1 +
 fs/libfs.c           | 2 ++
 fs/nfs/super.c       | 3 +++
 fs/ntfs3/super.c     | 1 +
 fs/ocfs2/super.c     | 1 +
 fs/orangefs/super.c  | 1 +
 fs/overlayfs/super.c | 2 +-
 fs/reiserfs/super.c  | 1 +
 fs/squashfs/super.c  | 1 +
 fs/ubifs/super.c     | 1 +
 fs/xfs/xfs_super.c   | 1 +
 include/linux/fs.h   | 2 ++
 mm/shmem.c           | 2 ++
 30 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index 266c4693e20c..003191b6ad97 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -93,6 +93,8 @@ v9fs_fill_super(struct super_block *sb, struct v9fs_session_info *v9ses,
 		sb->s_flags |= SB_POSIXACL;
 #endif
 
+	sb->s_iflags |= SB_I_XATTR;
+
 	return 0;
 }
 
diff --git a/fs/afs/super.c b/fs/afs/super.c
index 95d713074dc8..379e6d7ac827 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -457,8 +457,10 @@ static int afs_fill_super(struct super_block *sb, struct afs_fs_context *ctx)
 	sb->s_maxbytes		= MAX_LFS_FILESIZE;
 	sb->s_magic		= AFS_FS_MAGIC;
 	sb->s_op		= &afs_super_ops;
-	if (!as->dyn_root)
+	if (!as->dyn_root) {
 		sb->s_xattr	= afs_xattr_handlers;
+		sb->s_iflags	|= SB_I_XATTR;
+	}
 	ret = super_setup_bdi(sb);
 	if (ret)
 		return ret;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 93f52ee85f6f..9af31008aafa 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1141,6 +1141,7 @@ static int btrfs_fill_super(struct super_block *sb,
 #endif
 	sb->s_flags |= SB_I_VERSION;
 	sb->s_iflags |= SB_I_CGROUPWB;
+	sb->s_iflags |= SB_I_XATTR;
 
 	err = super_setup_bdi(sb);
 	if (err) {
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 3fc48b43cab0..5aeb4fa0c4b0 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1125,6 +1125,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
 	s->s_time_min = 0;
 	s->s_time_max = U32_MAX;
 	s->s_flags |= SB_NODIRATIME | SB_NOATIME;
+	s->s_iflags |= SB_I_XATTR;
 
 	ret = set_anon_super_fc(s, fc);
 	if (ret != 0)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 10e00c624922..6a938b93d75d 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -235,6 +235,7 @@ cifs_read_super(struct super_block *sb)
 	sb->s_magic = CIFS_SUPER_MAGIC;
 	sb->s_op = &cifs_super_ops;
 	sb->s_xattr = cifs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	rc = super_setup_bdi(sb);
 	if (rc)
 		goto out_no_root;
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index 2dc927ba067f..4ae69d23bc47 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -521,6 +521,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	sbi = NULL;
 	s->s_op = &ecryptfs_sops;
 	s->s_xattr = ecryptfs_xattr_handlers;
+	s->s_iflags |= SB_I_XATTR;
 	s->s_d_op = &ecryptfs_dops;
 
 	err = "Reading sb failed";
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 481788c24a68..ea349cea9229 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -772,6 +772,7 @@ static int erofs_fc_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	sb->s_time_gran = 1;
 	sb->s_xattr = erofs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	sb->s_export_op = &erofs_export_ops;
 
 	if (test_opt(&sbi->opt, POSIX_ACL))
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 69c88facfe90..ee6e0c4978d1 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1171,6 +1171,7 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op = &ext2_sops;
 	sb->s_export_op = &ext2_export_ops;
 	sb->s_xattr = ext2_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 
 #ifdef CONFIG_QUOTA
 	sb->dq_op = &dquot_operations;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 260c1b3e3ef2..2706930beed2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5104,6 +5104,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 
 	/* i_version is always enabled now */
 	sb->s_flags |= SB_I_VERSION;
+	sb->s_iflags |= SB_I_XATTR;
 
 	if (ext4_check_feature_compatibility(sb, es, silent))
 		goto failed_mount;
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 1f812b9ce985..6fff82f0efdf 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4217,6 +4217,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		(test_opt(sbi, POSIX_ACL) ? SB_POSIXACL : 0);
 	memcpy(&sb->s_uuid, raw_super->uuid, sizeof(raw_super->uuid));
 	sb->s_iflags |= SB_I_CGROUPWB;
+	sb->s_iflags |= SB_I_XATTR;
 
 	/* init f2fs-specific super block info */
 	sbi->valid_super_block = valid_super_block;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index de9b9ec5ce81..c9952bb9f9d5 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1181,6 +1181,7 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 			if ((flags & FUSE_POSIX_ACL)) {
 				fc->default_permissions = 1;
 				fc->posix_acl = 1;
+				fm->sb->s_iflags |= SB_I_XATTR;
 			}
 			if (flags & FUSE_CACHE_SYMLINKS)
 				fc->cache_symlinks = 1;
@@ -1426,6 +1427,7 @@ static void fuse_sb_defaults(struct super_block *sb)
 	if (sb->s_user_ns != &init_user_ns)
 		sb->s_iflags |= SB_I_UNTRUSTED_MOUNTER;
 	sb->s_flags &= ~(SB_NOSEC | SB_I_VERSION);
+	sb->s_iflags |= SB_I_XATTR;
 }
 
 static int fuse_fill_super_submount(struct super_block *sb,
@@ -1443,6 +1445,8 @@ static int fuse_fill_super_submount(struct super_block *sb,
 	sb->s_bdi = bdi_get(parent_sb->s_bdi);
 
 	sb->s_xattr = parent_sb->s_xattr;
+	if (sb->s_xattr)
+		sb->s_iflags |= SB_I_XATTR;
 	sb->s_time_gran = parent_sb->s_time_gran;
 	sb->s_blocksize = parent_sb->s_blocksize;
 	sb->s_blocksize_bits = parent_sb->s_blocksize_bits;
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index c0cf1d2d0ef5..71c4fa1cdc1c 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -506,6 +506,7 @@ static int init_sb(struct gfs2_sbd *sdp, int silent)
 	default:
 		BUG();
 	}
+	sb->s_iflags |= SB_I_XATTR;
 
 	/* Set up the buffer cache and SB for real */
 	if (sdp->sd_sb.sb_bsize < bdev_logical_block_size(sb->s_bdev)) {
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 6764afa98a6f..6220ce2af0d0 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -401,6 +401,7 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op = &hfs_super_operations;
 	sb->s_xattr = hfs_xattr_handlers;
 	sb->s_flags |= SB_NODIRATIME;
+	sb->s_iflags |= SB_I_XATTR;
 	mutex_init(&sbi->bitmap_lock);
 
 	res = hfs_mdb_get(sb);
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 122ed89ebf9f..c5dd14057a78 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -490,6 +490,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
 	}
 	sb->s_xattr = hfsplus_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 
 	inode = hfsplus_iget(sb, HFSPLUS_ALLOC_CNID);
 	if (IS_ERR(inode)) {
diff --git a/fs/inode.c b/fs/inode.c
index f453eb58fd03..51824bde0b35 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -169,7 +169,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	inode->i_ino = 0;
 	inode->__i_nlink = 1;
 	inode->i_opflags = 0;
-	if (sb->s_xattr)
+	if (sb->s_iflags & SB_I_XATTR)
 		inode->i_opflags |= IOP_XATTR;
 	i_uid_write(inode, 0);
 	i_gid_write(inode, 0);
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 7ea37f49f1e1..1ae1ee573280 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -285,6 +285,7 @@ static int jffs2_fill_super(struct super_block *sb, struct fs_context *fc)
 #ifdef CONFIG_JFFS2_FS_POSIX_ACL
 	sb->s_flags |= SB_POSIXACL;
 #endif
+	sb->s_iflags |= SB_I_XATTR;
 	return jffs2_do_fill_super(sb, fc);
 }
 
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index d2f82cb7db1b..b78042d3f3c0 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -532,6 +532,7 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op = &jfs_super_operations;
 	sb->s_export_op = &jfs_export_operations;
 	sb->s_xattr = jfs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 #ifdef CONFIG_QUOTA
 	sb->dq_op = &dquot_operations;
 	sb->s_qcop = &jfs_quotactl_ops;
diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index e08e8d999807..cd3391419692 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -251,6 +251,7 @@ static int kernfs_fill_super(struct super_block *sb, struct kernfs_fs_context *k
 	sb->s_magic = kfc->magic;
 	sb->s_op = &kernfs_sops;
 	sb->s_xattr = kernfs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	if (info->root->flags & KERNFS_ROOT_SUPPORT_EXPORTOP)
 		sb->s_export_op = &kernfs_export_ops;
 	sb->s_time_gran = 1;
diff --git a/fs/libfs.c b/fs/libfs.c
index aada4e7c8713..bd219120f248 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -328,6 +328,8 @@ static int pseudo_fs_fill_super(struct super_block *s, struct fs_context *fc)
 	s->s_magic = ctx->magic;
 	s->s_op = ctx->ops ?: &simple_super_operations;
 	s->s_xattr = ctx->xattr;
+	if (s->s_xattr)
+		s->s_iflags |= SB_I_XATTR;
 	s->s_time_gran = 1;
 	root = new_inode(s);
 	if (!root)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 05ae23657527..23c746745129 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1077,6 +1077,9 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
 		break;
 	}
 
+	if (sb->s_xattr || (sb->s_flags & SB_POSIXACL))
+		sb->s_iflags |= SB_I_XATTR;
+
 	sb->s_magic = NFS_SUPER_MAGIC;
 
 	/* We probably want something more informative here */
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index ef4ea3f21905..93e66112fac5 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -950,6 +950,7 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	sb->s_export_op = &ntfs_export_ops;
 	sb->s_time_gran = NTFS_TIME_GRAN; // 100 nsec
 	sb->s_xattr = ntfs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	sb->s_d_op = sbi->options->nocase ? &ntfs_dentry_ops : NULL;
 
 	sbi->options->nls = ntfs_load_nls(sbi->options->nls_name);
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 0b0e6a132101..514f5bcc9722 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2020,6 +2020,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	sb->dq_op = &ocfs2_quota_operations;
 	sb->s_quota_types = QTYPE_MASK_USR | QTYPE_MASK_GRP;
 	sb->s_xattr = ocfs2_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	sb->s_time_gran = 1;
 	sb->s_flags |= SB_NOATIME;
 	/* this is needed to support O_LARGEFILE */
diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
index 5254256a224d..ec3e2d4ddb92 100644
--- a/fs/orangefs/super.c
+++ b/fs/orangefs/super.c
@@ -433,6 +433,7 @@ static int orangefs_fill_sb(struct super_block *sb,
 
 	/* Hang the xattr handlers off the superblock */
 	sb->s_xattr = orangefs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	sb->s_magic = ORANGEFS_SUPER_MAGIC;
 	sb->s_op = &orangefs_s_ops;
 	sb->s_d_op = &orangefs_dentry_operations;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 85b891152a2c..1002c8f332b9 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -2060,7 +2060,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		ovl_trusted_xattr_handlers;
 	sb->s_fs_info = ofs;
 	sb->s_flags |= SB_POSIXACL;
-	sb->s_iflags |= SB_I_SKIP_SYNC;
+	sb->s_iflags |= SB_I_SKIP_SYNC | SB_I_XATTR;
 
 	err = -ENOMEM;
 	root_dentry = ovl_get_root(sb, upperpath.dentry, oe);
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 929acce6e731..1655b384e0b4 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -2041,6 +2041,7 @@ static int reiserfs_fill_super(struct super_block *s, void *data, int silent)
 		goto error_unlocked;
 
 	s->s_xattr = reiserfs_xattr_handlers;
+	s->s_iflags |= SB_I_XATTR;
 
 	if (bdev_read_only(s->s_bdev) && !sb_rdonly(s)) {
 		SWARN(silent, s, "clm-7000",
diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index e090fae48e68..7ab4fbc1f663 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -338,6 +338,7 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	/* Handle xattrs */
 	sb->s_xattr = squashfs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	xattr_id_table_start = le64_to_cpu(sblk->xattr_id_table_start);
 	if (xattr_id_table_start == SQUASHFS_INVALID_BLK) {
 		next_table = msblk->bytes_used;
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index d0c9a09988bc..be9e96cf9d5a 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2215,6 +2215,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 		sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE;
 	sb->s_op = &ubifs_super_operations;
 	sb->s_xattr = ubifs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	fscrypt_set_ops(sb, &ubifs_crypt_operations);
 
 	mutex_lock(&c->umount_mutex);
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 0c4b73e9b29d..5a94d3eeab8a 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1461,6 +1461,7 @@ xfs_fs_fill_super(
 
 	sb_min_blocksize(sb, BBSIZE);
 	sb->s_xattr = xfs_xattr_handlers;
+	sb->s_iflags |= SB_I_XATTR;
 	sb->s_export_op = &xfs_export_operations;
 #ifdef CONFIG_XFS_QUOTA
 	sb->s_qcop = &xfs_quotactl_operations;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 066555ad1bf8..700be4b969ac 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1449,6 +1449,8 @@ extern int send_sigurg(struct fown_struct *fown);
 #define SB_I_TS_EXPIRY_WARNED 0x00000400 /* warned about timestamp range expiry */
 #define SB_I_RETIRED	0x00000800	/* superblock shouldn't be reused */
 
+#define SB_I_XATTR	0x00001000	/* superblock supports xattrs */
+
 /* Possible states of 'frozen' field */
 enum {
 	SB_UNFROZEN = 0,		/* FS is unfrozen */
diff --git a/mm/shmem.c b/mm/shmem.c
index c301487be5fb..f9604b64624e 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3817,6 +3817,8 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
 #ifdef CONFIG_TMPFS_POSIX_ACL
 	sb->s_flags |= SB_POSIXACL;
 #endif
+	if (sb->s_xattr || (sb->s_flags & SB_POSIXACL))
+		sb->s_iflags |= SB_I_XATTR;
 	uuid_gen(&sb->s_uuid);
 
 	inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0, VM_NORESERVE);
Christoph Hellwig Jan. 30, 2023, 9:16 a.m. UTC | #2
On Mon, Jan 30, 2023 at 10:10:52AM +0100, Christian Brauner wrote:
> However, a few filesystems still rely on the ->list() method of the
> generix POSIX ACL xattr handlers in their ->listxattr() inode operation.
> This is a very limited set of filesystems. For most of them there is no
> dependence on the generic POSIX ACL xattr handler in any way.
> 
> In addition, 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.
> 
> With the incoming removal of the legacy POSIX ACL handlers it is at
> least possible for a filesystem to only implement POSIX ACLs but no
> other xattrs. If that were to happen we would miss to raise IOP_XATTR
> because sb->s_xattr would be NULL. While there currently is no such
> filesystem we should still make sure that this just works should it ever
> happen in the future.

Now the real questions is: do we care?  Once Posix ACLs use an
entirely separate path, nothing should rely on IOP_XATTR for them.
So instead I think we're better off auditing all users of IOP_XATTR
and making sure that nothing relies on them for ACLs, as we've very
much split the VFS concept of ACLs from that from xattrs otherwise.
Christian Brauner Jan. 30, 2023, 10:23 a.m. UTC | #3
On Mon, Jan 30, 2023 at 10:16:15AM +0100, Christoph Hellwig wrote:
> On Mon, Jan 30, 2023 at 10:10:52AM +0100, Christian Brauner wrote:
> > However, a few filesystems still rely on the ->list() method of the
> > generix POSIX ACL xattr handlers in their ->listxattr() inode operation.
> > This is a very limited set of filesystems. For most of them there is no
> > dependence on the generic POSIX ACL xattr handler in any way.
> > 
> > In addition, 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.
> > 
> > With the incoming removal of the legacy POSIX ACL handlers it is at
> > least possible for a filesystem to only implement POSIX ACLs but no
> > other xattrs. If that were to happen we would miss to raise IOP_XATTR
> > because sb->s_xattr would be NULL. While there currently is no such
> > filesystem we should still make sure that this just works should it ever
> > happen in the future.
> 
> Now the real questions is: do we care?  Once Posix ACLs use an
> entirely separate path, nothing should rely on IOP_XATTR for them.
> So instead I think we're better off auditing all users of IOP_XATTR
> and making sure that nothing relies on them for ACLs, as we've very
> much split the VFS concept of ACLs from that from xattrs otherwise.

I had a patch like that but some filesystems create inodes that
explicitly remove IOP_XATTR to prevent any xattrs from being set on
specific inodes. I remember this for at least reiserfs and btrfs.

So we would probably need IOP_NOACL that can be raised by a filesystem
to explicitly opt out of them for specific inodes. That's probably fine
and avoids having to introduce something like SB_I_XATTR.