[RFC PATCH 0/4] fs: Add support for Windows file attributes

[RFC PATCH 0/4] fs: Add support for Windows file attributes

[Pali Rohár]

This is RFC patch series, first attempt to implement support for
additional file attributes via FS_IOC_FSGETXATTR and FS_IOC_FSSETXATTR
ioctl APIs, which are used by Windows applications and Windows
filesystems. It is not final, nor precise implementation, it is mean to
show how the API can look like and how it can be used. Please let me
know what do you think about it, if it is the right direction, or how to
move forward.

This RFC patch series contains API definition, VFS integration and
implementation only for SMB client / cifs.ko filesystem. Note that
cifs.ko does not set S_IMMUTABLE flag on local inode because
enforcement of immutable flag has to be done on server.

Design of this API comes from slightly longer discussion:
https://lore.kernel.org/linux-fsdevel/20241227121508.nofy6bho66pc5ry5@pali/t/#u

And conclusion was to abandon any idea of xattr APIs and uses
exclusively only the FS_IOC_FSGETXATTR and FS_IOC_FSSETXATTR ioctls.

There is no statx API extension yet.

Simple test client attrs which uses this new API is bellow.

$ cat attrs.c
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <linux/fs.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <stdio.h>
#include <errno.h>

#define FS_XFLAG_IMMUTABLEUSER	0x00000004
#define FS_XFLAG_COMPRESSED	0x00020000
#define FS_XFLAG_ENCRYPTED	0x00040000
#define FS_XFLAG_CHECKSUMS	0x00080000
#define FS_XFLAG_HASEXTFIELDS	0x40000000

#define FS_XFLAG2_HIDDEN	0x0001
#define FS_XFLAG2_SYSTEM	0x0002
#define FS_XFLAG2_ARCHIVE	0x0004
#define FS_XFLAG2_TEMPORARY	0x0008
#define FS_XFLAG2_NOTINDEXED	0x0010
#define FS_XFLAG2_NOSCRUBDATA	0x0020
#define FS_XFLAG2_OFFLINE	0x0040
#define FS_XFLAG2_PINNED	0x0080
#define FS_XFLAG2_UNPINNED	0x0100

int main(int argc, char *argv[]) {
	struct fsxattr fsxattr;
	int bit, is2, set;
	int fd;

	if (argc != 2 && argc != 3) {
		printf("Usage %s path [+|-attr]\n", argv[0]);
		return 1;
	}

	if (argc == 3) {
		if (argv[2][0] != '-' && argv[2][0] != '+') {
			printf("Attr must start with + or -\n");
			return 1;
		}
		set = argv[2][0] == '+';
		is2 = 1;
		if (strcmp(&argv[2][1], "readonly") == 0)
			bit = FS_XFLAG_IMMUTABLEUSER, is2 = 0;
		else if (strcmp(&argv[2][1], "hidden") == 0)
			bit = FS_XFLAG2_HIDDEN;
		else if (strcmp(&argv[2][1], "system") == 0)
			bit = FS_XFLAG2_SYSTEM;
		else if (strcmp(&argv[2][1], "archive") == 0)
			bit = FS_XFLAG2_ARCHIVE;
		else if (strcmp(&argv[2][1], "temporary") == 0)
			bit = FS_XFLAG2_TEMPORARY;
		else if (strcmp(&argv[2][1], "compressed") == 0)
			bit = FS_XFLAG_COMPRESSED, is2 = 0;
		else if (strcmp(&argv[2][1], "offline") == 0)
			bit = FS_XFLAG2_OFFLINE;
		else if (strcmp(&argv[2][1], "notindexed") == 0)
			bit = FS_XFLAG2_NOTINDEXED;
		else if (strcmp(&argv[2][1], "encrypted") == 0)
			bit = FS_XFLAG_ENCRYPTED, is2 = 0;
		else if (strcmp(&argv[2][1], "integrity") == 0)
			bit = FS_XFLAG_CHECKSUMS, is2 = 0;
		else if (strcmp(&argv[2][1], "noscrubdata") == 0)
			bit = FS_XFLAG2_NOSCRUBDATA;
		else if (strcmp(&argv[2][1], "pinned") == 0)
			bit = FS_XFLAG2_PINNED;
		else if (strcmp(&argv[2][1], "unpinned") == 0)
			bit = FS_XFLAG2_UNPINNED;
		else {
			printf("Unknown attr '%s'\n", &argv[2][1]);
			return 1;
		}
	}

	fd = open(argv[1], O_RDONLY | O_NONBLOCK);
	if (fd < 0) {
		printf("Cannot open '%s': %s\n", argv[1], strerror(errno));
		return 1;
	}

	if (ioctl(fd, FS_IOC_FSGETXATTR, &fsxattr) != 0) {
		printf("FS_IOC_FSGETXATTR for '%s' failed: %s\n", argv[1], strerror(errno));
		return 1;
	}

	if (!(fsxattr.fsx_xflags & FS_XFLAG_HASEXTFIELDS)) {
		printf("FS_XFLAG_HASEXTFIELDS is not supported\n");
		return 1;
	}

	if (argc == 2) {
		printf("readonly=%d\n", !!(fsxattr.fsx_xflags & FS_XFLAG_IMMUTABLEUSER));
		printf("hidden=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_HIDDEN));
		printf("system=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_SYSTEM));
		printf("archive=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_ARCHIVE));
		printf("temporary=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_TEMPORARY));
		printf("compressed=%d\n", !!(fsxattr.fsx_xflags & FS_XFLAG_COMPRESSED));
		printf("offline=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_OFFLINE));
		printf("notindexed=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_NOTINDEXED));
		printf("encrypted=%d\n", !!(fsxattr.fsx_xflags & FS_XFLAG_ENCRYPTED));
		printf("integrity=%d\n", !!(fsxattr.fsx_xflags & FS_XFLAG_CHECKSUMS));
		printf("noscrubdata=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_NOSCRUBDATA));
		printf("pinned=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_PINNED));
		printf("unpinned=%d\n", !!(*(__u16 *)&fsxattr.fsx_pad[0] & FS_XFLAG2_UNPINNED));
		return 0;
	}

	if (is2) {
		if (set)
			*(__u16 *)&fsxattr.fsx_pad[0] |= bit; /* fsx2_xflags */
		else
			*(__u16 *)&fsxattr.fsx_pad[0] &= ~bit; /* fsx2_xflags */
		*(__u16 *)&fsxattr.fsx_pad[2] = bit; /* fsx_xflags2_mask */
		*(__u32 *)&fsxattr.fsx_pad[4] = 0; /* fsx_xflags_mask */
	} else {
		if (set)
			fsxattr.fsx_xflags |= bit;
		else
			fsxattr.fsx_xflags &= ~bit;
		*(__u32 *)&fsxattr.fsx_pad[4] = bit; /* fsx_xflags_mask */
		*(__u16 *)&fsxattr.fsx_pad[2] = 0; /* fsx_xflags2_mask */
	}

	if (ioctl(fd, FS_IOC_FSSETXATTR, &fsxattr) != 0) {
		printf("FS_IOC_FSSETXATTR for '%s' failed: %s\n", argv[1], strerror(errno));
		return 1;
	}

	return 0;
}


Pali Rohár (4):
  fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for
    FS_IOC_FS[GS]ETXATTR API
  fs: Extend FS_IOC_FS[GS]ETXATTR API for Windows attributes
  fs: Implement support for fsx_xflags_mask, fsx_xflags2 and
    fsx_xflags2_mask into vfs
  cifs: Implement FS_IOC_FS[GS]ETXATTR API for Windows attributes

 fs/btrfs/ioctl.c          |   9 +-
 fs/efivarfs/inode.c       |   3 +-
 fs/ext2/ioctl.c           |   2 +-
 fs/ext4/ioctl.c           |   2 +-
 fs/f2fs/file.c            |   2 +-
 fs/fuse/ioctl.c           |  13 ++-
 fs/gfs2/file.c            |  14 ++-
 fs/hfsplus/inode.c        |   3 +-
 fs/ioctl.c                |  73 +++++++++++++--
 fs/jfs/ioctl.c            |  14 ++-
 fs/nilfs2/ioctl.c         |   2 +-
 fs/ntfs3/file.c           |   3 +-
 fs/ocfs2/ioctl.c          |   2 +-
 fs/orangefs/inode.c       |   2 +-
 fs/smb/client/cifsfs.c    |   4 +
 fs/smb/client/cifsfs.h    |   2 +
 fs/smb/client/cifsglob.h  |   4 +-
 fs/smb/client/cifsproto.h |   2 +-
 fs/smb/client/cifssmb.c   |   4 +-
 fs/smb/client/inode.c     | 181 ++++++++++++++++++++++++++++++++++++++
 fs/smb/client/ioctl.c     |   8 +-
 fs/smb/client/smb1ops.c   |   4 +-
 fs/smb/client/smb2ops.c   |   8 +-
 fs/smb/client/smb2pdu.c   |   4 +-
 fs/smb/client/smb2proto.h |   2 +-
 fs/smb/common/smb2pdu.h   |   2 +
 fs/ubifs/ioctl.c          |   3 +-
 fs/xfs/xfs_ioctl.c        |   5 +-
 include/linux/fileattr.h  |  15 ++--
 include/uapi/linux/fs.h   |  34 ++++++-
 mm/shmem.c                |   2 +-
 31 files changed, 380 insertions(+), 48 deletions(-)

-- 
2.20.1

[RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Pali Rohár]

This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/ioctl.c               | 8 ++++++++
 include/linux/fileattr.h | 4 ++--
 include/uapi/linux/fs.h  | 2 ++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 638a36be31c1..9f3609b50779 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -480,6 +480,10 @@ void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
 		fa->flags |= FS_DAX_FL;
 	if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
 		fa->flags |= FS_PROJINHERIT_FL;
+	if (fa->fsx_xflags & FS_XFLAG_COMPRESSED)
+		fa->flags |= FS_COMPR_FL;
+	if (fa->fsx_xflags & FS_XFLAG_ENCRYPTED)
+		fa->flags |= FS_ENCRYPT_FL;
 }
 EXPORT_SYMBOL(fileattr_fill_xflags);
 
@@ -496,6 +500,8 @@ void fileattr_fill_flags(struct fileattr *fa, u32 flags)
 	memset(fa, 0, sizeof(*fa));
 	fa->flags_valid = true;
 	fa->flags = flags;
+	if (fa->flags & FS_COMPR_FL)
+		fa->fsx_xflags |= FS_XFLAG_COMPRESSED;
 	if (fa->flags & FS_SYNC_FL)
 		fa->fsx_xflags |= FS_XFLAG_SYNC;
 	if (fa->flags & FS_IMMUTABLE_FL)
@@ -506,6 +512,8 @@ void fileattr_fill_flags(struct fileattr *fa, u32 flags)
 		fa->fsx_xflags |= FS_XFLAG_NODUMP;
 	if (fa->flags & FS_NOATIME_FL)
 		fa->fsx_xflags |= FS_XFLAG_NOATIME;
+	if (fa->flags & FS_ENCRYPT_FL)
+		fa->fsx_xflags |= FS_XFLAG_ENCRYPTED;
 	if (fa->flags & FS_DAX_FL)
 		fa->fsx_xflags |= FS_XFLAG_DAX;
 	if (fa->flags & FS_PROJINHERIT_FL)
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 47c05a9851d0..c297e6151703 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -7,12 +7,12 @@
 #define FS_COMMON_FL \
 	(FS_SYNC_FL | FS_IMMUTABLE_FL | FS_APPEND_FL | \
 	 FS_NODUMP_FL |	FS_NOATIME_FL | FS_DAX_FL | \
-	 FS_PROJINHERIT_FL)
+	 FS_PROJINHERIT_FL | FS_COMPR_FL | FS_ENCRYPT_FL)
 
 #define FS_XFLAG_COMMON \
 	(FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND | \
 	 FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
-	 FS_XFLAG_PROJINHERIT)
+	 FS_XFLAG_PROJINHERIT | FS_XFLAG_COMPRESSED | FS_XFLAG_ENCRYPTED)
 
 /*
  * Merged interface for miscellaneous file attributes.  'flags' originates from
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 2bbe00cf1248..367bc5289c47 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -167,6 +167,8 @@ struct fsxattr {
 #define FS_XFLAG_FILESTREAM	0x00004000	/* use filestream allocator */
 #define FS_XFLAG_DAX		0x00008000	/* use DAX for IO */
 #define FS_XFLAG_COWEXTSIZE	0x00010000	/* CoW extent size allocator hint */
+#define FS_XFLAG_COMPRESSED	0x00020000	/* compressed file */
+#define FS_XFLAG_ENCRYPTED	0x00040000	/* encrypted file */
 #define FS_XFLAG_HASATTR	0x80000000	/* no DIFLAG for this	*/
 
 /* the read-only stuff doesn't really belong here, but any other place is
-- 
2.20.1

[RFC PATCH 2/4] fs: Extend FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Pali Rohár]

struct fsxattr has 8 reserved padding bytes. Use these bytes for defining
new fields fsx_xflags2, fsx_xflags2_mask and fsx_xflags_mask in backward
compatible manner. If the new FS_XFLAG_HASEXTFIELDS flag in fsx_xflags is
not set then these new fields are treated as not present, like before this
change.

New field fsx_xflags_mask for SET operation specifies which flags in
fsx_xflags are going to be changed. This would allow userspace application
to change just subset of all flags. For GET operation this field specifies
which FS_XFLAG_* flags are supported by the file.

New field fsx_xflags2 specify new flags FS_XFLAG2_* which defines some of
Windows FILE_ATTRIBUTE_* attributes, which are mostly not going to be
interpreted or used by the kernel, and are mostly going to be used by
userspace. Field fsx_xflags2_mask then specify mask for them.

This change defines just API without filesystem support for them. These
attributes can be implemented later for Windows filesystems like FAT, NTFS,
exFAT, UDF, SMB, NFS4 which all native storage for those attributes (or at
least some subset of them).

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 include/uapi/linux/fs.h | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 367bc5289c47..93e947d6e604 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -145,15 +145,26 @@ struct fsxattr {
 	__u32		fsx_nextents;	/* nextents field value (get)	*/
 	__u32		fsx_projid;	/* project identifier (get/set) */
 	__u32		fsx_cowextsize;	/* CoW extsize field value (get/set)*/
-	unsigned char	fsx_pad[8];
+	__u16		fsx_xflags2;	/* xflags2 field value (get/set)*/
+	__u16		fsx_xflags2_mask;/*mask for xflags2 (get/set)*/
+	__u32		fsx_xflags_mask;/* mask for xflags (get/set)*/
+	/*
+	 * For FS_IOC_FSSETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
+	 * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags from fsx_xflags
+	 * and fsx_xflags2 fields are going to be changed.
+	 *
+	 * For FS_IOC_FSGETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
+	 * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags are supported.
+	 */
 };
 
 /*
- * Flags for the fsx_xflags field
+ * Flags for the fsx_xflags and fsx_xflags_mask fields
  */
 #define FS_XFLAG_REALTIME	0x00000001	/* data in realtime volume */
 #define FS_XFLAG_PREALLOC	0x00000002	/* preallocated file extents */
-#define FS_XFLAG_IMMUTABLE	0x00000008	/* file cannot be modified */
+#define FS_XFLAG_IMMUTABLEUSER	0x00000004	/* file cannot be modified, changing this bit does not require CAP_LINUX_IMMUTABLE, equivalent of Windows FILE_ATTRIBUTE_READONLY */
+#define FS_XFLAG_IMMUTABLE	0x00000008	/* file cannot be modified, changing this bit requires CAP_LINUX_IMMUTABLE */
 #define FS_XFLAG_APPEND		0x00000010	/* all writes append */
 #define FS_XFLAG_SYNC		0x00000020	/* all writes synchronous */
 #define FS_XFLAG_NOATIME	0x00000040	/* do not update access time */
@@ -167,10 +178,25 @@ struct fsxattr {
 #define FS_XFLAG_FILESTREAM	0x00004000	/* use filestream allocator */
 #define FS_XFLAG_DAX		0x00008000	/* use DAX for IO */
 #define FS_XFLAG_COWEXTSIZE	0x00010000	/* CoW extent size allocator hint */
-#define FS_XFLAG_COMPRESSED	0x00020000	/* compressed file */
-#define FS_XFLAG_ENCRYPTED	0x00040000	/* encrypted file */
+#define FS_XFLAG_COMPRESSED	0x00020000	/* compressed file, equivalent of Windows FILE_ATTRIBUTE_COMPRESSED */
+#define FS_XFLAG_ENCRYPTED	0x00040000	/* encrypted file, equivalent of Windows FILE_ATTRIBUTE_ENCRYPTED */
+#define FS_XFLAG_CHECKSUMS	0x00080000	/* checksum for data and metadata, equivalent of Windows FILE_ATTRIBUTE_INTEGRITY_STREAM */
+#define FS_XFLAG_HASEXTFIELDS	0x40000000	/* fields fsx_xflags_mask, fsx_xflags2 and fsx_xflags2_mask are present */
 #define FS_XFLAG_HASATTR	0x80000000	/* no DIFLAG for this	*/
 
+/*
+ * Flags for the fsx_xflags2 and fsx_xflags2_mask fields
+ */
+#define FS_XFLAG2_HIDDEN	0x0001	/* inode is hidden, equivalent of Widows FILE_ATTRIBUTE_HIDDEN */
+#define FS_XFLAG2_SYSTEM	0x0002	/* inode is part of operating system, equivalent of Windows FILE_ATTRIBUTE_SYSTEM */
+#define FS_XFLAG2_ARCHIVE	0x0004	/* inode was not archived yet, equivalent of Windows FILE_ATTRIBUTE_ARCHIVE */
+#define FS_XFLAG2_TEMPORARY	0x0008	/* inode content does not have to preserved across reboots, equivalent of Windows FILE_ATTRIBUTE_TEMPORARY */
+#define FS_XFLAG2_NOTINDEXED	0x0010	/* inode will not be indexed by content indexing service, equivalent of Windows FILE_ATTRIBUTE_NOT_CONTENT_INDEXED */
+#define FS_XFLAG2_NOSCRUBDATA	0x0020	/* file inode will not be checked by scrubber (proactive background data integrity scanner), for directory inode it means that newly created child would have this flag set, equivalent of Windows FILE_ATTRIBUTE_NO_SCRUB_DATA */
+#define FS_XFLAG2_OFFLINE	0x0040	/* inode is marked as HSM offline, equivalent of Windows FILE_ATTRIBUTE_OFFLINE */
+#define FS_XFLAG2_PINNED	0x0080	/* inode data content must be always stored in local HSM storage, equivalent of Windows FILE_ATTRIBUTE_PINNED */
+#define FS_XFLAG2_UNPINNED	0x0100	/* inode data content can be removed from local HSM storage, equivalent of Windows FILE_ATTRIBUTE_UNPINNED */
+
 /* the read-only stuff doesn't really belong here, but any other place is
    probably as bad and I don't want to create yet another include file. */
 
-- 
2.20.1

[RFC PATCH 3/4] fs: Implement support for fsx_xflags_mask, fsx_xflags2 and fsx_xflags2_mask into vfs

[Pali Rohár]

This change adds support for new struct fileattr fields fsx_xflags_mask,
fsx_xflags2 and fsx_xflags2_mask into FS_IOC_FSGETXATTR and
FS_IOC_FSSETXATTR ioctls.

All filesystem will start reporting values in new *_mask fields.
This change does not contain support for any new flag yet. This will be in
some followup changes.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/btrfs/ioctl.c         |  9 +++++-
 fs/efivarfs/inode.c      |  3 +-
 fs/ext2/ioctl.c          |  2 +-
 fs/ext4/ioctl.c          |  2 +-
 fs/f2fs/file.c           |  2 +-
 fs/fuse/ioctl.c          | 13 ++++++--
 fs/gfs2/file.c           | 14 ++++++++-
 fs/hfsplus/inode.c       |  3 +-
 fs/ioctl.c               | 65 +++++++++++++++++++++++++++++++++++-----
 fs/jfs/ioctl.c           | 14 ++++++++-
 fs/nilfs2/ioctl.c        |  2 +-
 fs/ntfs3/file.c          |  3 +-
 fs/ocfs2/ioctl.c         |  2 +-
 fs/orangefs/inode.c      |  2 +-
 fs/ubifs/ioctl.c         |  3 +-
 fs/xfs/xfs_ioctl.c       |  5 +++-
 include/linux/fileattr.h | 11 +++++--
 mm/shmem.c               |  2 +-
 18 files changed, 130 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6c18bad53cd3..600f502ffb46 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -164,6 +164,13 @@ static unsigned int btrfs_inode_flags_to_fsflags(struct btrfs_inode *binode)
 	return iflags;
 }
 
+static inline unsigned int btrfs_supported_fsflags(void)
+{
+	return FS_SYNC_FL | FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NODUMP_FL |
+	       FS_NOATIME_FL | FS_DIRSYNC_FL | FS_NOCOW_FL | FS_VERITY_FL |
+	       FS_NOCOMP_FL | FS_COMPR_FL;
+}
+
 /*
  * Update inode->i_flags based on the btrfs internal flags.
  */
@@ -250,7 +257,7 @@ int btrfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct btrfs_inode *binode = BTRFS_I(d_inode(dentry));
 
-	fileattr_fill_flags(fa, btrfs_inode_flags_to_fsflags(binode));
+	fileattr_fill_flags(fa, btrfs_inode_flags_to_fsflags(binode), btrfs_supported_fsflags());
 	return 0;
 }
 
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 98a7299a9ee9..a8a3962b7751 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -142,12 +142,13 @@ efivarfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	unsigned int i_flags;
 	unsigned int flags = 0;
+	unsigned int mask = FS_IMMUTABLE_FL;
 
 	i_flags = d_inode(dentry)->i_flags;
 	if (i_flags & S_IMMUTABLE)
 		flags |= FS_IMMUTABLE_FL;
 
-	fileattr_fill_flags(fa, flags);
+	fileattr_fill_flags(fa, flags, mask);
 
 	return 0;
 }
diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
index 44e04484e570..7a70f16c2824 100644
--- a/fs/ext2/ioctl.c
+++ b/fs/ext2/ioctl.c
@@ -22,7 +22,7 @@ int ext2_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct ext2_inode_info *ei = EXT2_I(d_inode(dentry));
 
-	fileattr_fill_flags(fa, ei->i_flags & EXT2_FL_USER_VISIBLE);
+	fileattr_fill_flags(fa, ei->i_flags & EXT2_FL_USER_VISIBLE, EXT2_FL_USER_VISIBLE);
 
 	return 0;
 }
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 7b9ce71c1c81..a199d0b74449 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -989,7 +989,7 @@ int ext4_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	if (S_ISREG(inode->i_mode))
 		flags &= ~FS_PROJINHERIT_FL;
 
-	fileattr_fill_flags(fa, flags);
+	fileattr_fill_flags(fa, flags, EXT4_FL_USER_VISIBLE);
 	if (ext4_has_feature_project(inode->i_sb))
 		fa->fsx_projid = from_kprojid(&init_user_ns, ei->i_projid);
 
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f92a9fba9991..03e1b31d1cbb 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3297,7 +3297,7 @@ int f2fs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	if (is_inode_flag_set(inode, FI_PIN_FILE))
 		fsflags |= FS_NOCOW_FL;
 
-	fileattr_fill_flags(fa, fsflags & F2FS_GETTABLE_FS_FL);
+	fileattr_fill_flags(fa, fsflags & F2FS_GETTABLE_FS_FL, F2FS_GETTABLE_FS_FL);
 
 	if (f2fs_sb_has_project_quota(F2FS_I_SB(inode)))
 		fa->fsx_projid = from_kprojid(&init_user_ns, fi->i_projid);
diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
index 2d9abf48828f..be0901d7c61e 100644
--- a/fs/fuse/ioctl.c
+++ b/fs/fuse/ioctl.c
@@ -520,14 +520,20 @@ int fuse_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 		if (err)
 			goto cleanup;
 
-		fileattr_fill_flags(fa, flags);
+		fileattr_fill_flags(fa, flags, ~0);
 	} else {
 		err = fuse_priv_ioctl(inode, ff, FS_IOC_FSGETXATTR,
 				      &xfa, sizeof(xfa));
 		if (err)
 			goto cleanup;
 
-		fileattr_fill_xflags(fa, xfa.fsx_xflags);
+		if (!(xfa.fsx_xflags & FS_XFLAG_HASEXTFIELDS)) {
+			xfa.fsx_xflags_mask = 0;
+			xfa.fsx_xflags2 = 0;
+			xfa.fsx_xflags2_mask = 0;
+		}
+
+		fileattr_fill_xflags(fa, xfa.fsx_xflags, xfa.fsx_xflags_mask, xfa.fsx_xflags2, xfa.fsx_xflags2_mask);
 		fa->fsx_extsize = xfa.fsx_extsize;
 		fa->fsx_nextents = xfa.fsx_nextents;
 		fa->fsx_projid = xfa.fsx_projid;
@@ -564,6 +570,9 @@ int fuse_fileattr_set(struct mnt_idmap *idmap,
 		xfa.fsx_nextents = fa->fsx_nextents;
 		xfa.fsx_projid = fa->fsx_projid;
 		xfa.fsx_cowextsize = fa->fsx_cowextsize;
+		xfa.fsx_xflags2 = fa->fsx_xflags2;
+		xfa.fsx_xflags2_mask = fa->fsx_xflags2_mask;
+		xfa.fsx_xflags_mask = fa->fsx_xflags_mask;
 
 		err = fuse_priv_ioctl(inode, ff, FS_IOC_FSSETXATTR,
 				      &xfa, sizeof(xfa));
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index c9bb3be21d2b..29d243ade026 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -139,6 +139,16 @@ static struct {
 	{FS_JOURNAL_DATA_FL, GFS2_DIF_JDATA | GFS2_DIF_INHERIT_JDATA},
 };
 
+static inline u32 gfs2_supported_fsflags(void)
+{
+	int i;
+	u32 fsflags = 0;
+
+	for (i = 0; i < ARRAY_SIZE(fsflag_gfs2flag); i++)
+		fsflags |= fsflag_gfs2flag[i].fsflag;
+	return fsflags;
+}
+
 static inline u32 gfs2_gfsflags_to_fsflags(struct inode *inode, u32 gfsflags)
 {
 	int i;
@@ -162,6 +172,7 @@ int gfs2_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	struct gfs2_holder gh;
 	int error;
 	u32 fsflags;
+	u32 fsmask;
 
 	if (d_is_special(dentry))
 		return -ENOTTY;
@@ -172,8 +183,9 @@ int gfs2_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 		goto out_uninit;
 
 	fsflags = gfs2_gfsflags_to_fsflags(inode, ip->i_diskflags);
+	fsmask = gfs2_supported_fsflags();
 
-	fileattr_fill_flags(fa, fsflags);
+	fileattr_fill_flags(fa, fsflags, fsmask);
 
 	gfs2_glock_dq(&gh);
 out_uninit:
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index f331e9574217..bb430a920f2b 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -659,6 +659,7 @@ int hfsplus_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	struct inode *inode = d_inode(dentry);
 	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	unsigned int flags = 0;
+	unsigned int mask = FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NODUMP_FL;
 
 	if (inode->i_flags & S_IMMUTABLE)
 		flags |= FS_IMMUTABLE_FL;
@@ -667,7 +668,7 @@ int hfsplus_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	if (hip->userflags & HFSPLUS_FLG_NODUMP)
 		flags |= FS_NODUMP_FL;
 
-	fileattr_fill_flags(fa, flags);
+	fileattr_fill_flags(fa, flags, mask);
 
 	return 0;
 }
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 9f3609b50779..ef4d7d3d417b 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -458,14 +458,19 @@ static int ioctl_file_dedupe_range(struct file *file,
  * @fa:		fileattr pointer
  * @xflags:	FS_XFLAG_* flags
  *
- * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags).  All
- * other fields are zeroed.
+ * Set ->fsx_xflags, ->fsx_xflags2, ->fsx->xflags_mask, ->fsx_xflags2_mask,
+ * ->fsx_valid and ->flags (translated xflags).  All other fields are zeroed.
  */
-void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
+void fileattr_fill_xflags(struct fileattr *fa, u32 xflags, u32 xflags_mask, u16 xflags2, u16 xflags2_mask)
 {
 	memset(fa, 0, sizeof(*fa));
 	fa->fsx_valid = true;
 	fa->fsx_xflags = xflags;
+	fa->fsx_xflags2 = xflags2;
+	fa->fsx_xflags_mask = xflags_mask;
+	fa->fsx_xflags2_mask = xflags2_mask;
+	if (fa->fsx_xflags2 != 0 || fa->fsx_xflags_mask != 0 || fa->fsx_xflags2_mask != 0)
+		fa->fsx_xflags |= FS_XFLAG_HASEXTFIELDS;
 	if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
 		fa->flags |= FS_IMMUTABLE_FL;
 	if (fa->fsx_xflags & FS_XFLAG_APPEND)
@@ -491,15 +496,20 @@ EXPORT_SYMBOL(fileattr_fill_xflags);
  * fileattr_fill_flags - initialize fileattr with flags
  * @fa:		fileattr pointer
  * @flags:	FS_*_FL flags
+ * @mask:	FS_*_FL flags mask
  *
- * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags).
+ * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags),
+ * fa->fsx_xflags_mask (translated flags mask).
  * All other fields are zeroed.
  */
-void fileattr_fill_flags(struct fileattr *fa, u32 flags)
+void fileattr_fill_flags(struct fileattr *fa, u32 flags, u32 mask)
 {
 	memset(fa, 0, sizeof(*fa));
 	fa->flags_valid = true;
 	fa->flags = flags;
+
+	fa->fsx_xflags |= FS_XFLAG_HASEXTFIELDS;
+
 	if (fa->flags & FS_COMPR_FL)
 		fa->fsx_xflags |= FS_XFLAG_COMPRESSED;
 	if (fa->flags & FS_SYNC_FL)
@@ -518,6 +528,25 @@ void fileattr_fill_flags(struct fileattr *fa, u32 flags)
 		fa->fsx_xflags |= FS_XFLAG_DAX;
 	if (fa->flags & FS_PROJINHERIT_FL)
 		fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
+
+	if (mask & FS_COMPR_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_COMPRESSED;
+	if (mask & FS_SYNC_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_SYNC;
+	if (mask & FS_IMMUTABLE_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_IMMUTABLE;
+	if (mask & FS_APPEND_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_APPEND;
+	if (mask & FS_NODUMP_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_NODUMP;
+	if (mask & FS_NOATIME_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_NOATIME;
+	if (mask & FS_ENCRYPT_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_ENCRYPTED;
+	if (mask & FS_DAX_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_DAX;
+	if (mask & FS_PROJINHERIT_FL)
+		fa->fsx_xflags_mask |= FS_XFLAG_PROJINHERIT;
 }
 EXPORT_SYMBOL(fileattr_fill_flags);
 
@@ -558,6 +587,11 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
 	xfa.fsx_nextents = fa->fsx_nextents;
 	xfa.fsx_projid = fa->fsx_projid;
 	xfa.fsx_cowextsize = fa->fsx_cowextsize;
+	if (xfa.fsx_xflags & FS_XFLAG_HASEXTFIELDS) {
+		xfa.fsx_xflags2 = fa->fsx_xflags2;
+		xfa.fsx_xflags2_mask = fa->fsx_xflags2_mask;
+		xfa.fsx_xflags_mask = fa->fsx_xflags_mask;
+	}
 
 	if (copy_to_user(ufa, &xfa, sizeof(xfa)))
 		return -EFAULT;
@@ -574,7 +608,13 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
 	if (copy_from_user(&xfa, ufa, sizeof(xfa)))
 		return -EFAULT;
 
-	fileattr_fill_xflags(fa, xfa.fsx_xflags);
+	if (!(xfa.fsx_xflags & FS_XFLAG_HASEXTFIELDS)) {
+		xfa.fsx_xflags_mask = 0;
+		xfa.fsx_xflags2 = 0;
+		xfa.fsx_xflags2_mask = 0;
+	}
+
+	fileattr_fill_xflags(fa, xfa.fsx_xflags, xfa.fsx_xflags_mask, xfa.fsx_xflags2, xfa.fsx_xflags2_mask);
 	fa->fsx_extsize = xfa.fsx_extsize;
 	fa->fsx_nextents = xfa.fsx_nextents;
 	fa->fsx_projid = xfa.fsx_projid;
@@ -692,11 +732,22 @@ int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
 		/* initialize missing bits from old_ma */
 		if (fa->flags_valid) {
 			fa->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
+			fa->fsx_xflags_mask = fa->fsx_xflags ^ old_ma.fsx_xflags;
 			fa->fsx_extsize = old_ma.fsx_extsize;
 			fa->fsx_nextents = old_ma.fsx_nextents;
 			fa->fsx_projid = old_ma.fsx_projid;
 			fa->fsx_cowextsize = old_ma.fsx_cowextsize;
+			fa->fsx_xflags2 = 0;
+			fa->fsx_xflags2_mask = 0;
 		} else {
+			if (fa->fsx_xflags & FS_XFLAG_HASEXTFIELDS) {
+				fa->fsx_xflags = (fa->fsx_xflags & fa->fsx_xflags_mask) | (old_ma.fsx_xflags & ~fa->fsx_xflags_mask);
+				fa->fsx_xflags2 = (fa->fsx_xflags2 & fa->fsx_xflags2_mask) | (old_ma.fsx_xflags2 & ~fa->fsx_xflags2_mask);
+			} else {
+				fa->fsx_xflags_mask = fa->fsx_xflags ^ old_ma.fsx_xflags;
+				fa->fsx_xflags2 = old_ma.fsx_xflags2;
+				fa->fsx_xflags2_mask = 0;
+			}
 			fa->flags |= old_ma.flags & ~FS_COMMON_FL;
 		}
 		err = fileattr_set_prepare(inode, &old_ma, fa);
@@ -732,7 +783,7 @@ static int ioctl_setflags(struct file *file, unsigned int __user *argp)
 	if (!err) {
 		err = mnt_want_write_file(file);
 		if (!err) {
-			fileattr_fill_flags(&fa, flags);
+			fileattr_fill_flags(&fa, flags, FS_COMMON_FL);
 			err = vfs_fileattr_set(idmap, dentry, &fa);
 			mnt_drop_write_file(file);
 		}
diff --git a/fs/jfs/ioctl.c b/fs/jfs/ioctl.c
index f7bd7e8f5be4..86184e32015c 100644
--- a/fs/jfs/ioctl.c
+++ b/fs/jfs/ioctl.c
@@ -39,6 +39,18 @@ static struct {
 	{0, 0},
 };
 
+static long jfs_supported_ext2_flags(void)
+{
+	int index=0;
+	long mapped=0;
+
+	while (jfs_map[index].jfs_flag) {
+		mapped |= jfs_map[index].ext2_flag;
+		index++;
+	}
+	return mapped;
+}
+
 static long jfs_map_ext2(unsigned long flags, int from)
 {
 	int index=0;
@@ -65,7 +77,7 @@ int jfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	if (d_is_special(dentry))
 		return -ENOTTY;
 
-	fileattr_fill_flags(fa, jfs_map_ext2(flags, 0));
+	fileattr_fill_flags(fa, jfs_map_ext2(flags, 0), jfs_supported_ext2_flags());
 
 	return 0;
 }
diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index a66d62a51f77..2f1d5d765dcb 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -122,7 +122,7 @@ int nilfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
 
-	fileattr_fill_flags(fa, NILFS_I(inode)->i_flags & FS_FL_USER_VISIBLE);
+	fileattr_fill_flags(fa, NILFS_I(inode)->i_flags & FS_FL_USER_VISIBLE, FS_FL_USER_VISIBLE);
 
 	return 0;
 }
diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
index 3f96a11804c9..a8f4d0b08d83 100644
--- a/fs/ntfs3/file.c
+++ b/fs/ntfs3/file.c
@@ -57,6 +57,7 @@ int ntfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	struct inode *inode = d_inode(dentry);
 	struct ntfs_inode *ni = ntfs_i(inode);
 	u32 flags = 0;
+	u32 mask = FS_IMMUTABLE_FL | FS_APPEND_FL | FS_COMPR_FL | FS_ENCRYPT_FL;
 
 	if (inode->i_flags & S_IMMUTABLE)
 		flags |= FS_IMMUTABLE_FL;
@@ -70,7 +71,7 @@ int ntfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	if (is_encrypted(ni))
 		flags |= FS_ENCRYPT_FL;
 
-	fileattr_fill_flags(fa, flags);
+	fileattr_fill_flags(fa, flags, mask);
 
 	return 0;
 }
diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
index 7ae96fb8807a..fa48d1b92aab 100644
--- a/fs/ocfs2/ioctl.c
+++ b/fs/ocfs2/ioctl.c
@@ -77,7 +77,7 @@ int ocfs2_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	flags = OCFS2_I(inode)->ip_attr;
 	ocfs2_inode_unlock(inode, 0);
 
-	fileattr_fill_flags(fa, flags & OCFS2_FL_VISIBLE);
+	fileattr_fill_flags(fa, flags & OCFS2_FL_VISIBLE, OCFS2_FL_VISIBLE);
 
 	return status;
 }
diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index aae6d2b8767d..36104a08d654 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -923,7 +923,7 @@ static int orangefs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 
 	gossip_debug(GOSSIP_FILE_DEBUG, "%s: flags=%u\n", __func__, (u32) val);
 
-	fileattr_fill_flags(fa, val);
+	fileattr_fill_flags(fa, val, FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NOATIME_FL);
 	return 0;
 }
 
diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
index 2c99349cf537..7d445c03a877 100644
--- a/fs/ubifs/ioctl.c
+++ b/fs/ubifs/ioctl.c
@@ -134,12 +134,13 @@ int ubifs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
 	int flags = ubifs2ioctl(ubifs_inode(inode)->flags);
+	int mask = ubifs2ioctl(~0);
 
 	if (d_is_special(dentry))
 		return -ENOTTY;
 
 	dbg_gen("get flags: %#x, i_flags %#x", flags, inode->i_flags);
-	fileattr_fill_flags(fa, flags);
+	fileattr_fill_flags(fa, flags, mask);
 
 	return 0;
 }
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index ed85322507dd..9c5e75568c94 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -448,8 +448,11 @@ xfs_fill_fsxattr(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
+	struct xfs_inode	ip_all_xflags = { .i_diflags = XFS_DIFLAG_ANY,
+						  .i_diflags2 = XFS_DIFLAG2_ANY,
+						  .i_forkoff = 1 };
 
-	fileattr_fill_xflags(fa, xfs_ip2xflags(ip));
+	fileattr_fill_xflags(fa, xfs_ip2xflags(ip), xfs_ip2xflags(&ip_all_xflags), 0, 0);
 
 	if (ip->i_diflags & XFS_DIFLAG_EXTSIZE) {
 		fa->fsx_extsize = XFS_FSB_TO_B(mp, ip->i_extsize);
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index c297e6151703..f2107598ed6b 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -28,6 +28,9 @@ struct fileattr {
 	u32	fsx_nextents;	/* nextents field value (get)	*/
 	u32	fsx_projid;	/* project identifier (get/set) */
 	u32	fsx_cowextsize;	/* CoW extsize field value (get/set)*/
+	u16	fsx_xflags2;	/* xflags2 field value (get/set)*/
+	u16	fsx_xflags2_mask;/*mask for xflags2 (get/set)*/
+	u32	fsx_xflags_mask;/* mask for xflags (get/set)*/
 	/* selectors: */
 	bool	flags_valid:1;
 	bool	fsx_valid:1;
@@ -35,8 +38,8 @@ struct fileattr {
 
 int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
 
-void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
-void fileattr_fill_flags(struct fileattr *fa, u32 flags);
+void fileattr_fill_xflags(struct fileattr *fa, u32 xflags, u32 xflags_mask, u16 xflags2, u16 xflags2_mask);
+void fileattr_fill_flags(struct fileattr *fa, u32 flags, u32 mask);
 
 /**
  * fileattr_has_fsx - check for extended flags/attributes
@@ -49,7 +52,9 @@ static inline bool fileattr_has_fsx(const struct fileattr *fa)
 {
 	return fa->fsx_valid &&
 		((fa->fsx_xflags & ~FS_XFLAG_COMMON) || fa->fsx_extsize != 0 ||
-		 fa->fsx_projid != 0 ||	fa->fsx_cowextsize != 0);
+		 fa->fsx_projid != 0 || fa->fsx_cowextsize != 0 ||
+		 fa->fsx_xflags2 != 0 || fa->fsx_xflags2_mask != 0 ||
+		 (fa->fsx_xflags_mask & ~FS_XFLAG_COMMON));
 }
 
 int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
diff --git a/mm/shmem.c b/mm/shmem.c
index 4ea6109a8043..b991f49ee638 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -4178,7 +4178,7 @@ static int shmem_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct shmem_inode_info *info = SHMEM_I(d_inode(dentry));
 
-	fileattr_fill_flags(fa, info->fsflags & SHMEM_FL_USER_VISIBLE);
+	fileattr_fill_flags(fa, info->fsflags & SHMEM_FL_USER_VISIBLE, SHMEM_FL_USER_VISIBLE);
 
 	return 0;
 }
-- 
2.20.1

[RFC PATCH 4/4] cifs: Implement FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Pali Rohár]

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/cifsfs.c    |   4 +
 fs/smb/client/cifsfs.h    |   2 +
 fs/smb/client/cifsglob.h  |   4 +-
 fs/smb/client/cifsproto.h |   2 +-
 fs/smb/client/cifssmb.c   |   4 +-
 fs/smb/client/inode.c     | 181 ++++++++++++++++++++++++++++++++++++++
 fs/smb/client/ioctl.c     |   8 +-
 fs/smb/client/smb1ops.c   |   4 +-
 fs/smb/client/smb2ops.c   |   8 +-
 fs/smb/client/smb2pdu.c   |   4 +-
 fs/smb/client/smb2proto.h |   2 +-
 fs/smb/common/smb2pdu.h   |   2 +
 12 files changed, 209 insertions(+), 16 deletions(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index ea31d693ea9f..b441675f9afd 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -1182,6 +1182,8 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.listxattr = cifs_listxattr,
 	.get_acl = cifs_get_acl,
 	.set_acl = cifs_set_acl,
+	.fileattr_get = cifs_fileattr_get,
+	.fileattr_set = cifs_fileattr_set,
 };
 
 const struct inode_operations cifs_file_inode_ops = {
@@ -1192,6 +1194,8 @@ const struct inode_operations cifs_file_inode_ops = {
 	.fiemap = cifs_fiemap,
 	.get_acl = cifs_get_acl,
 	.set_acl = cifs_set_acl,
+	.fileattr_get = cifs_fileattr_get,
+	.fileattr_set = cifs_fileattr_set,
 };
 
 const char *cifs_get_link(struct dentry *dentry, struct inode *inode,
diff --git a/fs/smb/client/cifsfs.h b/fs/smb/client/cifsfs.h
index 831fee962c4d..b1e6025e2cbc 100644
--- a/fs/smb/client/cifsfs.h
+++ b/fs/smb/client/cifsfs.h
@@ -77,6 +77,8 @@ extern int cifs_setattr(struct mnt_idmap *, struct dentry *,
 			struct iattr *);
 extern int cifs_fiemap(struct inode *, struct fiemap_extent_info *, u64 start,
 		       u64 len);
+extern int cifs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
+extern int cifs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry, struct fileattr *fa);
 
 extern const struct inode_operations cifs_file_inode_ops;
 extern const struct inode_operations cifs_symlink_inode_ops;
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index b764bfe916b4..233a0a13b0e2 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -426,7 +426,7 @@ struct smb_version_operations {
 	int (*set_file_info)(struct inode *, const char *, FILE_BASIC_INFO *,
 			     const unsigned int);
 	int (*set_compression)(const unsigned int, struct cifs_tcon *,
-			       struct cifsFileInfo *);
+			       struct cifsFileInfo *, bool);
 	/* check if we can send an echo or nor */
 	bool (*can_echo)(struct TCP_Server_Info *);
 	/* send echo request */
@@ -538,7 +538,7 @@ struct smb_version_operations {
 	int (*calc_signature)(struct smb_rqst *, struct TCP_Server_Info *,
 				bool allocate_crypto);
 	int (*set_integrity)(const unsigned int, struct cifs_tcon *tcon,
-			     struct cifsFileInfo *src_file);
+			     struct cifsFileInfo *src_file, bool enable);
 	int (*enum_snapshots)(const unsigned int xid, struct cifs_tcon *tcon,
 			     struct cifsFileInfo *src_file, void __user *);
 	int (*notify)(const unsigned int xid, struct file *pfile,
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 47ecc0884a74..f5f6be6f343e 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -506,7 +506,7 @@ extern struct inode *cifs_create_reparse_inode(struct cifs_open_info_data *data,
 					       struct kvec *reparse_iov,
 					       struct kvec *xattr_iov);
 extern int CIFSSMB_set_compression(const unsigned int xid,
-				   struct cifs_tcon *tcon, __u16 fid);
+				   struct cifs_tcon *tcon, __u16 fid, bool enable);
 extern int CIFS_open(const unsigned int xid, struct cifs_open_parms *oparms,
 		     int *oplock, FILE_ALL_INFO *buf);
 extern int SMBOldOpen(const unsigned int xid, struct cifs_tcon *tcon,
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 3dbff55b639d..643a55db3ca9 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3454,7 +3454,7 @@ struct inode *cifs_create_reparse_inode(struct cifs_open_info_data *data,
 
 int
 CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
-		    __u16 fid)
+		    __u16 fid, bool enable)
 {
 	int rc = 0;
 	int bytes_returned;
@@ -3467,7 +3467,7 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
 	if (rc)
 		return rc;
 
-	pSMB->compression_state = cpu_to_le16(COMPRESSION_FORMAT_DEFAULT);
+	pSMB->compression_state = cpu_to_le16(enable ? COMPRESSION_FORMAT_DEFAULT : COMPRESSION_FORMAT_NONE);
 
 	pSMB->TotalParameterCount = 0;
 	pSMB->TotalDataCount = cpu_to_le32(2);
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index dfad9284a87c..d07ebb99c262 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -13,6 +13,7 @@
 #include <linux/sched/signal.h>
 #include <linux/wait_bit.h>
 #include <linux/fiemap.h>
+#include <linux/fileattr.h>
 #include <asm/div64.h>
 #include "cifsfs.h"
 #include "cifspdu.h"
@@ -83,6 +84,7 @@ static void cifs_set_ops(struct inode *inode)
 		inode->i_op = &cifs_symlink_inode_ops;
 		break;
 	default:
+		inode->i_op = &cifs_file_inode_ops;
 		init_special_inode(inode, inode->i_mode, inode->i_rdev);
 		break;
 	}
@@ -3282,3 +3284,182 @@ cifs_setattr(struct mnt_idmap *idmap, struct dentry *direntry,
 	/* BB: add cifs_setattr_legacy for really old servers */
 	return rc;
 }
+
+int cifs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
+	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	struct inode *inode = d_inode(dentry);
+	u32 attrs = CIFS_I(inode)->cifsAttrs;
+	u32 fsattrs = le32_to_cpu(tcon->fsAttrInfo.Attributes);
+	u32 xflags = 0;
+	u32 xflags_mask = FS_XFLAG_IMMUTABLEUSER;
+	u16 xflags2 = 0;
+	u16 xflags2_mask = FS_XFLAG2_HIDDEN | FS_XFLAG2_SYSTEM | FS_XFLAG2_ARCHIVE |
+			   FS_XFLAG2_TEMPORARY | FS_XFLAG2_NOTINDEXED |
+			   FS_XFLAG2_NOSCRUBDATA | FS_XFLAG2_OFFLINE |
+			   FS_XFLAG2_PINNED | FS_XFLAG2_UNPINNED;
+
+	if (fsattrs & FILE_FILE_COMPRESSION)
+		xflags_mask |= FS_XFLAG_COMPRESSED;
+	if (fsattrs & FILE_SUPPORTS_ENCRYPTION)
+		xflags_mask |= FS_XFLAG_COMPRESSED;
+	if (fsattrs & FILE_SUPPORT_INTEGRITY_STREAMS)
+		xflags_mask |= FS_XFLAG_CHECKSUMS;
+
+	if (attrs & FILE_ATTRIBUTE_READONLY)
+		xflags |= FS_XFLAG_IMMUTABLEUSER;
+	if (attrs & FILE_ATTRIBUTE_HIDDEN)
+		xflags2 |= FS_XFLAG2_HIDDEN;
+	if (attrs & FILE_ATTRIBUTE_SYSTEM)
+		xflags2 |= FS_XFLAG2_SYSTEM;
+	if (attrs & FILE_ATTRIBUTE_ARCHIVE)
+		xflags2 |= FS_XFLAG2_ARCHIVE;
+	if (attrs & FILE_ATTRIBUTE_TEMPORARY)
+		xflags2 |= FS_XFLAG2_TEMPORARY;
+	if (attrs & FILE_ATTRIBUTE_COMPRESSED)
+		xflags |= FS_XFLAG_COMPRESSED;
+	if (attrs & FILE_ATTRIBUTE_OFFLINE)
+		xflags2 |= FS_XFLAG2_OFFLINE;
+	if (attrs & FILE_ATTRIBUTE_NOT_CONTENT_INDEXED)
+		xflags2 |= FS_XFLAG2_NOTINDEXED;
+	if (attrs & FILE_ATTRIBUTE_ENCRYPTED)
+		xflags |= FS_XFLAG_ENCRYPTED;
+	if (attrs & FILE_ATTRIBUTE_INTEGRITY_STREAM)
+		xflags |= FS_XFLAG_CHECKSUMS;
+	if (attrs & FILE_ATTRIBUTE_NO_SCRUB_DATA)
+		xflags2 |= FS_XFLAG2_NOSCRUBDATA;
+	if (attrs & FILE_ATTRIBUTE_PINNED)
+		xflags2 |= FS_XFLAG2_PINNED;
+	if (attrs & FILE_ATTRIBUTE_UNPINNED)
+		xflags2 |= FS_XFLAG2_UNPINNED;
+
+	fileattr_fill_xflags(fa, xflags, xflags_mask, xflags2, xflags2_mask);
+	return 0;
+}
+
+#define MODIFY_ATTRS_COND(attrs, xflags, xflag, attr) (attrs) ^= ((-(!!((xflags) & (xflag))) ^ (attrs)) & (attr))
+
+int cifs_fileattr_set(struct mnt_idmap *idmap,
+		      struct dentry *dentry, struct fileattr *fa)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
+	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	struct inode *inode = d_inode(dentry);
+	u32 attrs = CIFS_I(inode)->cifsAttrs;
+	struct cifsFileInfo open_file_tmp = {};
+	struct cifsFileInfo *open_file = NULL;
+	struct cifs_open_parms oparms;
+	FILE_BASIC_INFO info_buf = {};
+	bool do_close = false;
+	const char *full_path;
+	unsigned int xid;
+	__u32 oplock;
+	void *page;
+	int rc;
+
+	if ((fa->fsx_xflags_mask & ~(FS_XFLAG_IMMUTABLEUSER | FS_XFLAG_COMPRESSED |
+				 FS_XFLAG_ENCRYPTED | FS_XFLAG_CHECKSUMS)) ||
+	    (fa->fsx_xflags2_mask & ~(FS_XFLAG2_HIDDEN | FS_XFLAG2_SYSTEM | FS_XFLAG2_ARCHIVE |
+				  FS_XFLAG2_TEMPORARY | FS_XFLAG2_NOTINDEXED |
+				  FS_XFLAG2_NOSCRUBDATA | FS_XFLAG2_OFFLINE |
+				  FS_XFLAG2_PINNED | FS_XFLAG2_UNPINNED)) ||
+	    (fa->flags & ~FS_COMMON_FL))
+		return -EOPNOTSUPP;
+
+	if (fa->fsx_xflags_mask & FS_XFLAG_IMMUTABLEUSER)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags, FS_XFLAG_IMMUTABLEUSER, FILE_ATTRIBUTE_READONLY);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_HIDDEN)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_HIDDEN, FILE_ATTRIBUTE_HIDDEN);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_SYSTEM)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_SYSTEM, FILE_ATTRIBUTE_SYSTEM);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_ARCHIVE)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_ARCHIVE, FILE_ATTRIBUTE_ARCHIVE);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_TEMPORARY)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_TEMPORARY, FILE_ATTRIBUTE_TEMPORARY);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_NOTINDEXED)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_NOTINDEXED, FILE_ATTRIBUTE_NOT_CONTENT_INDEXED);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_NOSCRUBDATA)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_NOSCRUBDATA, FILE_ATTRIBUTE_NO_SCRUB_DATA);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_OFFLINE)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_OFFLINE, FILE_ATTRIBUTE_OFFLINE);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_PINNED)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_PINNED, FILE_ATTRIBUTE_PINNED);
+	if (fa->fsx_xflags2_mask & FS_XFLAG2_UNPINNED)
+		MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_UNPINNED, FILE_ATTRIBUTE_UNPINNED);
+
+	page = alloc_dentry_path();
+
+	full_path = build_path_from_dentry(dentry, page);
+	if (IS_ERR(full_path)) {
+		rc = PTR_ERR(full_path);
+		goto out_page;
+	}
+
+	xid = get_xid();
+
+	if (attrs != CIFS_I(inode)->cifsAttrs) {
+		info_buf.Attributes = cpu_to_le32(attrs);
+		if (tcon->ses->server->ops->set_file_info)
+			rc = tcon->ses->server->ops->set_file_info(inode, full_path, &info_buf, xid);
+		else
+			rc = -EOPNOTSUPP;
+		if (rc)
+			goto out_xid;
+		CIFS_I(inode)->cifsAttrs = attrs;
+	}
+
+	if (fa->fsx_xflags_mask & (FS_XFLAG_COMPRESSED | FS_XFLAG_ENCRYPTED | FS_XFLAG_CHECKSUMS)) {
+		open_file = find_writable_file(CIFS_I(inode), FIND_WR_FSUID_ONLY);
+		if (!open_file) {
+			oparms = CIFS_OPARMS(cifs_sb, tcon, full_path, FILE_WRITE_DATA, FILE_OPEN, 0, ACL_NO_MODE);
+			oparms.fid = &open_file_tmp.fid;
+			oplock = 0;
+			oparms.create_options = cifs_create_options(cifs_sb, 0);
+			rc = tcon->ses->server->ops->open(xid, &oparms, &oplock, NULL);
+			if (rc)
+				goto out_file;
+			do_close = true;
+			open_file = &open_file_tmp;
+		}
+	}
+
+	if (fa->fsx_xflags_mask & FS_XFLAG_COMPRESSED) {
+		if (tcon->ses->server->ops->set_compression)
+			rc = tcon->ses->server->ops->set_compression(xid, tcon, open_file, fa->fsx_xflags & FS_XFLAG_COMPRESSED);
+		else
+			rc = -EOPNOTSUPP;
+		if (rc)
+			goto out_file;
+		CIFS_I(inode)->cifsAttrs |= FILE_ATTRIBUTE_COMPRESSED;
+	}
+
+	if (fa->fsx_xflags_mask & FS_XFLAG_ENCRYPTED) {
+		/* TODO */
+		rc = -EOPNOTSUPP;
+		if (rc)
+			goto out_file;
+		CIFS_I(inode)->cifsAttrs |= FILE_ATTRIBUTE_ENCRYPTED;
+	}
+
+	if (fa->fsx_xflags_mask & FS_XFLAG_CHECKSUMS) {
+		if (tcon->ses->server->ops->set_integrity)
+			rc = tcon->ses->server->ops->set_integrity(xid, tcon, open_file, fa->fsx_xflags & FS_XFLAG_CHECKSUMS);
+		else
+			rc = -EOPNOTSUPP;
+		if (rc)
+			goto out_file;
+		CIFS_I(inode)->cifsAttrs |= FILE_ATTRIBUTE_INTEGRITY_STREAM;
+	}
+
+out_file:
+	if (do_close)
+		tcon->ses->server->ops->close(xid, tcon, oparms.fid);
+	else if (open_file)
+		cifsFileInfo_put(open_file);
+out_xid:
+	free_xid(xid);
+out_page:
+	free_dentry_path(page);
+	return rc;
+}
diff --git a/fs/smb/client/ioctl.c b/fs/smb/client/ioctl.c
index 56439da4f119..7c245085f891 100644
--- a/fs/smb/client/ioctl.c
+++ b/fs/smb/client/ioctl.c
@@ -356,12 +356,14 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
 	struct cifs_tcon *tcon;
 	struct tcon_link *tlink;
 	struct cifs_sb_info *cifs_sb;
+#if 0
 	__u64	ExtAttrBits = 0;
 #ifdef CONFIG_CIFS_POSIX
 #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
 	__u64   caps;
 #endif /* CONFIG_CIFS_ALLOW_INSECURE_LEGACY */
 #endif /* CONFIG_CIFS_POSIX */
+#endif
 
 	xid = get_xid();
 
@@ -372,6 +374,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
 		trace_smb3_ioctl(xid, pSMBFile->fid.persistent_fid, command);
 
 	switch (command) {
+#if 0
 		case FS_IOC_GETFLAGS:
 			if (pSMBFile == NULL)
 				break;
@@ -429,10 +432,11 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
 			/* Try to set compress flag */
 			if (tcon->ses->server->ops->set_compression) {
 				rc = tcon->ses->server->ops->set_compression(
-							xid, tcon, pSMBFile);
+							xid, tcon, pSMBFile, true);
 				cifs_dbg(FYI, "set compress flag rc %d\n", rc);
 			}
 			break;
+#endif
 		case CIFS_IOC_COPYCHUNK_FILE:
 			rc = cifs_ioctl_copychunk(xid, filep, arg);
 			break;
@@ -445,7 +449,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
 			tcon = tlink_tcon(pSMBFile->tlink);
 			if (tcon->ses->server->ops->set_integrity)
 				rc = tcon->ses->server->ops->set_integrity(xid,
-						tcon, pSMBFile);
+						tcon, pSMBFile, true);
 			else
 				rc = -EOPNOTSUPP;
 			break;
diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
index ba6452d89df3..2e854bde67de 100644
--- a/fs/smb/client/smb1ops.c
+++ b/fs/smb/client/smb1ops.c
@@ -1245,9 +1245,9 @@ smb_set_file_info(struct inode *inode, const char *full_path,
 
 static int
 cifs_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
-		   struct cifsFileInfo *cfile)
+		   struct cifsFileInfo *cfile, bool enable)
 {
-	return CIFSSMB_set_compression(xid, tcon, cfile->fid.netfid);
+	return CIFSSMB_set_compression(xid, tcon, cfile->fid.netfid, enable);
 }
 
 static int
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index f8445a9ff9a1..9c66e413c59c 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2106,20 +2106,20 @@ smb2_duplicate_extents(const unsigned int xid,
 
 static int
 smb2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
-		   struct cifsFileInfo *cfile)
+		   struct cifsFileInfo *cfile, bool enable)
 {
 	return SMB2_set_compression(xid, tcon, cfile->fid.persistent_fid,
-			    cfile->fid.volatile_fid);
+			    cfile->fid.volatile_fid, enable);
 }
 
 static int
 smb3_set_integrity(const unsigned int xid, struct cifs_tcon *tcon,
-		   struct cifsFileInfo *cfile)
+		   struct cifsFileInfo *cfile, bool enable)
 {
 	struct fsctl_set_integrity_information_req integr_info;
 	unsigned int ret_data_len;
 
-	integr_info.ChecksumAlgorithm = cpu_to_le16(CHECKSUM_TYPE_UNCHANGED);
+	integr_info.ChecksumAlgorithm = cpu_to_le16(enable ? CHECKSUM_TYPE_CRC64 : CHECKSUM_TYPE_NONE);
 	integr_info.Flags = 0;
 	integr_info.Reserved = 0;
 
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index a75947797d58..57d716cfc800 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -3537,14 +3537,14 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,
 
 int
 SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
-		     u64 persistent_fid, u64 volatile_fid)
+		     u64 persistent_fid, u64 volatile_fid, bool enable)
 {
 	int rc;
 	struct  compress_ioctl fsctl_input;
 	char *ret_data = NULL;
 
 	fsctl_input.CompressionState =
-			cpu_to_le16(COMPRESSION_FORMAT_DEFAULT);
+			cpu_to_le16(enable ? COMPRESSION_FORMAT_DEFAULT : COMPRESSION_FORMAT_NONE);
 
 	rc = SMB2_ioctl(xid, tcon, persistent_fid, volatile_fid,
 			FSCTL_SET_COMPRESSION,
diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
index cec5921bfdd2..6086bbdeeae0 100644
--- a/fs/smb/client/smb2proto.h
+++ b/fs/smb/client/smb2proto.h
@@ -250,7 +250,7 @@ extern int SMB2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
 		       u64 persistent_fid, u64 volatile_fid,
 		       struct smb2_file_full_ea_info *buf, int len);
 extern int SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
-				u64 persistent_fid, u64 volatile_fid);
+				u64 persistent_fid, u64 volatile_fid, bool enable);
 extern int SMB2_oplock_break(const unsigned int xid, struct cifs_tcon *tcon,
 			     const u64 persistent_fid, const u64 volatile_fid,
 			     const __u8 oplock_level);
diff --git a/fs/smb/common/smb2pdu.h b/fs/smb/common/smb2pdu.h
index ab902b155650..a24194bef849 100644
--- a/fs/smb/common/smb2pdu.h
+++ b/fs/smb/common/smb2pdu.h
@@ -1077,6 +1077,8 @@ struct smb2_server_client_notification {
 #define FILE_ATTRIBUTE_ENCRYPTED		0x00004000
 #define FILE_ATTRIBUTE_INTEGRITY_STREAM		0x00008000
 #define FILE_ATTRIBUTE_NO_SCRUB_DATA		0x00020000
+#define FILE_ATTRIBUTE_PINNED			0x00080000
+#define FILE_ATTRIBUTE_UNPINNED			0x00100000
 #define FILE_ATTRIBUTE__MASK			0x00007FB7
 
 #define FILE_ATTRIBUTE_READONLY_LE              cpu_to_le32(0x00000001)
-- 
2.20.1

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Eric Biggers]

On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
which use that flag?  In the fscrypt case it's very intentional that
FS_ENCRYPT_FL can be gotten via FS_IOC_GETFLAGS but not set via FS_IOC_SETFLAGS.
A simple toggle of the flag can't work, as it doesn't provide the needed
information.  Instead there is a separate ioctl (FS_IOC_SET_ENCRYPTION_POLICY)
for enabling encryption which takes additional parameters and only works on
empty directories.

- Eric

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Pali Rohár]

On Sunday 16 February 2025 10:34:32 Eric Biggers wrote:
> On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> which use that flag?  In the fscrypt case it's very intentional that
> FS_ENCRYPT_FL can be gotten via FS_IOC_GETFLAGS but not set via FS_IOC_SETFLAGS.
> A simple toggle of the flag can't work, as it doesn't provide the needed
> information.  Instead there is a separate ioctl (FS_IOC_SET_ENCRYPTION_POLICY)
> for enabling encryption which takes additional parameters and only works on
> empty directories.
> 
> - Eric

This encrypt flag I have not implemented in the last cifs patch.
For SMB it needs to use additional SMB IOCTL which is not supported yet.
So I have not looked at that deeply yet.

I tested only that setting and clearing compression bit is working over
cifs SMB client, via that additional SMB IOCTL.

Re: [RFC PATCH 2/4] fs: Extend FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Amir Goldstein]

On Sun, Feb 16, 2025 at 5:42 PM Pali Rohár <pali@kernel.org> wrote:
>
> struct fsxattr has 8 reserved padding bytes. Use these bytes for defining
> new fields fsx_xflags2, fsx_xflags2_mask and fsx_xflags_mask in backward
> compatible manner. If the new FS_XFLAG_HASEXTFIELDS flag in fsx_xflags is
> not set then these new fields are treated as not present, like before this
> change.
>
> New field fsx_xflags_mask for SET operation specifies which flags in
> fsx_xflags are going to be changed. This would allow userspace application
> to change just subset of all flags. For GET operation this field specifies
> which FS_XFLAG_* flags are supported by the file.
>
> New field fsx_xflags2 specify new flags FS_XFLAG2_* which defines some of
> Windows FILE_ATTRIBUTE_* attributes, which are mostly not going to be
> interpreted or used by the kernel, and are mostly going to be used by
> userspace. Field fsx_xflags2_mask then specify mask for them.
>
> This change defines just API without filesystem support for them. These
> attributes can be implemented later for Windows filesystems like FAT, NTFS,
> exFAT, UDF, SMB, NFS4 which all native storage for those attributes (or at
> least some subset of them).
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  include/uapi/linux/fs.h | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 367bc5289c47..93e947d6e604 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -145,15 +145,26 @@ struct fsxattr {
>         __u32           fsx_nextents;   /* nextents field value (get)   */
>         __u32           fsx_projid;     /* project identifier (get/set) */
>         __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
> -       unsigned char   fsx_pad[8];
> +       __u16           fsx_xflags2;    /* xflags2 field value (get/set)*/
> +       __u16           fsx_xflags2_mask;/*mask for xflags2 (get/set)*/
> +       __u32           fsx_xflags_mask;/* mask for xflags (get/set)*/
> +       /*
> +        * For FS_IOC_FSSETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags from fsx_xflags
> +        * and fsx_xflags2 fields are going to be changed.
> +        *
> +        * For FS_IOC_FSGETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags are supported.
> +        */
>  };
>
>  /*
> - * Flags for the fsx_xflags field
> + * Flags for the fsx_xflags and fsx_xflags_mask fields
>   */
>  #define FS_XFLAG_REALTIME      0x00000001      /* data in realtime volume */
>  #define FS_XFLAG_PREALLOC      0x00000002      /* preallocated file extents */
> -#define FS_XFLAG_IMMUTABLE     0x00000008      /* file cannot be modified */
> +#define FS_XFLAG_IMMUTABLEUSER 0x00000004      /* file cannot be modified, changing this bit does not require CAP_LINUX_IMMUTABLE, equivalent of Windows FILE_ATTRIBUTE_READONLY */

So why not call it FS_XFLAG2_READONLY? IDGI

Does anyone think that FS_XFLAG_IMMUTABLEUSER is more clear or something?

Thanks,
Amir.

> +#define FS_XFLAG_IMMUTABLE     0x00000008      /* file cannot be modified, changing this bit requires CAP_LINUX_IMMUTABLE */
>  #define FS_XFLAG_APPEND                0x00000010      /* all writes append */
>  #define FS_XFLAG_SYNC          0x00000020      /* all writes synchronous */
>  #define FS_XFLAG_NOATIME       0x00000040      /* do not update access time */
> @@ -167,10 +178,25 @@ struct fsxattr {
>  #define FS_XFLAG_FILESTREAM    0x00004000      /* use filestream allocator */
>  #define FS_XFLAG_DAX           0x00008000      /* use DAX for IO */
>  #define FS_XFLAG_COWEXTSIZE    0x00010000      /* CoW extent size allocator hint */
> -#define FS_XFLAG_COMPRESSED    0x00020000      /* compressed file */
> -#define FS_XFLAG_ENCRYPTED     0x00040000      /* encrypted file */
> +#define FS_XFLAG_COMPRESSED    0x00020000      /* compressed file, equivalent of Windows FILE_ATTRIBUTE_COMPRESSED */
> +#define FS_XFLAG_ENCRYPTED     0x00040000      /* encrypted file, equivalent of Windows FILE_ATTRIBUTE_ENCRYPTED */
> +#define FS_XFLAG_CHECKSUMS     0x00080000      /* checksum for data and metadata, equivalent of Windows FILE_ATTRIBUTE_INTEGRITY_STREAM */
> +#define FS_XFLAG_HASEXTFIELDS  0x40000000      /* fields fsx_xflags_mask, fsx_xflags2 and fsx_xflags2_mask are present */
>  #define FS_XFLAG_HASATTR       0x80000000      /* no DIFLAG for this   */
>
> +/*
> + * Flags for the fsx_xflags2 and fsx_xflags2_mask fields
> + */
> +#define FS_XFLAG2_HIDDEN       0x0001  /* inode is hidden, equivalent of Widows FILE_ATTRIBUTE_HIDDEN */
> +#define FS_XFLAG2_SYSTEM       0x0002  /* inode is part of operating system, equivalent of Windows FILE_ATTRIBUTE_SYSTEM */
> +#define FS_XFLAG2_ARCHIVE      0x0004  /* inode was not archived yet, equivalent of Windows FILE_ATTRIBUTE_ARCHIVE */
> +#define FS_XFLAG2_TEMPORARY    0x0008  /* inode content does not have to preserved across reboots, equivalent of Windows FILE_ATTRIBUTE_TEMPORARY */
> +#define FS_XFLAG2_NOTINDEXED   0x0010  /* inode will not be indexed by content indexing service, equivalent of Windows FILE_ATTRIBUTE_NOT_CONTENT_INDEXED */
> +#define FS_XFLAG2_NOSCRUBDATA  0x0020  /* file inode will not be checked by scrubber (proactive background data integrity scanner), for directory inode it means that newly created child would have this flag set, equivalent of Windows FILE_ATTRIBUTE_NO_SCRUB_DATA */
> +#define FS_XFLAG2_OFFLINE      0x0040  /* inode is marked as HSM offline, equivalent of Windows FILE_ATTRIBUTE_OFFLINE */
> +#define FS_XFLAG2_PINNED       0x0080  /* inode data content must be always stored in local HSM storage, equivalent of Windows FILE_ATTRIBUTE_PINNED */
> +#define FS_XFLAG2_UNPINNED     0x0100  /* inode data content can be removed from local HSM storage, equivalent of Windows FILE_ATTRIBUTE_UNPINNED */
> +
>  /* the read-only stuff doesn't really belong here, but any other place is
>     probably as bad and I don't want to create yet another include file. */
>
> --
> 2.20.1
>

Re: [RFC PATCH 2/4] fs: Extend FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Pali Rohár]

On Sunday 16 February 2025 20:55:09 Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 5:42 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > struct fsxattr has 8 reserved padding bytes. Use these bytes for defining
> > new fields fsx_xflags2, fsx_xflags2_mask and fsx_xflags_mask in backward
> > compatible manner. If the new FS_XFLAG_HASEXTFIELDS flag in fsx_xflags is
> > not set then these new fields are treated as not present, like before this
> > change.
> >
> > New field fsx_xflags_mask for SET operation specifies which flags in
> > fsx_xflags are going to be changed. This would allow userspace application
> > to change just subset of all flags. For GET operation this field specifies
> > which FS_XFLAG_* flags are supported by the file.
> >
> > New field fsx_xflags2 specify new flags FS_XFLAG2_* which defines some of
> > Windows FILE_ATTRIBUTE_* attributes, which are mostly not going to be
> > interpreted or used by the kernel, and are mostly going to be used by
> > userspace. Field fsx_xflags2_mask then specify mask for them.
> >
> > This change defines just API without filesystem support for them. These
> > attributes can be implemented later for Windows filesystems like FAT, NTFS,
> > exFAT, UDF, SMB, NFS4 which all native storage for those attributes (or at
> > least some subset of them).
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  include/uapi/linux/fs.h | 36 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > index 367bc5289c47..93e947d6e604 100644
> > --- a/include/uapi/linux/fs.h
> > +++ b/include/uapi/linux/fs.h
> > @@ -145,15 +145,26 @@ struct fsxattr {
> >         __u32           fsx_nextents;   /* nextents field value (get)   */
> >         __u32           fsx_projid;     /* project identifier (get/set) */
> >         __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > -       unsigned char   fsx_pad[8];
> > +       __u16           fsx_xflags2;    /* xflags2 field value (get/set)*/
> > +       __u16           fsx_xflags2_mask;/*mask for xflags2 (get/set)*/
> > +       __u32           fsx_xflags_mask;/* mask for xflags (get/set)*/
> > +       /*
> > +        * For FS_IOC_FSSETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> > +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags from fsx_xflags
> > +        * and fsx_xflags2 fields are going to be changed.
> > +        *
> > +        * For FS_IOC_FSGETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> > +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags are supported.
> > +        */
> >  };
> >
> >  /*
> > - * Flags for the fsx_xflags field
> > + * Flags for the fsx_xflags and fsx_xflags_mask fields
> >   */
> >  #define FS_XFLAG_REALTIME      0x00000001      /* data in realtime volume */
> >  #define FS_XFLAG_PREALLOC      0x00000002      /* preallocated file extents */
> > -#define FS_XFLAG_IMMUTABLE     0x00000008      /* file cannot be modified */
> > +#define FS_XFLAG_IMMUTABLEUSER 0x00000004      /* file cannot be modified, changing this bit does not require CAP_LINUX_IMMUTABLE, equivalent of Windows FILE_ATTRIBUTE_READONLY */
> 
> So why not call it FS_XFLAG2_READONLY? IDGI

Just because to show that these two flags are similar, just one is for
root (or CAP_LINUX_IMMUTABLE) and another is for normal user.

For example FreeBSD has also both flags (one for root and one for user)
and uses names SF_IMMUTABLE and UF_IMMUTABLE.

For me having FS_XFLAG_IMMUTABLE and FS_XFLAG2_READONLY sounds less
clear, and does not explain how these two flags differs.

> Does anyone think that FS_XFLAG_IMMUTABLEUSER is more clear or something?
> 
> Thanks,
> Amir.
> 
> > +#define FS_XFLAG_IMMUTABLE     0x00000008      /* file cannot be modified, changing this bit requires CAP_LINUX_IMMUTABLE */
> >  #define FS_XFLAG_APPEND                0x00000010      /* all writes append */
> >  #define FS_XFLAG_SYNC          0x00000020      /* all writes synchronous */
> >  #define FS_XFLAG_NOATIME       0x00000040      /* do not update access time */
> > @@ -167,10 +178,25 @@ struct fsxattr {
> >  #define FS_XFLAG_FILESTREAM    0x00004000      /* use filestream allocator */
> >  #define FS_XFLAG_DAX           0x00008000      /* use DAX for IO */
> >  #define FS_XFLAG_COWEXTSIZE    0x00010000      /* CoW extent size allocator hint */
> > -#define FS_XFLAG_COMPRESSED    0x00020000      /* compressed file */
> > -#define FS_XFLAG_ENCRYPTED     0x00040000      /* encrypted file */
> > +#define FS_XFLAG_COMPRESSED    0x00020000      /* compressed file, equivalent of Windows FILE_ATTRIBUTE_COMPRESSED */
> > +#define FS_XFLAG_ENCRYPTED     0x00040000      /* encrypted file, equivalent of Windows FILE_ATTRIBUTE_ENCRYPTED */
> > +#define FS_XFLAG_CHECKSUMS     0x00080000      /* checksum for data and metadata, equivalent of Windows FILE_ATTRIBUTE_INTEGRITY_STREAM */
> > +#define FS_XFLAG_HASEXTFIELDS  0x40000000      /* fields fsx_xflags_mask, fsx_xflags2 and fsx_xflags2_mask are present */
> >  #define FS_XFLAG_HASATTR       0x80000000      /* no DIFLAG for this   */
> >
> > +/*
> > + * Flags for the fsx_xflags2 and fsx_xflags2_mask fields
> > + */
> > +#define FS_XFLAG2_HIDDEN       0x0001  /* inode is hidden, equivalent of Widows FILE_ATTRIBUTE_HIDDEN */
> > +#define FS_XFLAG2_SYSTEM       0x0002  /* inode is part of operating system, equivalent of Windows FILE_ATTRIBUTE_SYSTEM */
> > +#define FS_XFLAG2_ARCHIVE      0x0004  /* inode was not archived yet, equivalent of Windows FILE_ATTRIBUTE_ARCHIVE */
> > +#define FS_XFLAG2_TEMPORARY    0x0008  /* inode content does not have to preserved across reboots, equivalent of Windows FILE_ATTRIBUTE_TEMPORARY */
> > +#define FS_XFLAG2_NOTINDEXED   0x0010  /* inode will not be indexed by content indexing service, equivalent of Windows FILE_ATTRIBUTE_NOT_CONTENT_INDEXED */
> > +#define FS_XFLAG2_NOSCRUBDATA  0x0020  /* file inode will not be checked by scrubber (proactive background data integrity scanner), for directory inode it means that newly created child would have this flag set, equivalent of Windows FILE_ATTRIBUTE_NO_SCRUB_DATA */
> > +#define FS_XFLAG2_OFFLINE      0x0040  /* inode is marked as HSM offline, equivalent of Windows FILE_ATTRIBUTE_OFFLINE */
> > +#define FS_XFLAG2_PINNED       0x0080  /* inode data content must be always stored in local HSM storage, equivalent of Windows FILE_ATTRIBUTE_PINNED */
> > +#define FS_XFLAG2_UNPINNED     0x0100  /* inode data content can be removed from local HSM storage, equivalent of Windows FILE_ATTRIBUTE_UNPINNED */
> > +
> >  /* the read-only stuff doesn't really belong here, but any other place is
> >     probably as bad and I don't want to create yet another include file. */
> >
> > --
> > 2.20.1
> >

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Amir Goldstein]

On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
>
> Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> which use that flag?

As far as I can tell, after fileattr_fill_xflags() call in
ioctl_fssetxattr(), the call
to ext4_fileattr_set() should behave exactly the same if it came some
FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.

However, unlike the legacy API, we now have an opportunity to deal with
EXT4_FL_USER_MODIFIABLE better than this:
        /*
         * chattr(1) grabs flags via GETFLAGS, modifies the result and
         * passes that to SETFLAGS. So we cannot easily make SETFLAGS
         * more restrictive than just silently masking off visible but
         * not settable flags as we always did.
         */

if we have the xflags_mask in the new API (not only the xflags) then
chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
ext4_fileattr_set() can verify that
(xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).

However, Pali, this is an important point that your RFC did not follow -
AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
(and other fs) does not return any error for unknown xflags, it just
ignores them.

This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
before adding support to ANY new xflags, whether they are mapped to
existing flags like in this patch or are completely new xflags.

Thanks,
Amir.

Re: [RFC PATCH 4/4] cifs: Implement FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Amir Goldstein]

On Sun, Feb 16, 2025 at 5:42 PM Pali Rohár <pali@kernel.org> wrote:
>

No empty commit message please

> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  fs/smb/client/cifsfs.c    |   4 +
>  fs/smb/client/cifsfs.h    |   2 +
>  fs/smb/client/cifsglob.h  |   4 +-
>  fs/smb/client/cifsproto.h |   2 +-
>  fs/smb/client/cifssmb.c   |   4 +-
>  fs/smb/client/inode.c     | 181 ++++++++++++++++++++++++++++++++++++++
>  fs/smb/client/ioctl.c     |   8 +-
>  fs/smb/client/smb1ops.c   |   4 +-
>  fs/smb/client/smb2ops.c   |   8 +-
>  fs/smb/client/smb2pdu.c   |   4 +-
>  fs/smb/client/smb2proto.h |   2 +-
>  fs/smb/common/smb2pdu.h   |   2 +
>  12 files changed, 209 insertions(+), 16 deletions(-)
>
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index ea31d693ea9f..b441675f9afd 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1182,6 +1182,8 @@ const struct inode_operations cifs_dir_inode_ops = {
>         .listxattr = cifs_listxattr,
>         .get_acl = cifs_get_acl,
>         .set_acl = cifs_set_acl,
> +       .fileattr_get = cifs_fileattr_get,
> +       .fileattr_set = cifs_fileattr_set,
>  };
>
>  const struct inode_operations cifs_file_inode_ops = {
> @@ -1192,6 +1194,8 @@ const struct inode_operations cifs_file_inode_ops = {
>         .fiemap = cifs_fiemap,
>         .get_acl = cifs_get_acl,
>         .set_acl = cifs_set_acl,
> +       .fileattr_get = cifs_fileattr_get,
> +       .fileattr_set = cifs_fileattr_set,
>  };
>
>  const char *cifs_get_link(struct dentry *dentry, struct inode *inode,
> diff --git a/fs/smb/client/cifsfs.h b/fs/smb/client/cifsfs.h
> index 831fee962c4d..b1e6025e2cbc 100644
> --- a/fs/smb/client/cifsfs.h
> +++ b/fs/smb/client/cifsfs.h
> @@ -77,6 +77,8 @@ extern int cifs_setattr(struct mnt_idmap *, struct dentry *,
>                         struct iattr *);
>  extern int cifs_fiemap(struct inode *, struct fiemap_extent_info *, u64 start,
>                        u64 len);
> +extern int cifs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
> +extern int cifs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry, struct fileattr *fa);
>
>  extern const struct inode_operations cifs_file_inode_ops;
>  extern const struct inode_operations cifs_symlink_inode_ops;
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index b764bfe916b4..233a0a13b0e2 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -426,7 +426,7 @@ struct smb_version_operations {
>         int (*set_file_info)(struct inode *, const char *, FILE_BASIC_INFO *,
>                              const unsigned int);
>         int (*set_compression)(const unsigned int, struct cifs_tcon *,
> -                              struct cifsFileInfo *);
> +                              struct cifsFileInfo *, bool);
>         /* check if we can send an echo or nor */
>         bool (*can_echo)(struct TCP_Server_Info *);
>         /* send echo request */
> @@ -538,7 +538,7 @@ struct smb_version_operations {
>         int (*calc_signature)(struct smb_rqst *, struct TCP_Server_Info *,
>                                 bool allocate_crypto);
>         int (*set_integrity)(const unsigned int, struct cifs_tcon *tcon,
> -                            struct cifsFileInfo *src_file);
> +                            struct cifsFileInfo *src_file, bool enable);
>         int (*enum_snapshots)(const unsigned int xid, struct cifs_tcon *tcon,
>                              struct cifsFileInfo *src_file, void __user *);
>         int (*notify)(const unsigned int xid, struct file *pfile,
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 47ecc0884a74..f5f6be6f343e 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -506,7 +506,7 @@ extern struct inode *cifs_create_reparse_inode(struct cifs_open_info_data *data,
>                                                struct kvec *reparse_iov,
>                                                struct kvec *xattr_iov);
>  extern int CIFSSMB_set_compression(const unsigned int xid,
> -                                  struct cifs_tcon *tcon, __u16 fid);
> +                                  struct cifs_tcon *tcon, __u16 fid, bool enable);
>  extern int CIFS_open(const unsigned int xid, struct cifs_open_parms *oparms,
>                      int *oplock, FILE_ALL_INFO *buf);
>  extern int SMBOldOpen(const unsigned int xid, struct cifs_tcon *tcon,
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index 3dbff55b639d..643a55db3ca9 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -3454,7 +3454,7 @@ struct inode *cifs_create_reparse_inode(struct cifs_open_info_data *data,
>
>  int
>  CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> -                   __u16 fid)
> +                   __u16 fid, bool enable)
>  {
>         int rc = 0;
>         int bytes_returned;
> @@ -3467,7 +3467,7 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
>         if (rc)
>                 return rc;
>
> -       pSMB->compression_state = cpu_to_le16(COMPRESSION_FORMAT_DEFAULT);
> +       pSMB->compression_state = cpu_to_le16(enable ? COMPRESSION_FORMAT_DEFAULT : COMPRESSION_FORMAT_NONE);
>
>         pSMB->TotalParameterCount = 0;
>         pSMB->TotalDataCount = cpu_to_le32(2);
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index dfad9284a87c..d07ebb99c262 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -13,6 +13,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/wait_bit.h>
>  #include <linux/fiemap.h>
> +#include <linux/fileattr.h>
>  #include <asm/div64.h>
>  #include "cifsfs.h"
>  #include "cifspdu.h"
> @@ -83,6 +84,7 @@ static void cifs_set_ops(struct inode *inode)
>                 inode->i_op = &cifs_symlink_inode_ops;
>                 break;
>         default:
> +               inode->i_op = &cifs_file_inode_ops;
>                 init_special_inode(inode, inode->i_mode, inode->i_rdev);
>                 break;
>         }
> @@ -3282,3 +3284,182 @@ cifs_setattr(struct mnt_idmap *idmap, struct dentry *direntry,
>         /* BB: add cifs_setattr_legacy for really old servers */
>         return rc;
>  }
> +
> +int cifs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> +{
> +       struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
> +       struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> +       struct inode *inode = d_inode(dentry);
> +       u32 attrs = CIFS_I(inode)->cifsAttrs;
> +       u32 fsattrs = le32_to_cpu(tcon->fsAttrInfo.Attributes);
> +       u32 xflags = 0;
> +       u32 xflags_mask = FS_XFLAG_IMMUTABLEUSER;
> +       u16 xflags2 = 0;
> +       u16 xflags2_mask = FS_XFLAG2_HIDDEN | FS_XFLAG2_SYSTEM | FS_XFLAG2_ARCHIVE |
> +                          FS_XFLAG2_TEMPORARY | FS_XFLAG2_NOTINDEXED |
> +                          FS_XFLAG2_NOSCRUBDATA | FS_XFLAG2_OFFLINE |
> +                          FS_XFLAG2_PINNED | FS_XFLAG2_UNPINNED;
> +
> +       if (fsattrs & FILE_FILE_COMPRESSION)
> +               xflags_mask |= FS_XFLAG_COMPRESSED;
> +       if (fsattrs & FILE_SUPPORTS_ENCRYPTION)
> +               xflags_mask |= FS_XFLAG_COMPRESSED;
> +       if (fsattrs & FILE_SUPPORT_INTEGRITY_STREAMS)
> +               xflags_mask |= FS_XFLAG_CHECKSUMS;
> +
> +       if (attrs & FILE_ATTRIBUTE_READONLY)
> +               xflags |= FS_XFLAG_IMMUTABLEUSER;
> +       if (attrs & FILE_ATTRIBUTE_HIDDEN)
> +               xflags2 |= FS_XFLAG2_HIDDEN;
> +       if (attrs & FILE_ATTRIBUTE_SYSTEM)
> +               xflags2 |= FS_XFLAG2_SYSTEM;
> +       if (attrs & FILE_ATTRIBUTE_ARCHIVE)
> +               xflags2 |= FS_XFLAG2_ARCHIVE;
> +       if (attrs & FILE_ATTRIBUTE_TEMPORARY)
> +               xflags2 |= FS_XFLAG2_TEMPORARY;
> +       if (attrs & FILE_ATTRIBUTE_COMPRESSED)
> +               xflags |= FS_XFLAG_COMPRESSED;
> +       if (attrs & FILE_ATTRIBUTE_OFFLINE)
> +               xflags2 |= FS_XFLAG2_OFFLINE;
> +       if (attrs & FILE_ATTRIBUTE_NOT_CONTENT_INDEXED)
> +               xflags2 |= FS_XFLAG2_NOTINDEXED;
> +       if (attrs & FILE_ATTRIBUTE_ENCRYPTED)
> +               xflags |= FS_XFLAG_ENCRYPTED;
> +       if (attrs & FILE_ATTRIBUTE_INTEGRITY_STREAM)
> +               xflags |= FS_XFLAG_CHECKSUMS;
> +       if (attrs & FILE_ATTRIBUTE_NO_SCRUB_DATA)
> +               xflags2 |= FS_XFLAG2_NOSCRUBDATA;
> +       if (attrs & FILE_ATTRIBUTE_PINNED)
> +               xflags2 |= FS_XFLAG2_PINNED;
> +       if (attrs & FILE_ATTRIBUTE_UNPINNED)
> +               xflags2 |= FS_XFLAG2_UNPINNED;
> +
> +       fileattr_fill_xflags(fa, xflags, xflags_mask, xflags2, xflags2_mask);
> +       return 0;
> +}
> +
> +#define MODIFY_ATTRS_COND(attrs, xflags, xflag, attr) (attrs) ^= ((-(!!((xflags) & (xflag))) ^ (attrs)) & (attr))
> +
> +int cifs_fileattr_set(struct mnt_idmap *idmap,
> +                     struct dentry *dentry, struct fileattr *fa)
> +{
> +       struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
> +       struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> +       struct inode *inode = d_inode(dentry);
> +       u32 attrs = CIFS_I(inode)->cifsAttrs;
> +       struct cifsFileInfo open_file_tmp = {};
> +       struct cifsFileInfo *open_file = NULL;
> +       struct cifs_open_parms oparms;
> +       FILE_BASIC_INFO info_buf = {};
> +       bool do_close = false;
> +       const char *full_path;
> +       unsigned int xid;
> +       __u32 oplock;
> +       void *page;
> +       int rc;
> +
> +       if ((fa->fsx_xflags_mask & ~(FS_XFLAG_IMMUTABLEUSER | FS_XFLAG_COMPRESSED |
> +                                FS_XFLAG_ENCRYPTED | FS_XFLAG_CHECKSUMS)) ||
> +           (fa->fsx_xflags2_mask & ~(FS_XFLAG2_HIDDEN | FS_XFLAG2_SYSTEM | FS_XFLAG2_ARCHIVE |
> +                                 FS_XFLAG2_TEMPORARY | FS_XFLAG2_NOTINDEXED |
> +                                 FS_XFLAG2_NOSCRUBDATA | FS_XFLAG2_OFFLINE |
> +                                 FS_XFLAG2_PINNED | FS_XFLAG2_UNPINNED)) ||
> +           (fa->flags & ~FS_COMMON_FL))
> +               return -EOPNOTSUPP;
> +
> +       if (fa->fsx_xflags_mask & FS_XFLAG_IMMUTABLEUSER)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags, FS_XFLAG_IMMUTABLEUSER, FILE_ATTRIBUTE_READONLY);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_HIDDEN)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_HIDDEN, FILE_ATTRIBUTE_HIDDEN);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_SYSTEM)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_SYSTEM, FILE_ATTRIBUTE_SYSTEM);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_ARCHIVE)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_ARCHIVE, FILE_ATTRIBUTE_ARCHIVE);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_TEMPORARY)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_TEMPORARY, FILE_ATTRIBUTE_TEMPORARY);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_NOTINDEXED)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_NOTINDEXED, FILE_ATTRIBUTE_NOT_CONTENT_INDEXED);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_NOSCRUBDATA)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_NOSCRUBDATA, FILE_ATTRIBUTE_NO_SCRUB_DATA);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_OFFLINE)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_OFFLINE, FILE_ATTRIBUTE_OFFLINE);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_PINNED)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_PINNED, FILE_ATTRIBUTE_PINNED);
> +       if (fa->fsx_xflags2_mask & FS_XFLAG2_UNPINNED)
> +               MODIFY_ATTRS_COND(attrs, fa->fsx_xflags2, FS_XFLAG2_UNPINNED, FILE_ATTRIBUTE_UNPINNED);
> +
> +       page = alloc_dentry_path();
> +
> +       full_path = build_path_from_dentry(dentry, page);
> +       if (IS_ERR(full_path)) {
> +               rc = PTR_ERR(full_path);
> +               goto out_page;
> +       }
> +
> +       xid = get_xid();
> +
> +       if (attrs != CIFS_I(inode)->cifsAttrs) {
> +               info_buf.Attributes = cpu_to_le32(attrs);
> +               if (tcon->ses->server->ops->set_file_info)
> +                       rc = tcon->ses->server->ops->set_file_info(inode, full_path, &info_buf, xid);
> +               else
> +                       rc = -EOPNOTSUPP;
> +               if (rc)
> +                       goto out_xid;
> +               CIFS_I(inode)->cifsAttrs = attrs;
> +       }
> +
> +       if (fa->fsx_xflags_mask & (FS_XFLAG_COMPRESSED | FS_XFLAG_ENCRYPTED | FS_XFLAG_CHECKSUMS)) {
> +               open_file = find_writable_file(CIFS_I(inode), FIND_WR_FSUID_ONLY);
> +               if (!open_file) {
> +                       oparms = CIFS_OPARMS(cifs_sb, tcon, full_path, FILE_WRITE_DATA, FILE_OPEN, 0, ACL_NO_MODE);
> +                       oparms.fid = &open_file_tmp.fid;
> +                       oplock = 0;
> +                       oparms.create_options = cifs_create_options(cifs_sb, 0);
> +                       rc = tcon->ses->server->ops->open(xid, &oparms, &oplock, NULL);
> +                       if (rc)
> +                               goto out_file;
> +                       do_close = true;
> +                       open_file = &open_file_tmp;
> +               }
> +       }
> +
> +       if (fa->fsx_xflags_mask & FS_XFLAG_COMPRESSED) {
> +               if (tcon->ses->server->ops->set_compression)
> +                       rc = tcon->ses->server->ops->set_compression(xid, tcon, open_file, fa->fsx_xflags & FS_XFLAG_COMPRESSED);
> +               else
> +                       rc = -EOPNOTSUPP;
> +               if (rc)
> +                       goto out_file;
> +               CIFS_I(inode)->cifsAttrs |= FILE_ATTRIBUTE_COMPRESSED;
> +       }
> +
> +       if (fa->fsx_xflags_mask & FS_XFLAG_ENCRYPTED) {
> +               /* TODO */
> +               rc = -EOPNOTSUPP;
> +               if (rc)
> +                       goto out_file;
> +               CIFS_I(inode)->cifsAttrs |= FILE_ATTRIBUTE_ENCRYPTED;
> +       }
> +
> +       if (fa->fsx_xflags_mask & FS_XFLAG_CHECKSUMS) {
> +               if (tcon->ses->server->ops->set_integrity)
> +                       rc = tcon->ses->server->ops->set_integrity(xid, tcon, open_file, fa->fsx_xflags & FS_XFLAG_CHECKSUMS);
> +               else
> +                       rc = -EOPNOTSUPP;
> +               if (rc)
> +                       goto out_file;
> +               CIFS_I(inode)->cifsAttrs |= FILE_ATTRIBUTE_INTEGRITY_STREAM;
> +       }
> +
> +out_file:
> +       if (do_close)
> +               tcon->ses->server->ops->close(xid, tcon, oparms.fid);
> +       else if (open_file)
> +               cifsFileInfo_put(open_file);
> +out_xid:
> +       free_xid(xid);
> +out_page:
> +       free_dentry_path(page);
> +       return rc;
> +}
> diff --git a/fs/smb/client/ioctl.c b/fs/smb/client/ioctl.c
> index 56439da4f119..7c245085f891 100644
> --- a/fs/smb/client/ioctl.c
> +++ b/fs/smb/client/ioctl.c
> @@ -356,12 +356,14 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
>         struct cifs_tcon *tcon;
>         struct tcon_link *tlink;
>         struct cifs_sb_info *cifs_sb;
> +#if 0
>         __u64   ExtAttrBits = 0;
>  #ifdef CONFIG_CIFS_POSIX
>  #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
>         __u64   caps;
>  #endif /* CONFIG_CIFS_ALLOW_INSECURE_LEGACY */
>  #endif /* CONFIG_CIFS_POSIX */
> +#endif
>
>         xid = get_xid();
>
> @@ -372,6 +374,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
>                 trace_smb3_ioctl(xid, pSMBFile->fid.persistent_fid, command);
>
>         switch (command) {
> +#if 0
>                 case FS_IOC_GETFLAGS:
>                         if (pSMBFile == NULL)
>                                 break;
> @@ -429,10 +432,11 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
>                         /* Try to set compress flag */
>                         if (tcon->ses->server->ops->set_compression) {
>                                 rc = tcon->ses->server->ops->set_compression(
> -                                                       xid, tcon, pSMBFile);
> +                                                       xid, tcon, pSMBFile, true);
>                                 cifs_dbg(FYI, "set compress flag rc %d\n", rc);
>                         }
>                         break;
> +#endif
>                 case CIFS_IOC_COPYCHUNK_FILE:
>                         rc = cifs_ioctl_copychunk(xid, filep, arg);
>                         break;
> @@ -445,7 +449,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg)
>                         tcon = tlink_tcon(pSMBFile->tlink);
>                         if (tcon->ses->server->ops->set_integrity)
>                                 rc = tcon->ses->server->ops->set_integrity(xid,
> -                                               tcon, pSMBFile);
> +                                               tcon, pSMBFile, true);
>                         else
>                                 rc = -EOPNOTSUPP;
>                         break;
> diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
> index ba6452d89df3..2e854bde67de 100644
> --- a/fs/smb/client/smb1ops.c
> +++ b/fs/smb/client/smb1ops.c
> @@ -1245,9 +1245,9 @@ smb_set_file_info(struct inode *inode, const char *full_path,
>
>  static int
>  cifs_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> -                  struct cifsFileInfo *cfile)
> +                  struct cifsFileInfo *cfile, bool enable)
>  {
> -       return CIFSSMB_set_compression(xid, tcon, cfile->fid.netfid);
> +       return CIFSSMB_set_compression(xid, tcon, cfile->fid.netfid, enable);
>  }
>
>  static int
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index f8445a9ff9a1..9c66e413c59c 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -2106,20 +2106,20 @@ smb2_duplicate_extents(const unsigned int xid,
>
>  static int
>  smb2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> -                  struct cifsFileInfo *cfile)
> +                  struct cifsFileInfo *cfile, bool enable)
>  {
>         return SMB2_set_compression(xid, tcon, cfile->fid.persistent_fid,
> -                           cfile->fid.volatile_fid);
> +                           cfile->fid.volatile_fid, enable);
>  }
>
>  static int
>  smb3_set_integrity(const unsigned int xid, struct cifs_tcon *tcon,
> -                  struct cifsFileInfo *cfile)
> +                  struct cifsFileInfo *cfile, bool enable)
>  {
>         struct fsctl_set_integrity_information_req integr_info;
>         unsigned int ret_data_len;
>
> -       integr_info.ChecksumAlgorithm = cpu_to_le16(CHECKSUM_TYPE_UNCHANGED);
> +       integr_info.ChecksumAlgorithm = cpu_to_le16(enable ? CHECKSUM_TYPE_CRC64 : CHECKSUM_TYPE_NONE);
>         integr_info.Flags = 0;
>         integr_info.Reserved = 0;
>
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index a75947797d58..57d716cfc800 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -3537,14 +3537,14 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,
>
>  int
>  SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> -                    u64 persistent_fid, u64 volatile_fid)
> +                    u64 persistent_fid, u64 volatile_fid, bool enable)
>  {
>         int rc;
>         struct  compress_ioctl fsctl_input;
>         char *ret_data = NULL;
>
>         fsctl_input.CompressionState =
> -                       cpu_to_le16(COMPRESSION_FORMAT_DEFAULT);
> +                       cpu_to_le16(enable ? COMPRESSION_FORMAT_DEFAULT : COMPRESSION_FORMAT_NONE);
>
>         rc = SMB2_ioctl(xid, tcon, persistent_fid, volatile_fid,
>                         FSCTL_SET_COMPRESSION,
> diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
> index cec5921bfdd2..6086bbdeeae0 100644
> --- a/fs/smb/client/smb2proto.h
> +++ b/fs/smb/client/smb2proto.h
> @@ -250,7 +250,7 @@ extern int SMB2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
>                        u64 persistent_fid, u64 volatile_fid,
>                        struct smb2_file_full_ea_info *buf, int len);
>  extern int SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> -                               u64 persistent_fid, u64 volatile_fid);
> +                               u64 persistent_fid, u64 volatile_fid, bool enable);
>  extern int SMB2_oplock_break(const unsigned int xid, struct cifs_tcon *tcon,
>                              const u64 persistent_fid, const u64 volatile_fid,
>                              const __u8 oplock_level);
> diff --git a/fs/smb/common/smb2pdu.h b/fs/smb/common/smb2pdu.h
> index ab902b155650..a24194bef849 100644
> --- a/fs/smb/common/smb2pdu.h
> +++ b/fs/smb/common/smb2pdu.h
> @@ -1077,6 +1077,8 @@ struct smb2_server_client_notification {
>  #define FILE_ATTRIBUTE_ENCRYPTED               0x00004000
>  #define FILE_ATTRIBUTE_INTEGRITY_STREAM                0x00008000
>  #define FILE_ATTRIBUTE_NO_SCRUB_DATA           0x00020000
> +#define FILE_ATTRIBUTE_PINNED                  0x00080000
> +#define FILE_ATTRIBUTE_UNPINNED                        0x00100000
>  #define FILE_ATTRIBUTE__MASK                   0x00007FB7
>
>  #define FILE_ATTRIBUTE_READONLY_LE              cpu_to_le32(0x00000001)
> --
> 2.20.1
>

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Pali Rohár]

On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> >
> > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > which use that flag?
> 
> As far as I can tell, after fileattr_fill_xflags() call in
> ioctl_fssetxattr(), the call
> to ext4_fileattr_set() should behave exactly the same if it came some
> FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> 
> However, unlike the legacy API, we now have an opportunity to deal with
> EXT4_FL_USER_MODIFIABLE better than this:
>         /*
>          * chattr(1) grabs flags via GETFLAGS, modifies the result and
>          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
>          * more restrictive than just silently masking off visible but
>          * not settable flags as we always did.
>          */
> 
> if we have the xflags_mask in the new API (not only the xflags) then
> chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> ext4_fileattr_set() can verify that
> (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> 
> However, Pali, this is an important point that your RFC did not follow -
> AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> (and other fs) does not return any error for unknown xflags, it just
> ignores them.
> 
> This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> before adding support to ANY new xflags, whether they are mapped to
> existing flags like in this patch or are completely new xflags.
> 
> Thanks,
> Amir.

But xflags_mask is available in this new API. It is available if the
FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
mentioned above can be included into this new API.

Or I'm missing something?

Re: [RFC PATCH 4/4] cifs: Implement FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Pali Rohár]

On Sunday 16 February 2025 21:21:27 Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 5:42 PM Pali Rohár <pali@kernel.org> wrote:
> >
> 
> No empty commit message please

This is mean as RFC, not the final version, I just included the oneline
commit message.

Re: [RFC PATCH 2/4] fs: Extend FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Amir Goldstein]

On Sun, Feb 16, 2025 at 9:01 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Sunday 16 February 2025 20:55:09 Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 5:42 PM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > struct fsxattr has 8 reserved padding bytes. Use these bytes for defining
> > > new fields fsx_xflags2, fsx_xflags2_mask and fsx_xflags_mask in backward
> > > compatible manner. If the new FS_XFLAG_HASEXTFIELDS flag in fsx_xflags is
> > > not set then these new fields are treated as not present, like before this
> > > change.
> > >
> > > New field fsx_xflags_mask for SET operation specifies which flags in
> > > fsx_xflags are going to be changed. This would allow userspace application
> > > to change just subset of all flags. For GET operation this field specifies
> > > which FS_XFLAG_* flags are supported by the file.
> > >
> > > New field fsx_xflags2 specify new flags FS_XFLAG2_* which defines some of
> > > Windows FILE_ATTRIBUTE_* attributes, which are mostly not going to be
> > > interpreted or used by the kernel, and are mostly going to be used by
> > > userspace. Field fsx_xflags2_mask then specify mask for them.
> > >
> > > This change defines just API without filesystem support for them. These
> > > attributes can be implemented later for Windows filesystems like FAT, NTFS,
> > > exFAT, UDF, SMB, NFS4 which all native storage for those attributes (or at
> > > least some subset of them).
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  include/uapi/linux/fs.h | 36 +++++++++++++++++++++++++++++++-----
> > >  1 file changed, 31 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > > index 367bc5289c47..93e947d6e604 100644
> > > --- a/include/uapi/linux/fs.h
> > > +++ b/include/uapi/linux/fs.h
> > > @@ -145,15 +145,26 @@ struct fsxattr {
> > >         __u32           fsx_nextents;   /* nextents field value (get)   */
> > >         __u32           fsx_projid;     /* project identifier (get/set) */
> > >         __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > > -       unsigned char   fsx_pad[8];
> > > +       __u16           fsx_xflags2;    /* xflags2 field value (get/set)*/
> > > +       __u16           fsx_xflags2_mask;/*mask for xflags2 (get/set)*/
> > > +       __u32           fsx_xflags_mask;/* mask for xflags (get/set)*/
> > > +       /*
> > > +        * For FS_IOC_FSSETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> > > +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags from fsx_xflags
> > > +        * and fsx_xflags2 fields are going to be changed.
> > > +        *
> > > +        * For FS_IOC_FSGETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> > > +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags are supported.
> > > +        */
> > >  };
> > >
> > >  /*
> > > - * Flags for the fsx_xflags field
> > > + * Flags for the fsx_xflags and fsx_xflags_mask fields
> > >   */
> > >  #define FS_XFLAG_REALTIME      0x00000001      /* data in realtime volume */
> > >  #define FS_XFLAG_PREALLOC      0x00000002      /* preallocated file extents */
> > > -#define FS_XFLAG_IMMUTABLE     0x00000008      /* file cannot be modified */
> > > +#define FS_XFLAG_IMMUTABLEUSER 0x00000004      /* file cannot be modified, changing this bit does not require CAP_LINUX_IMMUTABLE, equivalent of Windows FILE_ATTRIBUTE_READONLY */
> >
> > So why not call it FS_XFLAG2_READONLY? IDGI
>
> Just because to show that these two flags are similar, just one is for
> root (or CAP_LINUX_IMMUTABLE) and another is for normal user.
>
> For example FreeBSD has also both flags (one for root and one for user)
> and uses names SF_IMMUTABLE and UF_IMMUTABLE.
>
> For me having FS_XFLAG_IMMUTABLE and FS_XFLAG2_READONLY sounds less
> clear, and does not explain how these two flags differs.
>

Yes, I understand, but I do not agree.

What is your goal here?

Do you want to implement FreeBSD UF_IMMUTABLE?
maybe UF_APPEND as well?
Did anyone ask for this functionality?
Not that I know of.
The requirement is to implement an API to the functionality known
as READONLY in SMB and NTFS. Right?

TBH, I did not study the semantics of READONLY, but I had a
strong feeling that if we looked closely, we will find that other things are
possible to do with READONLY files that are not possible with IMMUTABLE
files. So I asked ChatGPT and it told me that all these can be changed:
1. File Attributes (Hidden, System, Archive, or Indexed).
2. Permissions (ACL - Access Control List)
3. Timestamps
4. Alternate Data Streams (ADS)

I do not know if ChatGPT is correct, but it also told me that a READONLY
file can be deleted (without removing the flag first).

This is very very far from what IS_IMMUTABLE is.
IS_IMMUTABLE is immutable to any metadata change.

Thanks,
Amir.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Amir Goldstein]

On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > >
> > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > which use that flag?
> >
> > As far as I can tell, after fileattr_fill_xflags() call in
> > ioctl_fssetxattr(), the call
> > to ext4_fileattr_set() should behave exactly the same if it came some
> > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> >
> > However, unlike the legacy API, we now have an opportunity to deal with
> > EXT4_FL_USER_MODIFIABLE better than this:
> >         /*
> >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> >          * more restrictive than just silently masking off visible but
> >          * not settable flags as we always did.
> >          */
> >
> > if we have the xflags_mask in the new API (not only the xflags) then
> > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > ext4_fileattr_set() can verify that
> > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> >
> > However, Pali, this is an important point that your RFC did not follow -
> > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > (and other fs) does not return any error for unknown xflags, it just
> > ignores them.
> >
> > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > before adding support to ANY new xflags, whether they are mapped to
> > existing flags like in this patch or are completely new xflags.
> >
> > Thanks,
> > Amir.
>
> But xflags_mask is available in this new API. It is available if the
> FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> mentioned above can be included into this new API.
>
> Or I'm missing something?

Yes, you are missing something very fundamental to backward compat API -
You cannot change the existing kernels.

You should ask yourself one question:
What happens if I execute the old ioctl FS_IOC_FSSETXATTR
on an existing old kernel with the new extended flags?

The answer, to the best of my code emulation abilities is that
old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
and this is suboptimal, because it would be better for the new chattr tool
to get -EINVAL when trying to set new xflags and mask on an old kernel.

It is true that the new chattr can call the old FS_IOC_FSGETXATTR
ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
so I agree that a new ioctl is not absolutely necessary,
but I still believe that it is a better API design.

Would love to hear what other fs developers prefer.

Thanks,
Amir.

Re: [RFC PATCH 2/4] fs: Extend FS_IOC_FS[GS]ETXATTR API for Windows attributes

[Pali Rohár]

On Sunday 16 February 2025 21:34:42 Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 9:01 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Sunday 16 February 2025 20:55:09 Amir Goldstein wrote:
> > > On Sun, Feb 16, 2025 at 5:42 PM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > struct fsxattr has 8 reserved padding bytes. Use these bytes for defining
> > > > new fields fsx_xflags2, fsx_xflags2_mask and fsx_xflags_mask in backward
> > > > compatible manner. If the new FS_XFLAG_HASEXTFIELDS flag in fsx_xflags is
> > > > not set then these new fields are treated as not present, like before this
> > > > change.
> > > >
> > > > New field fsx_xflags_mask for SET operation specifies which flags in
> > > > fsx_xflags are going to be changed. This would allow userspace application
> > > > to change just subset of all flags. For GET operation this field specifies
> > > > which FS_XFLAG_* flags are supported by the file.
> > > >
> > > > New field fsx_xflags2 specify new flags FS_XFLAG2_* which defines some of
> > > > Windows FILE_ATTRIBUTE_* attributes, which are mostly not going to be
> > > > interpreted or used by the kernel, and are mostly going to be used by
> > > > userspace. Field fsx_xflags2_mask then specify mask for them.
> > > >
> > > > This change defines just API without filesystem support for them. These
> > > > attributes can be implemented later for Windows filesystems like FAT, NTFS,
> > > > exFAT, UDF, SMB, NFS4 which all native storage for those attributes (or at
> > > > least some subset of them).
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >  include/uapi/linux/fs.h | 36 +++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 31 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > > > index 367bc5289c47..93e947d6e604 100644
> > > > --- a/include/uapi/linux/fs.h
> > > > +++ b/include/uapi/linux/fs.h
> > > > @@ -145,15 +145,26 @@ struct fsxattr {
> > > >         __u32           fsx_nextents;   /* nextents field value (get)   */
> > > >         __u32           fsx_projid;     /* project identifier (get/set) */
> > > >         __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > > > -       unsigned char   fsx_pad[8];
> > > > +       __u16           fsx_xflags2;    /* xflags2 field value (get/set)*/
> > > > +       __u16           fsx_xflags2_mask;/*mask for xflags2 (get/set)*/
> > > > +       __u32           fsx_xflags_mask;/* mask for xflags (get/set)*/
> > > > +       /*
> > > > +        * For FS_IOC_FSSETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> > > > +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags from fsx_xflags
> > > > +        * and fsx_xflags2 fields are going to be changed.
> > > > +        *
> > > > +        * For FS_IOC_FSGETXATTR ioctl, fsx_xflags_mask and fsx_xflags2_mask
> > > > +        * fields specify which FS_XFLAG_* and FS_XFLAG2_* flags are supported.
> > > > +        */
> > > >  };
> > > >
> > > >  /*
> > > > - * Flags for the fsx_xflags field
> > > > + * Flags for the fsx_xflags and fsx_xflags_mask fields
> > > >   */
> > > >  #define FS_XFLAG_REALTIME      0x00000001      /* data in realtime volume */
> > > >  #define FS_XFLAG_PREALLOC      0x00000002      /* preallocated file extents */
> > > > -#define FS_XFLAG_IMMUTABLE     0x00000008      /* file cannot be modified */
> > > > +#define FS_XFLAG_IMMUTABLEUSER 0x00000004      /* file cannot be modified, changing this bit does not require CAP_LINUX_IMMUTABLE, equivalent of Windows FILE_ATTRIBUTE_READONLY */
> > >
> > > So why not call it FS_XFLAG2_READONLY? IDGI
> >
> > Just because to show that these two flags are similar, just one is for
> > root (or CAP_LINUX_IMMUTABLE) and another is for normal user.
> >
> > For example FreeBSD has also both flags (one for root and one for user)
> > and uses names SF_IMMUTABLE and UF_IMMUTABLE.
> >
> > For me having FS_XFLAG_IMMUTABLE and FS_XFLAG2_READONLY sounds less
> > clear, and does not explain how these two flags differs.
> >
> 
> Yes, I understand, but I do not agree.
> 
> What is your goal here?
> 
> Do you want to implement FreeBSD UF_IMMUTABLE?
> maybe UF_APPEND as well?
> Did anyone ask for this functionality?
> Not that I know of.

None of those.

> The requirement is to implement an API to the functionality known
> as READONLY in SMB and NTFS. Right?

Yes. But Linux is already calling this functionality as "immutable", not
as "readonly". That is why I thought that using existing name is better
than inventing a new name for some existing functionality.

> TBH, I did not study the semantics of READONLY, but I had a
> strong feeling that if we looked closely, we will find that other things are
> possible to do with READONLY files that are not possible with IMMUTABLE
> files. So I asked ChatGPT and it told me that all these can be changed:
> 1. File Attributes (Hidden, System, Archive, or Indexed).
> 2. Permissions (ACL - Access Control List)
> 3. Timestamps
> 4. Alternate Data Streams (ADS)
> 
> I do not know if ChatGPT is correct

Do not trust ChatGPT.

Modification of main data stream (= file content) or alternate data
streams is not possible.

Changing of file or extended attributes is possible.
Timestamp is a file attribute. xattrs are extended attributes.

About ACL I was not sure. But now I tried it and changing ACL is
possible even with readonly attribute set.

So 1. 2. and 3. is possible to change. 4. not.

> but it also told me that a READONLY
> file can be deleted (without removing the flag first).

That is wrong. File cannot be deleted if the READONLY attribute is set.
You first need to clear the READONLY flag. This is one of the main point
of READONLY attribute.

For example cifs.ko for unlink syscall issue SMB removal and if it is
failing due to readonly attribute, it clears it and try removal again.

> This is very very far from what IS_IMMUTABLE is.
> IS_IMMUTABLE is immutable to any metadata change.

I was always looking at this readonly attribute as IMMUTABLE as it
prevents modification of file content and prevents unlinking the file.

For me it was always very similar to immutable. But if you think that
changing ACL and file/extended attributes is the reason why it should
not be called immutable, then I'm fine. Maybe it needs better
documentation and explanation what each of those two flags can and
cannot do.

> Thanks,
> Amir.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Pali Rohár]

On Sunday 16 February 2025 21:43:02 Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > >
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > >
> > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > which use that flag?
> > >
> > > As far as I can tell, after fileattr_fill_xflags() call in
> > > ioctl_fssetxattr(), the call
> > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > >
> > > However, unlike the legacy API, we now have an opportunity to deal with
> > > EXT4_FL_USER_MODIFIABLE better than this:
> > >         /*
> > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > >          * more restrictive than just silently masking off visible but
> > >          * not settable flags as we always did.
> > >          */
> > >
> > > if we have the xflags_mask in the new API (not only the xflags) then
> > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > ext4_fileattr_set() can verify that
> > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > >
> > > However, Pali, this is an important point that your RFC did not follow -
> > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > (and other fs) does not return any error for unknown xflags, it just
> > > ignores them.
> > >
> > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > before adding support to ANY new xflags, whether they are mapped to
> > > existing flags like in this patch or are completely new xflags.
> > >
> > > Thanks,
> > > Amir.
> >
> > But xflags_mask is available in this new API. It is available if the
> > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > mentioned above can be included into this new API.
> >
> > Or I'm missing something?
> 
> Yes, you are missing something very fundamental to backward compat API -
> You cannot change the existing kernels.
> 
> You should ask yourself one question:
> What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> on an existing old kernel with the new extended flags?
> 
> The answer, to the best of my code emulation abilities is that
> old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> and this is suboptimal, because it would be better for the new chattr tool
> to get -EINVAL when trying to set new xflags and mask on an old kernel.
> 
> It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,

Yes, this was my intention how the backward and forward compatibility
will work. I thought that reusing existing IOCTL is better than creating
new IOCTL and duplicating functionality.

> so I agree that a new ioctl is not absolutely necessary,
> but I still believe that it is a better API design.

If it is a bad idea then for sure I can prepare new IOCTL and move all
new functionality only into the new IOCTL, no problem.

> Would love to hear what other fs developers prefer.
> 
> Thanks,
> Amir.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Amir Goldstein]

On Sun, Feb 16, 2025 at 10:17 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Sunday 16 February 2025 21:43:02 Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > > >
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > >
> > > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > > which use that flag?
> > > >
> > > > As far as I can tell, after fileattr_fill_xflags() call in
> > > > ioctl_fssetxattr(), the call
> > > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > > >
> > > > However, unlike the legacy API, we now have an opportunity to deal with
> > > > EXT4_FL_USER_MODIFIABLE better than this:
> > > >         /*
> > > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > > >          * more restrictive than just silently masking off visible but
> > > >          * not settable flags as we always did.
> > > >          */
> > > >
> > > > if we have the xflags_mask in the new API (not only the xflags) then
> > > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > > ext4_fileattr_set() can verify that
> > > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > > >
> > > > However, Pali, this is an important point that your RFC did not follow -
> > > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > > (and other fs) does not return any error for unknown xflags, it just
> > > > ignores them.
> > > >
> > > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > > before adding support to ANY new xflags, whether they are mapped to
> > > > existing flags like in this patch or are completely new xflags.
> > > >
> > > > Thanks,
> > > > Amir.
> > >
> > > But xflags_mask is available in this new API. It is available if the
> > > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > > mentioned above can be included into this new API.
> > >
> > > Or I'm missing something?
> >
> > Yes, you are missing something very fundamental to backward compat API -
> > You cannot change the existing kernels.
> >
> > You should ask yourself one question:
> > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > on an existing old kernel with the new extended flags?
> >
> > The answer, to the best of my code emulation abilities is that
> > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > and this is suboptimal, because it would be better for the new chattr tool
> > to get -EINVAL when trying to set new xflags and mask on an old kernel.
> >
> > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
>
> Yes, this was my intention how the backward and forward compatibility
> will work. I thought that reusing existing IOCTL is better than creating
> new IOCTL and duplicating functionality.
>
> > so I agree that a new ioctl is not absolutely necessary,
> > but I still believe that it is a better API design.
>
> If it is a bad idea then for sure I can prepare new IOCTL and move all
> new functionality only into the new IOCTL, no problem.
>

Well, there is at least one flaw in using the get ioctl for support detection,
as Eric pointed out -
the settable xflags set is a subset of the gettable flags set.

Let's ask some stake holders shall we?

Ted, Darrick, Eric,

What is your opinion on the plan presented in this patch set to extend the API:

1. Add some of the *_FL flags to FS_XFLAG_COMMON [*]
2. Add fsx_xflags2 for more xflags
3. Add fsx_xflags{,2}_mask to declare the supported in/out xflags
4. Should we use the existing FS_IOC_FSSETXATTR ioctl which ignores
   setting unknown flags or define a new v2 ioctl FS_IOC_SETFSXATTR_V2
   which properly fails on unknown flags [**]

[*] we can consider adding all of the flags used by actively maintained fs to
    FS_XFLAGS_COMMON, something like the set of F2FS_GETTABLE_FS_FL,
    maybe even split them to FS_XFLAGS_COMMON_[GS]ETTABLE
[**] we can either return EINVAL for unknown flags or make the ioctl _IOWR
     and return the set of flags that were not ignored

Thanks,
Amir.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Dave Chinner]

On Sun, Feb 16, 2025 at 09:43:02PM +0100, Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > >
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > >
> > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > which use that flag?
> > >
> > > As far as I can tell, after fileattr_fill_xflags() call in
> > > ioctl_fssetxattr(), the call
> > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > >
> > > However, unlike the legacy API, we now have an opportunity to deal with
> > > EXT4_FL_USER_MODIFIABLE better than this:
> > >         /*
> > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > >          * more restrictive than just silently masking off visible but
> > >          * not settable flags as we always did.
> > >          */
> > >
> > > if we have the xflags_mask in the new API (not only the xflags) then
> > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > ext4_fileattr_set() can verify that
> > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > >
> > > However, Pali, this is an important point that your RFC did not follow -
> > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > (and other fs) does not return any error for unknown xflags, it just
> > > ignores them.
> > >
> > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > before adding support to ANY new xflags, whether they are mapped to
> > > existing flags like in this patch or are completely new xflags.
> > >
> > > Thanks,
> > > Amir.
> >
> > But xflags_mask is available in this new API. It is available if the
> > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > mentioned above can be included into this new API.
> >
> > Or I'm missing something?
> 
> Yes, you are missing something very fundamental to backward compat API -
> You cannot change the existing kernels.
> 
> You should ask yourself one question:
> What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> on an existing old kernel with the new extended flags?

It should ignore all the things it does not know about.

But the correct usage of FS_IOC_FSSETXATTR is to call
FS_IOC_FSGETXATTR to initialise the structure with all the current
inode state. In this situation, the fsx.fsx_xflags field needs to
return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
knows that it can set/clear the MS windows attr flags.  Hence if the
filesystem supports windows attributes, we can require them to
-support the entire set-.

i.e. if an attempt to set a win attr that the filesystem cannot
implement (e.g. compression) then it returns an error (-EINVAL),
otherwise it will store the attr and perform whatever operation it
requires.

IMO, the whole implementation in the patchset is wrong - there is no
need for a new xflags2 field, and there is no need for whacky field
masks or anything like that. All it needs is a single bit to
indicate if the windows attributes are supported, and they are all
implemented as normal FS_XFLAG fields in the fsx_xflags field.

> The answer, to the best of my code emulation abilities is that
> old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> and this is suboptimal, because it would be better for the new chattr tool
> to get -EINVAL when trying to set new xflags and mask on an old kernel.

What new chattr tool? I would expect that xfs_io -c "chattr ..."
will be extended to support all these new fields because that is the
historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
support in fstests. I would also expect that the e2fsprogs chattr(1)
program to grow support for the new FS_XFLAGS bits as well...

> It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> so I agree that a new ioctl is not absolutely necessary,
> but I still believe that it is a better API design.

This is how FS{GS}ETXATTR interface has always worked. You *must*
do a GET before a SET so that the fsx structure has been correctly
initialised so the SET operation makes only the exact change being
asked for.

This makes the -API pair- backwards compatible by allowing struct
fsxattr feature bits to be reported in the GET operation. If the
feature bit is set, then those optional fields can be set. If the
feature bit is not set by the GET operation, then if you try to use
it on a SET operation you'll either get an error or it will be
silently ignored.

> Would love to hear what other fs developers prefer.

Do not overcomplicate the problem. Use the interface as intended:
GET then SET, and GET returns feature bits in the xflags field to
indicate what is valid in the overall struct fsxattr. It's trivial
to probe for native fs support of windows attributes like this. It
also allows filesystems to be converted one at a time to support the
windows attributes (e.g. as they have on-disk format support for
them added). Applications like Samba can then switch behaviours
based on what they probe from the underlying filesystem...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Amir Goldstein]

On Tue, Feb 18, 2025 at 2:33 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Sun, Feb 16, 2025 at 09:43:02PM +0100, Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > > >
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > >
> > > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > > which use that flag?
> > > >
> > > > As far as I can tell, after fileattr_fill_xflags() call in
> > > > ioctl_fssetxattr(), the call
> > > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > > >
> > > > However, unlike the legacy API, we now have an opportunity to deal with
> > > > EXT4_FL_USER_MODIFIABLE better than this:
> > > >         /*
> > > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > > >          * more restrictive than just silently masking off visible but
> > > >          * not settable flags as we always did.
> > > >          */
> > > >
> > > > if we have the xflags_mask in the new API (not only the xflags) then
> > > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > > ext4_fileattr_set() can verify that
> > > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > > >
> > > > However, Pali, this is an important point that your RFC did not follow -
> > > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > > (and other fs) does not return any error for unknown xflags, it just
> > > > ignores them.
> > > >
> > > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > > before adding support to ANY new xflags, whether they are mapped to
> > > > existing flags like in this patch or are completely new xflags.
> > > >
> > > > Thanks,
> > > > Amir.
> > >
> > > But xflags_mask is available in this new API. It is available if the
> > > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > > mentioned above can be included into this new API.
> > >
> > > Or I'm missing something?
> >
> > Yes, you are missing something very fundamental to backward compat API -
> > You cannot change the existing kernels.
> >
> > You should ask yourself one question:
> > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > on an existing old kernel with the new extended flags?
>
> It should ignore all the things it does not know about.
>

I don't know about "should" but it certainly does, that's why I was
wondering if a new fresh ioctl was in order before extending to new flags.

> But the correct usage of FS_IOC_FSSETXATTR is to call
> FS_IOC_FSGETXATTR to initialise the structure with all the current
> inode state.

Yeh, this is how the API is being used by exiting xfs_io chattr.
It does not mean that this is a good API.

> In this situation, the fsx.fsx_xflags field needs to
> return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
> knows that it can set/clear the MS windows attr flags.  Hence if the
> filesystem supports windows attributes, we can require them to
> -support the entire set-.
>
> i.e. if an attempt to set a win attr that the filesystem cannot
> implement (e.g. compression) then it returns an error (-EINVAL),
> otherwise it will store the attr and perform whatever operation it
> requires.
>

I prefer not to limit the discussion to new "win" attributes,
especially when discussing COMPR/ENCRYPT flags which are existing
flags that are also part of the win attributes.

To put it another way, suppose Pali did not come forward with a patch set
to add win attribute control to smb, but to add ENCRYPT support to xfs.
What would have been your prefered way to set the ENCRYPT flag in xfs?
via FS_IOC_SETFLAGS or by extending FS_IOC_FSSETXATTR?
and if the latter, then how would xfs_io chattr be expected to know if
setting the ENCRYPT flag is supported or not?

> IMO, the whole implementation in the patchset is wrong - there is no
> need for a new xflags2 field,

xflags2 is needed to add more bits. I am not following.

> and there is no need for whacky field
> masks or anything like that. All it needs is a single bit to
> indicate if the windows attributes are supported, and they are all
> implemented as normal FS_XFLAG fields in the fsx_xflags field.
>

Sorry, I did not understand your vision about the API.
On the one hand, you are saying that there is no need for xflags2.
On the other hand, that new flags should be treated differently than
old flags (FS_XFLAGS_HAS_WIN_ATTRS).
Either I did not understand what you mean or the documentation of
what you are proposing sounds terribly confusing to me.

> > The answer, to the best of my code emulation abilities is that
> > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > and this is suboptimal, because it would be better for the new chattr tool
> > to get -EINVAL when trying to set new xflags and mask on an old kernel.
>
> What new chattr tool? I would expect that xfs_io -c "chattr ..."
> will be extended to support all these new fields because that is the
> historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
> support in fstests. I would also expect that the e2fsprogs chattr(1)
> program to grow support for the new FS_XFLAGS bits as well...
>

That's exactly what I meant by "new chattr tool".
when user executes chattr +i or xfs_io -c "chattr +i"
user does not care which ioctl is used and it is best if both
utils will support the entire set of modern flags with the new ioctls
so that eventually (after old fs are deprecated) the old ioctl may also
be deprecated.

> > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> > so I agree that a new ioctl is not absolutely necessary,
> > but I still believe that it is a better API design.
>
> This is how FS{GS}ETXATTR interface has always worked. You *must*
> do a GET before a SET so that the fsx structure has been correctly
> initialised so the SET operation makes only the exact change being
> asked for.
>
> This makes the -API pair- backwards compatible by allowing struct
> fsxattr feature bits to be reported in the GET operation. If the
> feature bit is set, then those optional fields can be set. If the
> feature bit is not set by the GET operation, then if you try to use
> it on a SET operation you'll either get an error or it will be
> silently ignored.
>

Yes, I know. I still think that this is a poor API design pattern.
Imagine that people will be interested to include the fsxattr
in rsync or such (it has been mentioned in the context of this effort)
The current API is pretty useless for backup/restore and for
copying supported attributes across filesystems.

BTW, the internal ->fileattr_[gs]et() vfs API was created so that
overlayfs could copy flags between filesystems on copy-up,
but right now it only copies common flags.

Adding a support mask to the API will allow smarter copy of
supported attributes between filesystems that have a more
specific common set of supported flags.

> > Would love to hear what other fs developers prefer.
>
> Do not overcomplicate the problem. Use the interface as intended:
> GET then SET, and GET returns feature bits in the xflags field to
> indicate what is valid in the overall struct fsxattr. It's trivial
> to probe for native fs support of windows attributes like this. It
> also allows filesystems to be converted one at a time to support the
> windows attributes (e.g. as they have on-disk format support for
> them added). Applications like Samba can then switch behaviours
> based on what they probe from the underlying filesystem...
>

OK, so we have two opinions.
Debating at design time is much better than after API is released...

I'd still like to hear from other stakeholders before we perpetuate
the existing design pattern which requires apps to GET before SET
and treat new (WIN) flags differently than old flags.

Thanks,
Amir.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Pali Rohár]

On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> On Tue, Feb 18, 2025 at 2:33 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Sun, Feb 16, 2025 at 09:43:02PM +0100, Amir Goldstein wrote:
> > > On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > > >
> > > > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > > > >
> > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > >
> > > > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > > > which use that flag?
> > > > >
> > > > > As far as I can tell, after fileattr_fill_xflags() call in
> > > > > ioctl_fssetxattr(), the call
> > > > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > > > >
> > > > > However, unlike the legacy API, we now have an opportunity to deal with
> > > > > EXT4_FL_USER_MODIFIABLE better than this:
> > > > >         /*
> > > > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > > > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > > > >          * more restrictive than just silently masking off visible but
> > > > >          * not settable flags as we always did.
> > > > >          */
> > > > >
> > > > > if we have the xflags_mask in the new API (not only the xflags) then
> > > > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > > > ext4_fileattr_set() can verify that
> > > > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > > > >
> > > > > However, Pali, this is an important point that your RFC did not follow -
> > > > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > > > (and other fs) does not return any error for unknown xflags, it just
> > > > > ignores them.
> > > > >
> > > > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > > > before adding support to ANY new xflags, whether they are mapped to
> > > > > existing flags like in this patch or are completely new xflags.
> > > > >
> > > > > Thanks,
> > > > > Amir.
> > > >
> > > > But xflags_mask is available in this new API. It is available if the
> > > > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > > > mentioned above can be included into this new API.
> > > >
> > > > Or I'm missing something?
> > >
> > > Yes, you are missing something very fundamental to backward compat API -
> > > You cannot change the existing kernels.
> > >
> > > You should ask yourself one question:
> > > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > > on an existing old kernel with the new extended flags?
> >
> > It should ignore all the things it does not know about.
> >
> 
> I don't know about "should" but it certainly does, that's why I was
> wondering if a new fresh ioctl was in order before extending to new flags.

Not all filesystems ignore unknown flags. For example fs/ntfs3/file.c
function ntfs_fileattr_set() already contains:

	if (fileattr_has_fsx(fa))
		return -EOPNOTSUPP;

	if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | FS_COMPR_FL))
		return -EOPNOTSUPP;

So if it is called with FS_XFLAG_HASEXTFIELDS (required for fsx_xflags2)
then kernel already returns -EOPNOTSUPP.

But some filesystems like ext4 does not contain that
fileattr_has_fsx(fa) check.

> > But the correct usage of FS_IOC_FSSETXATTR is to call
> > FS_IOC_FSGETXATTR to initialise the structure with all the current
> > inode state.
> 
> Yeh, this is how the API is being used by exiting xfs_io chattr.
> It does not mean that this is a good API.
> 
> > In this situation, the fsx.fsx_xflags field needs to
> > return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
> > knows that it can set/clear the MS windows attr flags.

It does not say which windows attributes. For example UDF filesystem
supports only HIDDEN windows attribute. FAT32 supports more (+ READONLY,
SYSTEM, ARCHIVE), NTFS even more (+ TEMPORARY, OFFLINE, ...). And SMB
and ReFS even more (+ INTEGRITY). And then we have there NFS4 which
supports another subset of those windows attributes.

So how would userspace know if the OFFLINE attribute is supported or
not? OFFLINE is one which more people mentioned in these discussion that
want to see support for it.

That is why I introduced new mask field, which in FS_IOC_FSGETXATTR
ioctl response is filled with supported attributes and each filesystem
will return what it supports.

> > Hence if the
> > filesystem supports windows attributes, we can require them to
> > -support the entire set-.

We cannot because even the most commonly used NTFS filesystem does not
support the entire set.

> >
> > i.e. if an attempt to set a win attr that the filesystem cannot
> > implement (e.g. compression) then it returns an error (-EINVAL),
> > otherwise it will store the attr and perform whatever operation it
> > requires.
> >
> 
> I prefer not to limit the discussion to new "win" attributes,
> especially when discussing COMPR/ENCRYPT flags which are existing
> flags that are also part of the win attributes.

+1

> To put it another way, suppose Pali did not come forward with a patch set
> to add win attribute control to smb, but to add ENCRYPT support to xfs.

I agree. I chose smb in this RFC as an example to demonstrate as much
attribute as possible.

Sure I could choice xfs or udf in RFC to implement just one attribute
(ENCRYPT in xfs, HIDDEN in udf). But such example with just one
attribute would be less useful for demonstration.

> What would have been your prefered way to set the ENCRYPT flag in xfs?
> via FS_IOC_SETFLAGS or by extending FS_IOC_FSSETXATTR?
> and if the latter, then how would xfs_io chattr be expected to know if
> setting the ENCRYPT flag is supported or not?
> 
> > IMO, the whole implementation in the patchset is wrong - there is no
> > need for a new xflags2 field,
> 
> xflags2 is needed to add more bits. I am not following.

Theoretically it is possible to move all bits from xflags2 into xflags.
But if I'm counting correctly then there would be just one or two free
bits in xflags.

> > and there is no need for whacky field
> > masks or anything like that. All it needs is a single bit to
> > indicate if the windows attributes are supported, and they are all
> > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> >

If MS adds 3 new attributes then we cannot add them to fsx_xflags
because all bits of fsx_xflags would be exhausted.

> Sorry, I did not understand your vision about the API.
> On the one hand, you are saying that there is no need for xflags2.
> On the other hand, that new flags should be treated differently than
> old flags (FS_XFLAGS_HAS_WIN_ATTRS).
> Either I did not understand what you mean or the documentation of
> what you are proposing sounds terribly confusing to me.

I understood it as:
- add FS_XFLAGS_HAS_WIN_ATTRS bit into fsx_xflags
- for every windows attribute add FS_XFLAG_<attr>
That is for sure possible but the space of fsx_xflags would be
exhausted.

> > > The answer, to the best of my code emulation abilities is that
> > > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > > and this is suboptimal, because it would be better for the new chattr tool
> > > to get -EINVAL when trying to set new xflags and mask on an old kernel.
> >
> > What new chattr tool? I would expect that xfs_io -c "chattr ..."
> > will be extended to support all these new fields because that is the
> > historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
> > support in fstests. I would also expect that the e2fsprogs chattr(1)
> > program to grow support for the new FS_XFLAGS bits as well...
> >
> 
> That's exactly what I meant by "new chattr tool".
> when user executes chattr +i or xfs_io -c "chattr +i"
> user does not care which ioctl is used and it is best if both
> utils will support the entire set of modern flags with the new ioctls
> so that eventually (after old fs are deprecated) the old ioctl may also
> be deprecated.
> 
> > > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> > > so I agree that a new ioctl is not absolutely necessary,
> > > but I still believe that it is a better API design.
> >
> > This is how FS{GS}ETXATTR interface has always worked. You *must*
> > do a GET before a SET so that the fsx structure has been correctly
> > initialised so the SET operation makes only the exact change being
> > asked for.
> >
> > This makes the -API pair- backwards compatible by allowing struct
> > fsxattr feature bits to be reported in the GET operation. If the
> > feature bit is set, then those optional fields can be set. If the
> > feature bit is not set by the GET operation, then if you try to use
> > it on a SET operation you'll either get an error or it will be
> > silently ignored.

I think that this is perfectly fine, it is possible to implement it.
Personally I do not see a problem with this option, just it needs to be
decided what people wants.

> Yes, I know. I still think that this is a poor API design pattern.

I agree that this is also not my preferred API design. But I also
understand the argument "it is already there, so follow it".

> Imagine that people will be interested to include the fsxattr
> in rsync or such (it has been mentioned in the context of this effort)
> The current API is pretty useless for backup/restore and for
> copying supported attributes across filesystems.

I would not say that it is useless. It is still better than nothing. And
this API also allows to fully implement backup/restore functionality.
Just would require more calls:

    if (ioctl(orig_fd, FS_IOC_FSGETXATTR, &orig_attrs) != 0) goto fail;
    if (ioctl(back_fd, FS_IOC_FSGETXATTR, &back_attrs) != 0) gott fail;
    back_attrs.fsx_xflags = orig_attrs.fsx_xflags
    if (ioctl(back_fd, FS_IOC_FSSETXATTR, &back_attrs) != 0) goto fail;
    if (ioctl(back_fd, FS_IOC_FSGETXATTR, &back_attrs) != 0) goto fail;
    if (back_attrs.fsx_xflags != orig_attrs.fsx_xflags) goto fail;

> BTW, the internal ->fileattr_[gs]et() vfs API was created so that
> overlayfs could copy flags between filesystems on copy-up,
> but right now it only copies common flags.

This can be improved/extended.

> Adding a support mask to the API will allow smarter copy of
> supported attributes between filesystems that have a more
> specific common set of supported flags.
> 
> > > Would love to hear what other fs developers prefer.
> >
> > Do not overcomplicate the problem. Use the interface as intended:
> > GET then SET, and GET returns feature bits in the xflags field to
> > indicate what is valid in the overall struct fsxattr. It's trivial
> > to probe for native fs support of windows attributes like this. It
> > also allows filesystems to be converted one at a time to support the
> > windows attributes (e.g. as they have on-disk format support for
> > them added). Applications like Samba can then switch behaviours
> > based on what they probe from the underlying filesystem...
> >
> 
> OK, so we have two opinions.
> Debating at design time is much better than after API is released...
> 
> I'd still like to hear from other stakeholders before we perpetuate
> the existing design pattern which requires apps to GET before SET
> and treat new (WIN) flags differently than old flags.
> 
> Thanks,
> Amir.

I would like to know how to move forward. If we want to remove mask
fields or let them here. If we want to move all xflags2 to xflags or let
them split. I think that all of those are options which I can implement.
I'm open for any variant.

Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
attribute support is not enough, as it would not say anything useful for
userspace.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Dave Chinner]

On Tue, Feb 18, 2025 at 10:13:46AM +0100, Amir Goldstein wrote:
> On Tue, Feb 18, 2025 at 2:33 AM Dave Chinner <david@fromorbit.com> wrote:
> > > Yes, you are missing something very fundamental to backward compat API -
> > > You cannot change the existing kernels.
> > >
> > > You should ask yourself one question:
> > > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > > on an existing old kernel with the new extended flags?
> >
> > It should ignore all the things it does not know about.
> 
> I don't know about "should" but it certainly does, that's why I was
> wondering if a new fresh ioctl was in order before extending to new flags.
> 
> > But the correct usage of FS_IOC_FSSETXATTR is to call
> > FS_IOC_FSGETXATTR to initialise the structure with all the current
> > inode state.
> 
> Yeh, this is how the API is being used by exiting xfs_io chattr.
> It does not mean that this is a good API.
> 
> > In this situation, the fsx.fsx_xflags field needs to
> > return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
> > knows that it can set/clear the MS windows attr flags.  Hence if the
> > filesystem supports windows attributes, we can require them to
> > -support the entire set-.
> >
> > i.e. if an attempt to set a win attr that the filesystem cannot
> > implement (e.g. compression) then it returns an error (-EINVAL),
> > otherwise it will store the attr and perform whatever operation it
> > requires.
> 
> I prefer not to limit the discussion to new "win" attributes,
> especially when discussing COMPR/ENCRYPT flags which are existing
> flags that are also part of the win attributes.

Not sure I understand why you think I don't know this, and why it
is a problem in any way?

> To put it another way, suppose Pali did not come forward with a patch set
> to add win attribute control to smb, but to add ENCRYPT support to xfs.
> What would have been your prefered way to set the ENCRYPT flag in xfs?

<sigh>

We don't have encryption on XFS yet, so we'd completely ignore it.
It would never be set in the GET op, and it would be ignored on the
SET op.

> via FS_IOC_SETFLAGS or by extending FS_IOC_FSSETXATTR?
> and if the latter, then how would xfs_io chattr be expected to know if
> setting the ENCRYPT flag is supported or not?

chattr is not the interface for enabling encryption!

FS_IOC_FSGETXATTR returns various status information, and a subset
of that information can be used for changing inode state with
the FS_IOC_FSSETXATTR command.

The reason SET ignores stuff it can't set is because it expects that
GET->SET will result in flags being set that it can't actually
change, and so it ignores flags that cannot be set....

> > IMO, the whole implementation in the patchset is wrong - there is no
> > need for a new xflags2 field,
> 
> xflags2 is needed to add more bits. I am not following.

We've got a 13 or 14 free flag bits still remaining in fsx_xflags
before we need another flags field.

> > and there is no need for whacky field
> > masks or anything like that. All it needs is a single bit to
> > indicate if the windows attributes are supported, and they are all
> > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> >
> 
> Sorry, I did not understand your vision about the API.
> On the one hand, you are saying that there is no need for xflags2.

Because we have enough spare bits for all the new flags, yes?

> On the other hand, that new flags should be treated differently than
> old flags (FS_XFLAGS_HAS_WIN_ATTRS).

Yes, new flags can have different behaviour. If we tell userspace
that we support windows attributes, we can define whatever behaviour
we want when setting -those new flag fields-.

> Either I did not understand what you mean or the documentation of
> what you are proposing sounds terribly confusing to me.

Misunderstanding, I think.

> > > The answer, to the best of my code emulation abilities is that
> > > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > > and this is suboptimal, because it would be better for the new chattr tool
> > > to get -EINVAL when trying to set new xflags and mask on an old kernel.
> >
> > What new chattr tool? I would expect that xfs_io -c "chattr ..."
> > will be extended to support all these new fields because that is the
> > historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
> > support in fstests. I would also expect that the e2fsprogs chattr(1)
> > program to grow support for the new FS_XFLAGS bits as well...
> >
> 
> That's exactly what I meant by "new chattr tool".

"new chattr tool" implies a new implementation/binary, not modifying
the existing tools.

> when user executes chattr +i or xfs_io -c "chattr +i"
> user does not care which ioctl is used and it is best if both
> utils will support the entire set of modern flags with the new ioctls
> so that eventually (after old fs are deprecated) the old ioctl may also
> be deprecated.

We will never be able to deprecate/remove deprecate the existing
ioctls.


> > > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> > > so I agree that a new ioctl is not absolutely necessary,
> > > but I still believe that it is a better API design.
> >
> > This is how FS{GS}ETXATTR interface has always worked. You *must*
> > do a GET before a SET so that the fsx structure has been correctly
> > initialised so the SET operation makes only the exact change being
> > asked for.
> >
> > This makes the -API pair- backwards compatible by allowing struct
> > fsxattr feature bits to be reported in the GET operation. If the
> > feature bit is set, then those optional fields can be set. If the
> > feature bit is not set by the GET operation, then if you try to use
> > it on a SET operation you'll either get an error or it will be
> > silently ignored.
> >
> 
> Yes, I know. I still think that this is a poor API design pattern.
> Imagine that people will be interested to include the fsxattr
> in rsync or such (it has been mentioned in the context of this effort)
> The current API is pretty useless for backup/restore and for
> copying supported attributes across filesystems.

xfsrestore has been using this interface for backup/restore
for about 25 years now. It only uses the SET function, because the
dump format stores all the flags from the dump side GET operation
natively.

i.e. xfsdump returns all the FS_XFLAGS that it supports in bulkstat
output so that xfsdump can avoid needing to call GET on every inode
it is going to back up.

So, yeah, we've been using this get/set xattr interface successfully
for backup for a long, long time.

> BTW, the internal ->fileattr_[gs]et() vfs API was created so that
> overlayfs could copy flags between filesystems on copy-up,
> but right now it only copies common flags.

That's an implementation problem, not an API issue.

> Adding a support mask to the API will allow smarter copy of
> supported attributes between filesystems that have a more
> specific common set of supported flags.

I don't think such a static mask belongs in the GET/SET interface. If
overlay wants to know what features a filesytem supports so it can
find commonality, then the feature mask it should be calculated
once at mount time and not on every single operation...

> I'd still like to hear from other stakeholders before we perpetuate
> the existing design pattern which requires apps to GET before SET
> and treat new (WIN) flags differently than old flags.

I just don't see why we need -yet another- inode attribute userspace
API just to support a few new feature flags...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Dave Chinner]

On Tue, Feb 18, 2025 at 08:27:01PM +0100, Pali Rohár wrote:
> On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> > > and there is no need for whacky field
> > > masks or anything like that. All it needs is a single bit to
> > > indicate if the windows attributes are supported, and they are all
> > > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> > >
> 
> If MS adds 3 new attributes then we cannot add them to fsx_xflags
> because all bits of fsx_xflags would be exhausted.

And then we can discuss how to extend the fsxattr structure to
implement more flags.

In this scenario we'd also need another flag bit to indicate that
there is a second set of windows attributes that are supported...

i.e. this isn't a problem we need to solve right now.

> Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
> attribute support is not enough, as it would not say anything useful for
> userspace.

IDGI.

That flag is only needed to tell userspace "this filesystem supports
windows attributes". Then GET will return the ones that are set,
and SET will return -EINVAL for those that it can't set (e.g.
compress, encrypt). What more does userspace actually need?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Pali Rohár]

On Wednesday 19 February 2025 09:56:28 Dave Chinner wrote:
> On Tue, Feb 18, 2025 at 08:27:01PM +0100, Pali Rohár wrote:
> > On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> > > > and there is no need for whacky field
> > > > masks or anything like that. All it needs is a single bit to
> > > > indicate if the windows attributes are supported, and they are all
> > > > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> > > >
> > 
> > If MS adds 3 new attributes then we cannot add them to fsx_xflags
> > because all bits of fsx_xflags would be exhausted.
> 
> And then we can discuss how to extend the fsxattr structure to
> implement more flags.
> 
> In this scenario we'd also need another flag bit to indicate that
> there is a second set of windows attributes that are supported...
> 
> i.e. this isn't a problem we need to solve right now.

Ok, that is possible solution for now.

> > Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
> > attribute support is not enough, as it would not say anything useful for
> > userspace.
> 
> IDGI.
> 
> That flag is only needed to tell userspace "this filesystem supports
> windows attributes". Then GET will return the ones that are set,
> and SET will return -EINVAL for those that it can't set (e.g.
> compress, encrypt). What more does userspace actually need?

Userspace backup utility would like to backup as many attributes as
possible by what is supported by the target filesystem. What would such
utility would do if the target filesystem supports only HIDDEN
attribute, and source file has all windows attributes set? It would be
needed to issue 2*N syscalls in the worst case to set attributes.
It would be combination of GET+SET for every one windows attribute
because userspace does not know what is supported and what not.

IMHO this is suboptimal. If filesystem would provide API to get list of
supported attributes then this can be done by 2-3 syscalls.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Amir Goldstein]

On Wed, Feb 19, 2025 at 12:06 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 19 February 2025 09:56:28 Dave Chinner wrote:
> > On Tue, Feb 18, 2025 at 08:27:01PM +0100, Pali Rohár wrote:
> > > On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> > > > > and there is no need for whacky field
> > > > > masks or anything like that. All it needs is a single bit to
> > > > > indicate if the windows attributes are supported, and they are all
> > > > > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> > > > >
> > >
> > > If MS adds 3 new attributes then we cannot add them to fsx_xflags
> > > because all bits of fsx_xflags would be exhausted.
> >
> > And then we can discuss how to extend the fsxattr structure to
> > implement more flags.
> >
> > In this scenario we'd also need another flag bit to indicate that
> > there is a second set of windows attributes that are supported...
> >
> > i.e. this isn't a problem we need to solve right now.
>
> Ok, that is possible solution for now.
>
> > > Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
> > > attribute support is not enough, as it would not say anything useful for
> > > userspace.
> >
> > IDGI.
> >
> > That flag is only needed to tell userspace "this filesystem supports
> > windows attributes". Then GET will return the ones that are set,
> > and SET will return -EINVAL for those that it can't set (e.g.
> > compress, encrypt). What more does userspace actually need?

Let me state my opinion clearly.
I think this API smells.
I do not object to it, but I think we can do better.

I do however object to treating different flags in fsx_xflags
differently - this is unacceptable IMO.

The difference I am referring to is a nuance, but IMO an important one -

It's fine for GET to raise a flag "this filesystem does not accept SET
of any unknown flags".

It's not fine IMO for GET to raise a flag "this filesystem does not accept
SET of unknown flags except for a constant subset of flags that filesystem
may ignore".
It's not fine IMO, because it makes userspace life harder for no good reason.

This former still allows filesystems to opt-in one by one to the extended API,
but it does not assume anything about the subset of windows attributes
and legacy Linux attributes that need to be supported.

For example, adding support for set/get hidden/system/archive/readonly
fo fs/fat, does not require supporting all FS_XFLAG_COMMON by fs/fat
and an attempt to set unsupported FS_XFLAG_COMMON flags would
result in -EINVAL just as an attempt to set an unsupported win flag.

But if we agree on setting one special flag in GET to say "new SET
semantics", I do not understand the objection to fsx_xflags_mask.

Dave, if you actually object to fsx_xflags_mask please explain why.
"What more does userspace actually need?" is not a good enough
reason IMO. Userspace could make use of fsx_xflags_mask.

>
> Userspace backup utility would like to backup as many attributes as
> possible by what is supported by the target filesystem. What would such
> utility would do if the target filesystem supports only HIDDEN
> attribute, and source file has all windows attributes set? It would be
> needed to issue 2*N syscalls in the worst case to set attributes.
> It would be combination of GET+SET for every one windows attribute
> because userspace does not know what is supported and what not.
>
> IMHO this is suboptimal. If filesystem would provide API to get list of
> supported attributes then this can be done by 2-3 syscalls.

I agree that getting the "attributes supported by filesystem" is important
and even getting the "gettable" subset and "settable" subset and I also
agree with Dave that this could be done once and no need to do it for
every file (although different file types may support a different subsets).

Let's stop for a moment to talk about statx.

I think you should include a statx support path in your series - not later.
If anything, statx support for exporting those flags should be done
before adding the GET/SET API.

Why? because there is nothing controversial about it.
- add a bunch of new STATX_ATTR_ flags
- filesystems that support them will publish that in stx_attributes_mask
- COMPR/ENCRYPT are already exported in statx

With that, backup/sync programs are able to query filesystem support
even without fsx_xflags_mask.

I think this is a hacky way around a proper GET/SET API, but possible.

Thanks,
Amir.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Theodore Ts'o]

I think a few people were talking past each other, because there are two
fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later
started getting used by many other file systems, starting with
resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
that flags word were both the ioctl ABI and the on-disk encoding, and
because we were now allowing multiple file systems to allocate bits,
and we needed to avoid stepping on each other (for example since btrfs
started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
at least not for a publically exported flag).

So we started running out of space in the FS_FLAG_*_FL namespace, and
that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The
FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
positions, by my count.

As far as the arguments about "proper interface design", as far as
Linux is concerned, backwards compatibility trumps "we should have
done if it differently".  The one and only guarantee that we have that
FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.

The use case of "what if a backup program wants to backup the flags
and restore on a different file system" is one that hasn't been
considered, and I don't think any backup programs do it today.  For
that matter, some of the flags, such as the NODUMP flag, are designed
to be instructions to a dump/restore system, and not really one that
*should* be backed up.  Again, the only semantic that was guaranteed
is GETXATTR or GETXATTR followed by SETXATTR.

We can define some new interface for return what xflags are supported
by a particular file system.  This could either be the long-debated,
but never implemented statfsx() system call.  Or it could be extending
what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
fields in struct fsxattr.  This is arguably the simplest way of
dealing with the problem.

I suppose the field could double as the bitmask field when
FS_IOC_SETXATTR is called, but that just seems to be an overly complex
set of semantics.  If someone really wants to do that, I wouldn't
really complain, but then what we would actually call the field
"flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

				      	     - Ted

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Amir Goldstein]

On Fri, Feb 21, 2025 at 5:34 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> I think a few people were talking past each other, because there are two
> fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
> is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later
> started getting used by many other file systems, starting with
> resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
> that flags word were both the ioctl ABI and the on-disk encoding, and
> because we were now allowing multiple file systems to allocate bits,
> and we needed to avoid stepping on each other (for example since btrfs
> started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
> at least not for a publically exported flag).
>
> So we started running out of space in the FS_FLAG_*_FL namespace, and
> that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The
> FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
> positions, by my count.
>
> As far as the arguments about "proper interface design", as far as
> Linux is concerned, backwards compatibility trumps "we should have
> done if it differently".  The one and only guarantee that we have that
> FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.
>
> The use case of "what if a backup program wants to backup the flags
> and restore on a different file system" is one that hasn't been
> considered, and I don't think any backup programs do it today.  For
> that matter, some of the flags, such as the NODUMP flag, are designed
> to be instructions to a dump/restore system, and not really one that
> *should* be backed up.  Again, the only semantic that was guaranteed
> is GETXATTR or GETXATTR followed by SETXATTR.
>

Thanks for chiming in, Ted!
Notes from the original author of FS_IOC_EXT2_[GS]ETFLAGS
are valuable.

> We can define some new interface for return what xflags are supported
> by a particular file system.  This could either be the long-debated,
> but never implemented statfsx() system call.  Or it could be extending
> what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
> fields in struct fsxattr.  This is arguably the simplest way of
> dealing with the problem.
>

That is also what I think.
fsx_xflags_mask semantics for GET are pretty clear
and follows the established pattern of  stx_attributes_mask
Even if it is not mandatory for userspace, it can be useful.

I asked Dave if he objects to fsx_xflags_mask and got silence,
so IMO, if Pali wants to implement fsx_xflags_mask for the API
I see no reason to resist it.

> I suppose the field could double as the bitmask field when
> FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> set of semantics.  If someone really wants to do that, I wouldn't
> really complain, but then what we would actually call the field
> "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

If we follow the old rule of SET after GET should always work
then fsx_xflags_mask will always be a superset of fsx_xflags,
so I think it would be sane to return -EINVAL in the case
of (fsx_xflags_mask && fsx_xflags & ~fsx_xflags_mask),
which is anyway the correct behavior for filesystems when the
user is trying to set flags that the filesystem does not support.

As far as I could see, all in-tree filesystems behave this way
when the user is trying to set unsupported flags either via
FS_IOC_SETFLAGS or via FS_IOC_SETXATTR
except xfs, which ignores unsupported fsx_xflags from
FS_IOC_SETXATTR.

Changing the behavior of xfs to return -EINVAL for unsupported
fsx_xflags if fsx_xflags_mask is non zero is NOT going to break UAPI,
because as everyone keeps saying, the only guarantee from
FS_IOC_SETXATTR was that FS_IOC_SETXATTR after
FS_IOC_GETXATTR works and that guarantee will not be broken.

Thanks,
Amir.

Re: [RFC PATCH 3/4] fs: Implement support for fsx_xflags_mask, fsx_xflags2 and fsx_xflags2_mask into vfs

[Theodore Ts'o]

On Sun, Feb 16, 2025 at 05:40:28PM +0100, Pali Rohár wrote:
> This change adds support for new struct fileattr fields fsx_xflags_mask,
> fsx_xflags2 and fsx_xflags2_mask into FS_IOC_FSGETXATTR and
> FS_IOC_FSSETXATTR ioctls.

I don't think I saw an answer to this question (but the discussions of
the mail thread have really sprawled a bit and so it's hard to review
all of the messages in the thread) --- so.... what's your reason for
creating this new fsx_xflags2 field as opposed to just defining new
flags bits for fsx_xflags field?  There are plenty of unused bits in
the FS_XFLAG_* space.

Cheers,

					- Ted
					

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Feb 21, 2025, at 9:34 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> We can define some new interface for return what xflags are supported
> by a particular file system.  This could either be the long-debated,
> but never implemented statfsx() system call.  Or it could be extending
> what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
> fields in struct fsxattr.  This is arguably the simplest way of
> dealing with the problem.
> 
> I suppose the field could double as the bitmask field when
> FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> set of semantics.  If someone really wants to do that, I wouldn't
> really complain, but then what we would actually call the field
> "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

The nice thing about allowing the bitmask on SET to mean "only set/clear
the specified fields" is that this allows a race-free mechanism to change
the flags, whereas GET+SET could be racy between two callers.

I don't think the two uses are incompatible.  If called as GET+SET, where
the GET will return the flags+mask, then any flag bits set/cleared should
also be in mask when SET, and all of the other bits are reset to the same
value.  If called as "SET flags+mask" with a limited number of bits, only
those bits in mask would be set/cleared, and the other bits would be left
as-is.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Dave Chinner]

On Fri, Feb 21, 2025 at 11:34:43AM -0500, Theodore Ts'o wrote:
> I think a few people were talking past each other, because there are two
> fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
> is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later

I don't think anyone has been confusing the two - the entire
discussion has been about fsx_xflags and the struct fsxattr...

> started getting used by many other file systems, starting with
> resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
> that flags word were both the ioctl ABI and the on-disk encoding, and
> because we were now allowing multiple file systems to allocate bits,
> and we needed to avoid stepping on each other (for example since btrfs
> started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
> at least not for a publically exported flag).
> 
> So we started running out of space in the FS_FLAG_*_FL namespace, and
> that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The

No, that is most certainly not how this API came about. 

The FS_IOC_[GS]ETXATTR ioctls were first implement on IRIX close on
30 years ago. They were ported to Linux with the XFS linux port over
2 decades ago. Indeed, we've been using them for xfsdump/xfs_restore
since before XFS was ported to linux.

They got lifted to the VFS back in 2016 so that ext4 could use the
interface for getting/setting project IDs on files. This was done so
that existing userspace functionality for setting up
project/directory quotas on XFS could also be used on ext4.

> FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
> positions, by my count.
> 
> As far as the arguments about "proper interface design", as far as
> Linux is concerned, backwards compatibility trumps "we should have
> done if it differently".  The one and only guarantee that we have that
> FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.

That's a somewhat naive understanding of the overall API. The struct
fsxattr information is also directly exported to userspace via the
XFS blukstat ioctls. i.e. extent size hints, fsx_xflags, project
IDs, etc are all exported to userspace via multiple ioctl
interfaces.

This is all used by xfsdump/xfs_restore to be able to back up and
restore the inode state that is exposed/controlled by the
GET/SETXATTR interfaces.

> The use case of "what if a backup program wants to backup the flags
> and restore on a different file system" is one that hasn't been
> considered, and I don't think any backup programs do it today.

Wrong. As I've already said: we have been doing exactly this for 20+
years with xfsdump/restore.

xfsdump uses the bulkstat version of the GET interface, whilst
restore uses the FS_IOC_SETXATTR interface.

> For
> that matter, some of the flags, such as the NODUMP flag, are designed
> to be instructions to a dump/restore system, and not really one that
> *should* be backed up.

Yes. xfsdump sees this in the bulkstat flags field for the inode and
then omits the inode from the dump.

Further, xfs_fsr (the online file defragmenter for XFS) uses
bulkstat and looks at the FS_XFLAGS returned from bulkstat for each
inode it scans.

> Again, the only semantic that was guaranteed
> is GETXATTR or GETXATTR followed by SETXATTR.

For making a single delta state change, yes.

For the dump/restore case, calling SETXATTR on a newly created file
with a preconstructed struct fsxattr state retreived at dump time is
also supported.

This is not a general use case - it will destroy any existing state
that file was created with (e.g. override admin inheritence
settings) by overwriting it with the state from the backup.

It should also be noted that xfs_restore does this in two SETXATTR
calls, not one. i.e. it splits the set operation into a
pre-data restore SETXATTR, and one post-data restore SETXATTR.

Why?

Because stuff like extent size hints and realtime state needs to be
restored before any data is written whilst others can only be set
after the data has been written because they would otherwise prevent
data restoration:

/* extended inode flags that can only be set after all data
 * has been restored to a file.
 */
#define POST_DATA_XFLAGS        (XFS_XFLAG_IMMUTABLE |          \
                                  XFS_XFLAG_APPEND |            \
                                  XFS_XFLAG_SYNC)

Yup, you can't restore data to the file if it has already been
marked as immutable....

IOWs, any changes to the flag space also needs to be compatible with
the XFS bulkstat shadowing of the fsxattr fields+flags and the
existing usage of these APIs by xfsdump, xfs_restore and xfs_fsr.

> We can define some new interface for return what xflags are supported
> by a particular file system.

Why do we even care?

On the get side, it just doesn't matter - if the flag isn't set, it
either is not active or not supported. Either way, it doesn't
matter if there's a "this is supported mask".

On the set side, adding a mask isn't going to change historic
behaviour: existing applications will ignore the new mask because
they haven't been coded to understand it. And vice versa, an old
kernel will ignore the feature mask if the app uses it because it
ignores unknown flags/fields.

IOWs, adding a feature mask doesn't solve any of the problems
related to forwards/backwards compatibility of new features, and so
we are back to needing to use the API as a GET/SET pair where the
GET sets all the known state correctly such that a SET operation
will either do nothing, change the state or return an error because
an invalid combination of known parameters was passed.

> I suppose the field could double as the bitmask field when
> FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> set of semantics.  If someone really wants to do that, I wouldn't
> really complain, but then what we would actually call the field
> "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

That effectively prevents the existing dump/restore usage of the
API.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Dave Chinner]

On Fri, Feb 21, 2025 at 06:11:43PM +0100, Amir Goldstein wrote:
> On Fri, Feb 21, 2025 at 5:34 PM Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > I think a few people were talking past each other, because there are two
> > fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
> > is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later
> > started getting used by many other file systems, starting with
> > resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
> > that flags word were both the ioctl ABI and the on-disk encoding, and
> > because we were now allowing multiple file systems to allocate bits,
> > and we needed to avoid stepping on each other (for example since btrfs
> > started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
> > at least not for a publically exported flag).
> >
> > So we started running out of space in the FS_FLAG_*_FL namespace, and
> > that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The
> > FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
> > positions, by my count.
> >
> > As far as the arguments about "proper interface design", as far as
> > Linux is concerned, backwards compatibility trumps "we should have
> > done if it differently".  The one and only guarantee that we have that
> > FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.
> >
> > The use case of "what if a backup program wants to backup the flags
> > and restore on a different file system" is one that hasn't been
> > considered, and I don't think any backup programs do it today.  For
> > that matter, some of the flags, such as the NODUMP flag, are designed
> > to be instructions to a dump/restore system, and not really one that
> > *should* be backed up.  Again, the only semantic that was guaranteed
> > is GETXATTR or GETXATTR followed by SETXATTR.
> >
> 
> Thanks for chiming in, Ted!
> Notes from the original author of FS_IOC_EXT2_[GS]ETFLAGS
> are valuable.

..... except when they are completely wrong. FS_IOC_[GS]ETXATTR was
promoted from XFs to the VFS to enable ext4 to use the existing
project ID and quota infrastructure (e.g. for testing with fstests).

> 
> > We can define some new interface for return what xflags are supported
> > by a particular file system.  This could either be the long-debated,
> > but never implemented statfsx() system call.  Or it could be extending
> > what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
> > fields in struct fsxattr.  This is arguably the simplest way of
> > dealing with the problem.
> >
> 
> That is also what I think.
> fsx_xflags_mask semantics for GET are pretty clear
> and follows the established pattern of  stx_attributes_mask
> Even if it is not mandatory for userspace, it can be useful.

You keep saying "it can be useful" without actually giving an
example of when it will be a significant improvement. Then you
demand proof that it isn't useful to userspace to justify a NACK.
That's not the way development works - you want the functionality,
you have to prove to that it is a significant improvement, not
demand that people prove that it isn't.

As it is, a lot of the "masks won't work" reasoning is in the
response I jsut wrote to Ted's email.  There appears to be a
significant lack of knowledge of the scope of the GET/SETXATTR
interfaces and how we've been using them for the past 20+ years
amongst the people arguing they should be fundamentally changed
right now.

> I asked Dave if he objects to fsx_xflags_mask and got silence,
> so IMO, if Pali wants to implement fsx_xflags_mask for the API
> I see no reason to resist it.

You didn't get silence - I only work 3 days a week and I'm
explicitly not responding to upstream devel list email outside of
work hours. So it may take several days before I find time to
respond to any given discussion thread.

As I've told you many, many times before, Amir: slow down and have
patience. There is no rush, nor is there a need to force the
discussion to a rapid conclusion.


> > I suppose the field could double as the bitmask field when
> > FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> > set of semantics.  If someone really wants to do that, I wouldn't
> > really complain, but then what we would actually call the field
> > "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)
> 
> If we follow the old rule of SET after GET should always work
> then fsx_xflags_mask will always be a superset of fsx_xflags,

Doesn't work for xfsdump/restore use case.

Firstly, there's no space in the dump media format for extended
xflags (i.e. requires new code and backwards incompatible media dump
formats to be created).

Secondly, if the destination kernel and/or filesystem does not
support the flags mask or features defined in the flags mask, what
do we do then?  The policy decision made a couple of decades ago was
that it is better for the kernel to ignore file attrs it doesn't
understand on restore and let the restore continue than it is to
abort the restore....

Better to restore what we support than not be able to restore
anything, yes?

> so I think it would be sane to return -EINVAL in the case
> of (fsx_xflags_mask && fsx_xflags & ~fsx_xflags_mask),
> which is anyway the correct behavior for filesystems when the
> user is trying to set flags that the filesystem does not support.
> 
> As far as I could see, all in-tree filesystems behave this way
> when the user is trying to set unsupported flags either via
> FS_IOC_SETFLAGS or via FS_IOC_SETXATTR
> except xfs, which ignores unsupported fsx_xflags from
> FS_IOC_SETXATTR.
> 
> Changing the behavior of xfs to return -EINVAL for unsupported
> fsx_xflags if fsx_xflags_mask is non zero is NOT going to break UAPI,

Except that it's completely undesireable behaviour for the existing
dump/restore usage of this interface.

If we just add the windows attributes to the fsx_flags field as
I have suggested, then xfsdump/xfs_restore would natively supports
backing up and restoring windows attributes on XFS files the moment
that the kernel XFS code supports windows attributes. 

It will not require any dump/restore code or dump media format
changes, and with the existing SET policy if the destination kernel
and/or filesystem doesn't support those attributes then they will be
silently dropped on restore...

Seriously, I said no because I undestand how these interfaces have
been used for the past 20 years. If you want to change the
fundamental behavioural characteristics of the API, then you have to
make sure that it works with the way xfsdump/restore use the API
across the dump media format. i.e. the consumer of the GET
information can be a SET operation on a different kernel and
filesystem.

I've already said no to a new syscall because it's just a set
of feature bits that can be added to FS_XFLAGS. And the added
advantage of that is that any backup program that already supports
backing up the fsxattr information will also support it without API
or dump media format changes. i.e. we get widespread support for the
windows attributes pretty much for free.

Yet here we are, with people instead wanting to construct complex
new APIs which will require entirely new infrastructure across the
board to support.

Your call - windows attribute support via a small amount of work for
an existing API addition, or a huge amount of work across the entire
filesystem ecosystem for a whole new API. The end functionality will
be identical, but one path is a whole lot less work for everyone
than the other....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 3/4] fs: Implement support for fsx_xflags_mask, fsx_xflags2 and fsx_xflags2_mask into vfs

[Pali Rohár]

On Friday 21 February 2025 13:24:16 Theodore Ts'o wrote:
> On Sun, Feb 16, 2025 at 05:40:28PM +0100, Pali Rohár wrote:
> > This change adds support for new struct fileattr fields fsx_xflags_mask,
> > fsx_xflags2 and fsx_xflags2_mask into FS_IOC_FSGETXATTR and
> > FS_IOC_FSSETXATTR ioctls.
> 
> I don't think I saw an answer to this question (but the discussions of
> the mail thread have really sprawled a bit and so it's hard to review
> all of the messages in the thread) --- so.... what's your reason for
> creating this new fsx_xflags2 field as opposed to just defining new
> flags bits for fsx_xflags field?  There are plenty of unused bits in
> the FS_XFLAG_* space.
> 
> Cheers,
> 
> 					- Ted
> 					

If all bits which I currently defined in fsx_xflags2 should be instead
defined in fsx_xflags then in fsx_xflags would be only 2 or 3 free bits.

And it is possible that I forgot to include some bits in this RFC
series, or that Windows starts (or already started) using some reserved
bits. And that would mean that we are out of the fsx_xflags space.

Also there are 4-5 Windows get-only bits which are not covered by this
RFC series. It is questionable if they should or should not be.

So IMHO, we do not have enough space in fsx_xflags to cover everything.

Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED for FS_IOC_FS[GS]ETXATTR API

[Pali Rohár]

On Tuesday 25 February 2025 16:41:37 Dave Chinner wrote:
> Your call - windows attribute support via a small amount of work for
> an existing API addition, or a huge amount of work across the entire
> filesystem ecosystem for a whole new API. The end functionality will
> be identical, but one path is a whole lot less work for everyone
> than the other....
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

So, would you rather see a change which defines all windows attributes
in fsx_xflags field, without any mask fields, and if there is no space
in fsx_xflags field anymore, define new fsx_xflags2 field?

I can prepare new RFC with this approach, and we can compare and discuss
what is better.

Just to note, that NFS4 protocol for supported subset of windows
attribute has for each attribute 3 state value: unsupported, unset, set.
So for knfsd it would be beneficial from filesystem driver to provide
information if particular attribute is supported or not (and not only if
attribute is set or unset). It does not have to be via this interface.
But my approach with mask field can easily provide this information.