linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing
@ 2019-06-11  4:45 Darrick J. Wong
  2019-06-11  4:45 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-06-11  4:45 UTC (permalink / raw)
  To: matthew.garrett, yuchao0, tytso, darrick.wong, shaggy,
	ard.biesheuvel, josef, clm, adilger.kernel, jk, jack, dsterba,
	jaegeuk, viro
  Cc: cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

Hi all,

The FS_IOC_SETFLAGS and FS_IOC_FSSETXATTR ioctls were promoted from ext4
and XFS, respectively, into the VFS.  However, we didn't promote any of
the parameter checking code from those filesystems, which lead to a mess
where each filesystem open-codes whatever parameter checks they want and
the behavior across filesystems is no longer consistent.

Therefore, create some generic checking functions in the VFS and remove
all the open-coded pieces in each filesystem.  This preserves the
current behavior where a filesystem can choose to ignore fields it
doesn't understand.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=file-ioctl-cleanups

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
  2019-06-11  4:45 [PATCH 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
@ 2019-06-11  4:45 ` Darrick J. Wong
  2019-06-11 13:41   ` [Jfs-discussion] " Dave Kleikamp
  2019-06-12  0:42   ` [PATCH v2 " Darrick J. Wong
  2019-06-11  4:45 ` [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR Darrick J. Wong
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-06-11  4:45 UTC (permalink / raw)
  To: matthew.garrett, yuchao0, tytso, darrick.wong, shaggy,
	ard.biesheuvel, josef, clm, adilger.kernel, jk, jack, dsterba,
	jaegeuk, viro
  Cc: cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
values so that we can standardize the implementations that follow ext4's
flag values.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/btrfs/ioctl.c    |   13 +++++--------
 fs/efivarfs/file.c  |   18 +++++++++++++-----
 fs/ext2/ioctl.c     |   16 ++++------------
 fs/ext4/ioctl.c     |   13 +++----------
 fs/f2fs/file.c      |    7 ++++---
 fs/gfs2/file.c      |   42 +++++++++++++++++++++++++++++-------------
 fs/hfsplus/ioctl.c  |   21 ++++++++++++---------
 fs/inode.c          |   17 +++++++++++++++++
 fs/jfs/ioctl.c      |    6 ++++++
 fs/nilfs2/ioctl.c   |    9 ++-------
 fs/ocfs2/ioctl.c    |   13 +++----------
 fs/reiserfs/ioctl.c |   10 ++++------
 fs/ubifs/ioctl.c    |   13 +++----------
 include/linux/fs.h  |    2 ++
 14 files changed, 107 insertions(+), 93 deletions(-)


diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6dafa857bbb9..f408aa93b0cf 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -187,7 +187,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	struct btrfs_inode *binode = BTRFS_I(inode);
 	struct btrfs_root *root = binode->root;
 	struct btrfs_trans_handle *trans;
-	unsigned int fsflags;
+	unsigned int fsflags, old_fsflags;
 	int ret;
 	const char *comp = NULL;
 	u32 binode_flags = binode->flags;
@@ -212,13 +212,10 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	inode_lock(inode);
 
 	fsflags = btrfs_mask_fsflags_for_type(inode, fsflags);
-	if ((fsflags ^ btrfs_inode_flags_to_fsflags(binode->flags)) &
-	    (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
-		if (!capable(CAP_LINUX_IMMUTABLE)) {
-			ret = -EPERM;
-			goto out_unlock;
-		}
-	}
+	old_fsflags = btrfs_inode_flags_to_fsflags(binode->flags);
+	ret = vfs_ioc_setflags_check(inode, old_fsflags, fsflags);
+	if (ret)
+		goto out_unlock;
 
 	if (fsflags & FS_SYNC_FL)
 		binode_flags |= BTRFS_INODE_SYNC;
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 8e568428c88b..f4f6c1bec132 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -110,16 +110,22 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	return size;
 }
 
-static int
-efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+static inline unsigned int efivarfs_getflags(struct inode *inode)
 {
-	struct inode *inode = file->f_mapping->host;
 	unsigned int i_flags;
 	unsigned int flags = 0;
 
 	i_flags = inode->i_flags;
 	if (i_flags & S_IMMUTABLE)
 		flags |= FS_IMMUTABLE_FL;
+	return flags;
+}
+
+static int
+efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int flags = efivarfs_getflags(inode);
 
 	if (copy_to_user(arg, &flags, sizeof(flags)))
 		return -EFAULT;
@@ -132,6 +138,7 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
 	struct inode *inode = file->f_mapping->host;
 	unsigned int flags;
 	unsigned int i_flags = 0;
+	unsigned int oldflags = efivarfs_getflags(inode);
 	int error;
 
 	if (!inode_owner_or_capable(inode))
@@ -143,8 +150,9 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
 	if (flags & ~FS_IMMUTABLE_FL)
 		return -EOPNOTSUPP;
 
-	if (!capable(CAP_LINUX_IMMUTABLE))
-		return -EPERM;
+	error = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (error)
+		return error;
 
 	if (flags & FS_IMMUTABLE_FL)
 		i_flags |= S_IMMUTABLE;
diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
index 0367c0039e68..88b3b9720023 100644
--- a/fs/ext2/ioctl.c
+++ b/fs/ext2/ioctl.c
@@ -60,18 +60,10 @@ long ext2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		}
 		oldflags = ei->i_flags;
 
-		/*
-		 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-		 * the relevant capability.
-		 *
-		 * This test looks nicer. Thanks to Pauline Middelink
-		 */
-		if ((flags ^ oldflags) & (EXT2_APPEND_FL | EXT2_IMMUTABLE_FL)) {
-			if (!capable(CAP_LINUX_IMMUTABLE)) {
-				inode_unlock(inode);
-				ret = -EPERM;
-				goto setflags_out;
-			}
+		ret = vfs_ioc_setflags_check(inode, oldflags, flags);
+		if (ret) {
+			inode_unlock(inode);
+			goto setflags_out;
 		}
 
 		flags = flags & EXT2_FL_USER_MODIFIABLE;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index e486e49b31ed..5126ee351a84 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -289,16 +289,9 @@ static int ext4_ioctl_setflags(struct inode *inode,
 	/* The JOURNAL_DATA flag is modifiable only by root */
 	jflag = flags & EXT4_JOURNAL_DATA_FL;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-	 * the relevant capability.
-	 *
-	 * This test looks nicer. Thanks to Pauline Middelink
-	 */
-	if ((flags ^ oldflags) & (EXT4_APPEND_FL | EXT4_IMMUTABLE_FL)) {
-		if (!capable(CAP_LINUX_IMMUTABLE))
-			goto flags_out;
-	}
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		goto flags_out;
 
 	/*
 	 * The JOURNAL_DATA flag can only be changed by
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 45b45f37d347..a969d5497e03 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1670,6 +1670,7 @@ static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags)
 {
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	unsigned int oldflags;
+	int err;
 
 	/* Is it quota file? Do not allow user to mess with it */
 	if (IS_NOQUOTA(inode))
@@ -1679,9 +1680,9 @@ static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags)
 
 	oldflags = fi->i_flags;
 
-	if ((flags ^ oldflags) & (F2FS_APPEND_FL | F2FS_IMMUTABLE_FL))
-		if (!capable(CAP_LINUX_IMMUTABLE))
-			return -EPERM;
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		return err;
 
 	flags = flags & F2FS_FL_USER_MODIFIABLE;
 	flags |= oldflags & ~F2FS_FL_USER_MODIFIABLE;
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index d174b1f8fd08..99f53cf699c6 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -136,27 +136,36 @@ static struct {
 	{FS_JOURNAL_DATA_FL, GFS2_DIF_JDATA | GFS2_DIF_INHERIT_JDATA},
 };
 
+static inline u32 gfs2_gfsflags_to_fsflags(struct inode *inode, u32 gfsflags)
+{
+	int i;
+	u32 fsflags = 0;
+
+	if (S_ISDIR(inode->i_mode))
+		gfsflags &= ~GFS2_DIF_JDATA;
+	else
+		gfsflags &= ~GFS2_DIF_INHERIT_JDATA;
+
+	for (i = 0; i < ARRAY_SIZE(fsflag_gfs2flag); i++)
+		if (gfsflags & fsflag_gfs2flag[i].gfsflag)
+			fsflags |= fsflag_gfs2flag[i].fsflag;
+	return fsflags;
+}
+
 static int gfs2_get_flags(struct file *filp, u32 __user *ptr)
 {
 	struct inode *inode = file_inode(filp);
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_holder gh;
-	int i, error;
-	u32 gfsflags, fsflags = 0;
+	int error;
+	u32 fsflags;
 
 	gfs2_holder_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
 	error = gfs2_glock_nq(&gh);
 	if (error)
 		goto out_uninit;
 
-	gfsflags = ip->i_diskflags;
-	if (S_ISDIR(inode->i_mode))
-		gfsflags &= ~GFS2_DIF_JDATA;
-	else
-		gfsflags &= ~GFS2_DIF_INHERIT_JDATA;
-	for (i = 0; i < ARRAY_SIZE(fsflag_gfs2flag); i++)
-		if (gfsflags & fsflag_gfs2flag[i].gfsflag)
-			fsflags |= fsflag_gfs2flag[i].fsflag;
+	fsflags = gfs2_gfsflags_to_fsflags(inode, ip->i_diskflags);
 
 	if (put_user(fsflags, ptr))
 		error = -EFAULT;
@@ -200,9 +209,11 @@ void gfs2_set_inode_flags(struct inode *inode)
  * @filp: file pointer
  * @reqflags: The flags to set
  * @mask: Indicates which flags are valid
+ * @fsflags: The FS_* inode flags passed in
  *
  */
-static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
+static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask,
+			     const u32 fsflags)
 {
 	struct inode *inode = file_inode(filp);
 	struct gfs2_inode *ip = GFS2_I(inode);
@@ -210,7 +221,7 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
 	struct buffer_head *bh;
 	struct gfs2_holder gh;
 	int error;
-	u32 new_flags, flags;
+	u32 new_flags, flags, oldflags;
 
 	error = mnt_want_write_file(filp);
 	if (error)
@@ -220,6 +231,11 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
 	if (error)
 		goto out_drop_write;
 
+	oldflags = gfs2_gfsflags_to_fsflags(inode, ip->i_diskflags);
+	error = vfs_ioc_setflags_check(inode, oldflags, fsflags);
+	if (error)
+		goto out;
+
 	error = -EACCES;
 	if (!inode_owner_or_capable(inode))
 		goto out;
@@ -308,7 +324,7 @@ static int gfs2_set_flags(struct file *filp, u32 __user *ptr)
 		mask &= ~(GFS2_DIF_TOPDIR | GFS2_DIF_INHERIT_JDATA);
 	}
 
-	return do_gfs2_set_flags(filp, gfsflags, mask);
+	return do_gfs2_set_flags(filp, gfsflags, mask, fsflags);
 }
 
 static int gfs2_getlabel(struct file *filp, char __user *label)
diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
index 5e6502ef7415..862a3c9481d7 100644
--- a/fs/hfsplus/ioctl.c
+++ b/fs/hfsplus/ioctl.c
@@ -57,9 +57,8 @@ static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
 	return 0;
 }
 
-static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
+static inline unsigned int hfsplus_getflags(struct inode *inode)
 {
-	struct inode *inode = file_inode(file);
 	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	unsigned int flags = 0;
 
@@ -69,6 +68,13 @@ static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
 		flags |= FS_APPEND_FL;
 	if (hip->userflags & HFSPLUS_FLG_NODUMP)
 		flags |= FS_NODUMP_FL;
+	return flags;
+}
+
+static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
+{
+	struct inode *inode = file_inode(file);
+	unsigned int flags = hfsplus_getflags(inode);
 
 	return put_user(flags, user_flags);
 }
@@ -78,6 +84,7 @@ static int hfsplus_ioctl_setflags(struct file *file, int __user *user_flags)
 	struct inode *inode = file_inode(file);
 	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	unsigned int flags, new_fl = 0;
+	unsigned int oldflags = hfsplus_getflags(inode);
 	int err = 0;
 
 	err = mnt_want_write_file(file);
@@ -96,13 +103,9 @@ static int hfsplus_ioctl_setflags(struct file *file, int __user *user_flags)
 
 	inode_lock(inode);
 
-	if ((flags & (FS_IMMUTABLE_FL|FS_APPEND_FL)) ||
-	    inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
-		if (!capable(CAP_LINUX_IMMUTABLE)) {
-			err = -EPERM;
-			goto out_unlock_inode;
-		}
-	}
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		goto out_unlock_inode;
 
 	/* don't silently ignore unsupported ext2 flags */
 	if (flags & ~(FS_IMMUTABLE_FL|FS_APPEND_FL|FS_NODUMP_FL)) {
diff --git a/fs/inode.c b/fs/inode.c
index df6542ec3b88..0ce60b720608 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2170,3 +2170,20 @@ struct timespec64 current_time(struct inode *inode)
 	return timespec64_trunc(now, inode->i_sb->s_time_gran);
 }
 EXPORT_SYMBOL(current_time);
+
+/* Generic function to check FS_IOC_SETFLAGS values. */
+int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags)
+{
+	/*
+	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
+	 * the relevant capability.
+	 *
+	 * This test looks nicer. Thanks to Pauline Middelink
+	 */
+	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
+	    !capable(CAP_LINUX_IMMUTABLE))
+		return -EPERM;
+
+	return 0;
+}
+EXPORT_SYMBOL(vfs_ioc_setflags_check);
diff --git a/fs/jfs/ioctl.c b/fs/jfs/ioctl.c
index ba34dae8bd9f..c8446d2cd0c7 100644
--- a/fs/jfs/ioctl.c
+++ b/fs/jfs/ioctl.c
@@ -98,6 +98,12 @@ long jfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		/* Lock against other parallel changes of flags */
 		inode_lock(inode);
 
+		oldflags = jfs_map_ext2(jfs_inode->mode2 & JFS_FL_USER_VISIBLE,
+					0);
+		err = vfs_ioc_setflags_check(inode, oldflags, flags);
+		if (err)
+			goto setflags_out;
+
 		oldflags = jfs_inode->mode2;
 
 		/*
diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index 9b96d79eea6c..0632336d2515 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -148,13 +148,8 @@ static int nilfs_ioctl_setflags(struct inode *inode, struct file *filp,
 
 	oldflags = NILFS_I(inode)->i_flags;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by the
-	 * relevant capability.
-	 */
-	ret = -EPERM;
-	if (((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) &&
-	    !capable(CAP_LINUX_IMMUTABLE))
+	ret = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (ret)
 		goto out;
 
 	ret = nilfs_transaction_begin(inode->i_sb, &ti, 0);
diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
index 994726ada857..467a2faf0305 100644
--- a/fs/ocfs2/ioctl.c
+++ b/fs/ocfs2/ioctl.c
@@ -106,16 +106,9 @@ static int ocfs2_set_inode_attr(struct inode *inode, unsigned flags,
 	flags = flags & mask;
 	flags |= oldflags & ~mask;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-	 * the relevant capability.
-	 */
-	status = -EPERM;
-	if ((oldflags & OCFS2_IMMUTABLE_FL) || ((flags ^ oldflags) &
-		(OCFS2_APPEND_FL | OCFS2_IMMUTABLE_FL))) {
-		if (!capable(CAP_LINUX_IMMUTABLE))
-			goto bail_unlock;
-	}
+	status = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (status)
+		goto bail_unlock;
 
 	handle = ocfs2_start_trans(osb, OCFS2_INODE_UPDATE_CREDITS);
 	if (IS_ERR(handle)) {
diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c
index acbbaf7a0bb2..92bcb1ecd994 100644
--- a/fs/reiserfs/ioctl.c
+++ b/fs/reiserfs/ioctl.c
@@ -74,13 +74,11 @@ long reiserfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 				err = -EPERM;
 				goto setflags_out;
 			}
-			if (((flags ^ REISERFS_I(inode)->
-			      i_attrs) & (REISERFS_IMMUTABLE_FL |
-					  REISERFS_APPEND_FL))
-			    && !capable(CAP_LINUX_IMMUTABLE)) {
-				err = -EPERM;
+			err = vfs_ioc_setflags_check(inode,
+						     REISERFS_I(inode)->i_attrs,
+						     flags);
+			if (err)
 				goto setflags_out;
-			}
 			if ((flags & REISERFS_NOTAIL_FL) &&
 			    S_ISREG(inode->i_mode)) {
 				int result;
diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
index 4f1a397fda69..bdea836fc38b 100644
--- a/fs/ubifs/ioctl.c
+++ b/fs/ubifs/ioctl.c
@@ -107,18 +107,11 @@ static int setflags(struct inode *inode, int flags)
 	if (err)
 		return err;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-	 * the relevant capability.
-	 */
 	mutex_lock(&ui->ui_mutex);
 	oldflags = ubifs2ioctl(ui->flags);
-	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
-		if (!capable(CAP_LINUX_IMMUTABLE)) {
-			err = -EPERM;
-			goto out_unlock;
-		}
-	}
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		goto out_unlock;
 
 	ui->flags = ioctl2ubifs(flags);
 	ubifs_set_inode_flags(inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f7fdfe93e25d..1825d055808c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3546,4 +3546,6 @@ static inline struct sock *io_uring_get_socket(struct file *file)
 }
 #endif
 
+int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
+
 #endif /* _LINUX_FS_H */


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR
  2019-06-11  4:45 [PATCH 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
  2019-06-11  4:45 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
@ 2019-06-11  4:45 ` Darrick J. Wong
  2019-06-20 13:38   ` Jan Kara
  2019-06-11  4:45 ` [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info Darrick J. Wong
  2019-06-11  4:46 ` [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints Darrick J. Wong
  3 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2019-06-11  4:45 UTC (permalink / raw)
  To: matthew.garrett, yuchao0, tytso, darrick.wong, shaggy,
	ard.biesheuvel, josef, clm, adilger.kernel, jk, jack, dsterba,
	jaegeuk, viro
  Cc: cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a generic checking function for the incoming FS_IOC_FSSETXATTR
fsxattr values so that we can standardize some of the implementation
behaviors.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/btrfs/ioctl.c   |   21 +++++++++-------
 fs/ext4/ioctl.c    |   27 ++++++++++++++------
 fs/f2fs/file.c     |   26 ++++++++++++++-----
 fs/inode.c         |   17 +++++++++++++
 fs/xfs/xfs_ioctl.c |   70 ++++++++++++++++++++++++++++++----------------------
 include/linux/fs.h |    3 ++
 6 files changed, 111 insertions(+), 53 deletions(-)


diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f408aa93b0cf..7ddda5b4b6a6 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -366,6 +366,13 @@ static int check_xflags(unsigned int flags)
 	return 0;
 }
 
+static void __btrfs_ioctl_fsgetxattr(struct btrfs_inode *binode,
+				     struct fsxattr *fa)
+{
+	memset(fa, 0, sizeof(*fa));
+	fa->fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
+}
+
 /*
  * Set the xflags from the internal inode flags. The remaining items of fsxattr
  * are zeroed.
@@ -375,8 +382,7 @@ static int btrfs_ioctl_fsgetxattr(struct file *file, void __user *arg)
 	struct btrfs_inode *binode = BTRFS_I(file_inode(file));
 	struct fsxattr fa;
 
-	memset(&fa, 0, sizeof(fa));
-	fa.fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
+	__btrfs_ioctl_fsgetxattr(binode, &fa);
 
 	if (copy_to_user(arg, &fa, sizeof(fa)))
 		return -EFAULT;
@@ -390,7 +396,7 @@ static int btrfs_ioctl_fssetxattr(struct file *file, void __user *arg)
 	struct btrfs_inode *binode = BTRFS_I(inode);
 	struct btrfs_root *root = binode->root;
 	struct btrfs_trans_handle *trans;
-	struct fsxattr fa;
+	struct fsxattr fa, old_fa;
 	unsigned old_flags;
 	unsigned old_i_flags;
 	int ret = 0;
@@ -421,13 +427,10 @@ static int btrfs_ioctl_fssetxattr(struct file *file, void __user *arg)
 	old_flags = binode->flags;
 	old_i_flags = inode->i_flags;
 
-	/* We need the capabilities to change append-only or immutable inode */
-	if (((old_flags & (BTRFS_INODE_APPEND | BTRFS_INODE_IMMUTABLE)) ||
-	     (fa.fsx_xflags & (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE))) &&
-	    !capable(CAP_LINUX_IMMUTABLE)) {
-		ret = -EPERM;
+	__btrfs_ioctl_fsgetxattr(binode, &old_fa);
+	ret = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
+	if (ret)
 		goto out_unlock;
-	}
 
 	if (fa.fsx_xflags & FS_XFLAG_SYNC)
 		binode->flags |= BTRFS_INODE_SYNC;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 5126ee351a84..c2f48c90ca45 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -721,6 +721,19 @@ static int ext4_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
 	return 0;
 }
 
+static void ext4_fsgetxattr(struct inode *inode, struct fsxattr *fa)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+
+	memset(fa, 0, sizeof(struct fsxattr));
+	fa->fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & EXT4_FL_USER_VISIBLE);
+
+	if (ext4_has_feature_project(inode->i_sb)) {
+		fa->fsx_projid = (__u32)from_kprojid(&init_user_ns,
+				ei->i_projid);
+	}
+}
+
 long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -1089,13 +1102,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	{
 		struct fsxattr fa;
 
-		memset(&fa, 0, sizeof(struct fsxattr));
-		fa.fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & EXT4_FL_USER_VISIBLE);
-
-		if (ext4_has_feature_project(inode->i_sb)) {
-			fa.fsx_projid = (__u32)from_kprojid(&init_user_ns,
-				EXT4_I(inode)->i_projid);
-		}
+		ext4_fsgetxattr(inode, &fa);
 
 		if (copy_to_user((struct fsxattr __user *)arg,
 				 &fa, sizeof(fa)))
@@ -1104,7 +1111,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	}
 	case EXT4_IOC_FSSETXATTR:
 	{
-		struct fsxattr fa;
+		struct fsxattr fa, old_fa;
 		int err;
 
 		if (copy_from_user(&fa, (struct fsxattr __user *)arg,
@@ -1127,7 +1134,11 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			return err;
 
 		inode_lock(inode);
+		ext4_fsgetxattr(inode, &old_fa);
 		err = ext4_ioctl_check_project(inode, &fa);
+		if (err)
+			goto out;
+		err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
 		if (err)
 			goto out;
 		flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index a969d5497e03..f707de6bd4a8 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2773,19 +2773,26 @@ static inline unsigned long f2fs_xflags_to_iflags(__u32 xflags)
 	return iflags;
 }
 
-static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
+static void __f2fs_ioc_fsgetxattr(struct inode *inode,
+				  struct fsxattr *fa)
 {
-	struct inode *inode = file_inode(filp);
 	struct f2fs_inode_info *fi = F2FS_I(inode);
-	struct fsxattr fa;
 
-	memset(&fa, 0, sizeof(struct fsxattr));
-	fa.fsx_xflags = f2fs_iflags_to_xflags(fi->i_flags &
+	memset(fa, 0, sizeof(struct fsxattr));
+	fa->fsx_xflags = f2fs_iflags_to_xflags(fi->i_flags &
 				F2FS_FL_USER_VISIBLE);
 
 	if (f2fs_sb_has_project_quota(F2FS_I_SB(inode)))
-		fa.fsx_projid = (__u32)from_kprojid(&init_user_ns,
+		fa->fsx_projid = (__u32)from_kprojid(&init_user_ns,
 							fi->i_projid);
+}
+
+static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = file_inode(filp);
+	struct fsxattr fa;
+
+	__f2fs_ioc_fsgetxattr(inode, &fa);
 
 	if (copy_to_user((struct fsxattr __user *)arg, &fa, sizeof(fa)))
 		return -EFAULT;
@@ -2820,7 +2827,7 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
 	struct f2fs_inode_info *fi = F2FS_I(inode);
-	struct fsxattr fa;
+	struct fsxattr fa, old_fa;
 	unsigned int flags;
 	int err;
 
@@ -2844,6 +2851,11 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
 
 	inode_lock(inode);
 	err = f2fs_ioctl_check_project(inode, &fa);
+	if (err)
+		goto out;
+
+	__f2fs_ioc_fsgetxattr(inode, &old_fa);
+	err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
 	if (err)
 		goto out;
 	flags = (fi->i_flags & ~F2FS_FL_XFLAG_VISIBLE) |
diff --git a/fs/inode.c b/fs/inode.c
index 0ce60b720608..026955258a47 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2187,3 +2187,20 @@ int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags)
 	return 0;
 }
 EXPORT_SYMBOL(vfs_ioc_setflags_check);
+
+/* Generic function to check FS_IOC_FSSETXATTR values. */
+int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
+			     struct fsxattr *fa)
+{
+	/*
+	 * Can't modify an immutable/append-only file unless we have
+	 * appropriate permission.
+	 */
+	if ((old_fa->fsx_xflags ^ fa->fsx_xflags) &
+			(FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND) &&
+	    !capable(CAP_LINUX_IMMUTABLE))
+		return -EPERM;
+
+	return 0;
+}
+EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d7dfc13f30f5..08c24f2f55c3 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -879,37 +879,45 @@ xfs_di2lxflags(
 	return flags;
 }
 
-STATIC int
-xfs_ioc_fsgetxattr(
-	xfs_inode_t		*ip,
-	int			attr,
-	void			__user *arg)
+static void
+__xfs_ioc_fsgetxattr(
+	struct xfs_inode	*ip,
+	bool			attr,
+	struct fsxattr		*fa)
 {
-	struct fsxattr		fa;
-
-	memset(&fa, 0, sizeof(struct fsxattr));
-
-	xfs_ilock(ip, XFS_ILOCK_SHARED);
-	fa.fsx_xflags = xfs_ip2xflags(ip);
-	fa.fsx_extsize = ip->i_d.di_extsize << ip->i_mount->m_sb.sb_blocklog;
-	fa.fsx_cowextsize = ip->i_d.di_cowextsize <<
+	memset(fa, 0, sizeof(struct fsxattr));
+	fa->fsx_xflags = xfs_ip2xflags(ip);
+	fa->fsx_extsize = ip->i_d.di_extsize << ip->i_mount->m_sb.sb_blocklog;
+	fa->fsx_cowextsize = ip->i_d.di_cowextsize <<
 			ip->i_mount->m_sb.sb_blocklog;
-	fa.fsx_projid = xfs_get_projid(ip);
+	fa->fsx_projid = xfs_get_projid(ip);
 
 	if (attr) {
 		if (ip->i_afp) {
 			if (ip->i_afp->if_flags & XFS_IFEXTENTS)
-				fa.fsx_nextents = xfs_iext_count(ip->i_afp);
+				fa->fsx_nextents = xfs_iext_count(ip->i_afp);
 			else
-				fa.fsx_nextents = ip->i_d.di_anextents;
+				fa->fsx_nextents = ip->i_d.di_anextents;
 		} else
-			fa.fsx_nextents = 0;
+			fa->fsx_nextents = 0;
 	} else {
 		if (ip->i_df.if_flags & XFS_IFEXTENTS)
-			fa.fsx_nextents = xfs_iext_count(&ip->i_df);
+			fa->fsx_nextents = xfs_iext_count(&ip->i_df);
 		else
-			fa.fsx_nextents = ip->i_d.di_nextents;
+			fa->fsx_nextents = ip->i_d.di_nextents;
 	}
+}
+
+STATIC int
+xfs_ioc_fsgetxattr(
+	xfs_inode_t		*ip,
+	int			attr,
+	void			__user *arg)
+{
+	struct fsxattr		fa;
+
+	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	__xfs_ioc_fsgetxattr(ip, attr, &fa);
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
 
 	if (copy_to_user(arg, &fa, sizeof(fa)))
@@ -1035,15 +1043,6 @@ xfs_ioctl_setattr_xflags(
 	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
 		return -EINVAL;
 
-	/*
-	 * Can't modify an immutable/append-only file unless
-	 * we have appropriate permission.
-	 */
-	if (((ip->i_d.di_flags & (XFS_DIFLAG_IMMUTABLE | XFS_DIFLAG_APPEND)) ||
-	     (fa->fsx_xflags & (FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND))) &&
-	    !capable(CAP_LINUX_IMMUTABLE))
-		return -EPERM;
-
 	/* diflags2 only valid for v3 inodes. */
 	di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
 	if (di_flags2 && ip->i_d.di_version < 3)
@@ -1323,6 +1322,7 @@ xfs_ioctl_setattr(
 	xfs_inode_t		*ip,
 	struct fsxattr		*fa)
 {
+	struct fsxattr		old_fa;
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	struct xfs_dquot	*udqp = NULL;
@@ -1370,7 +1370,6 @@ xfs_ioctl_setattr(
 		goto error_free_dquots;
 	}
 
-
 	if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
 	    xfs_get_projid(ip) != fa->fsx_projid) {
 		code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL, pdqp,
@@ -1379,6 +1378,11 @@ xfs_ioctl_setattr(
 			goto error_trans_cancel;
 	}
 
+	__xfs_ioc_fsgetxattr(ip, false, &old_fa);
+	code = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
+	if (code)
+		goto error_trans_cancel;
+
 	code = xfs_ioctl_setattr_check_extsize(ip, fa);
 	if (code)
 		goto error_trans_cancel;
@@ -1489,6 +1493,7 @@ xfs_ioc_setxflags(
 {
 	struct xfs_trans	*tp;
 	struct fsxattr		fa;
+	struct fsxattr		old_fa;
 	unsigned int		flags;
 	int			join_flags = 0;
 	int			error;
@@ -1524,6 +1529,13 @@ xfs_ioc_setxflags(
 		goto out_drop_write;
 	}
 
+	__xfs_ioc_fsgetxattr(ip, false, &old_fa);
+	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, &fa);
+	if (error) {
+		xfs_trans_cancel(tp);
+		goto out_drop_write;
+	}
+
 	error = xfs_ioctl_setattr_xflags(tp, ip, &fa);
 	if (error) {
 		xfs_trans_cancel(tp);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1825d055808c..8dad3c80b611 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3548,4 +3548,7 @@ static inline struct sock *io_uring_get_socket(struct file *file)
 
 int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
 
+int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
+			     struct fsxattr *fa);
+
 #endif /* _LINUX_FS_H */


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info
  2019-06-11  4:45 [PATCH 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
  2019-06-11  4:45 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
  2019-06-11  4:45 ` [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR Darrick J. Wong
@ 2019-06-11  4:45 ` Darrick J. Wong
  2019-06-20 13:41   ` Jan Kara
  2019-06-11  4:46 ` [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints Darrick J. Wong
  3 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2019-06-11  4:45 UTC (permalink / raw)
  To: matthew.garrett, yuchao0, tytso, darrick.wong, shaggy,
	ard.biesheuvel, josef, clm, adilger.kernel, jk, jack, dsterba,
	jaegeuk, viro
  Cc: cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Standardize the project id checks for FSSETXATTR.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ext4/ioctl.c    |   27 ---------------------------
 fs/f2fs/file.c     |   27 ---------------------------
 fs/inode.c         |   13 +++++++++++++
 fs/xfs/xfs_ioctl.c |   15 ---------------
 4 files changed, 13 insertions(+), 69 deletions(-)


diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index c2f48c90ca45..6aa1df1918f7 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -697,30 +697,6 @@ static long ext4_ioctl_group_add(struct file *file,
 	return err;
 }
 
-static int ext4_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
-{
-	/*
-	 * Project Quota ID state is only allowed to change from within the init
-	 * namespace. Enforce that restriction only if we are trying to change
-	 * the quota ID state. Everything else is allowed in user namespaces.
-	 */
-	if (current_user_ns() == &init_user_ns)
-		return 0;
-
-	if (__kprojid_val(EXT4_I(inode)->i_projid) != fa->fsx_projid)
-		return -EINVAL;
-
-	if (ext4_test_inode_flag(inode, EXT4_INODE_PROJINHERIT)) {
-		if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
-			return -EINVAL;
-	} else {
-		if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
-			return -EINVAL;
-	}
-
-	return 0;
-}
-
 static void ext4_fsgetxattr(struct inode *inode, struct fsxattr *fa)
 {
 	struct ext4_inode_info *ei = EXT4_I(inode);
@@ -1135,9 +1111,6 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		inode_lock(inode);
 		ext4_fsgetxattr(inode, &old_fa);
-		err = ext4_ioctl_check_project(inode, &fa);
-		if (err)
-			goto out;
 		err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
 		if (err)
 			goto out;
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f707de6bd4a8..183ed1ac60e1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2799,30 +2799,6 @@ static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
 	return 0;
 }
 
-static int f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
-{
-	/*
-	 * Project Quota ID state is only allowed to change from within the init
-	 * namespace. Enforce that restriction only if we are trying to change
-	 * the quota ID state. Everything else is allowed in user namespaces.
-	 */
-	if (current_user_ns() == &init_user_ns)
-		return 0;
-
-	if (__kprojid_val(F2FS_I(inode)->i_projid) != fa->fsx_projid)
-		return -EINVAL;
-
-	if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) {
-		if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
-			return -EINVAL;
-	} else {
-		if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
-			return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -2850,9 +2826,6 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
 		return err;
 
 	inode_lock(inode);
-	err = f2fs_ioctl_check_project(inode, &fa);
-	if (err)
-		goto out;
 
 	__f2fs_ioc_fsgetxattr(inode, &old_fa);
 	err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
diff --git a/fs/inode.c b/fs/inode.c
index 026955258a47..40ecd3a6a188 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2201,6 +2201,19 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
 	    !capable(CAP_LINUX_IMMUTABLE))
 		return -EPERM;
 
+	/*
+	 * Project Quota ID state is only allowed to change from within the init
+	 * namespace. Enforce that restriction only if we are trying to change
+	 * the quota ID state. Everything else is allowed in user namespaces.
+	 */
+	if (current_user_ns() != &init_user_ns) {
+		if (old_fa->fsx_projid != fa->fsx_projid)
+			return -EINVAL;
+		if ((old_fa->fsx_xflags ^ fa->fsx_xflags) &
+				FS_XFLAG_PROJINHERIT)
+			return -EINVAL;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 08c24f2f55c3..82961de98900 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1299,21 +1299,6 @@ xfs_ioctl_setattr_check_projid(
 	if (fa->fsx_projid > (uint16_t)-1 &&
 	    !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
 		return -EINVAL;
-
-	/*
-	 * Project Quota ID state is only allowed to change from within the init
-	 * namespace. Enforce that restriction only if we are trying to change
-	 * the quota ID state. Everything else is allowed in user namespaces.
-	 */
-	if (current_user_ns() == &init_user_ns)
-		return 0;
-
-	if (xfs_get_projid(ip) != fa->fsx_projid)
-		return -EINVAL;
-	if ((fa->fsx_xflags & FS_XFLAG_PROJINHERIT) !=
-	    (ip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT))
-		return -EINVAL;
-
 	return 0;
 }
 


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints
  2019-06-11  4:45 [PATCH 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
                   ` (2 preceding siblings ...)
  2019-06-11  4:45 ` [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info Darrick J. Wong
@ 2019-06-11  4:46 ` Darrick J. Wong
  2019-06-20 13:46   ` Jan Kara
  3 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2019-06-11  4:46 UTC (permalink / raw)
  To: matthew.garrett, yuchao0, tytso, darrick.wong, shaggy,
	ard.biesheuvel, josef, clm, adilger.kernel, jk, jack, dsterba,
	jaegeuk, viro
  Cc: cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the extent size hint checks that aren't xfs-specific to the vfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/inode.c         |   18 +++++++++++++
 fs/xfs/xfs_ioctl.c |   70 ++++++++++++++++++++++------------------------------
 2 files changed, 47 insertions(+), 41 deletions(-)


diff --git a/fs/inode.c b/fs/inode.c
index 40ecd3a6a188..a3757051fd55 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2214,6 +2214,24 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
 			return -EINVAL;
 	}
 
+	/* Check extent size hints. */
+	if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
+		return -EINVAL;
+
+	if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
+			!S_ISDIR(inode->i_mode))
+		return -EINVAL;
+
+	if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
+	    !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+		return -EINVAL;
+
+	/* Extent size hints of zero turn off the flags. */
+	if (fa->fsx_extsize == 0)
+		fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
+	if (fa->fsx_cowextsize == 0)
+		fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
+
 	return 0;
 }
 EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 82961de98900..b494e7e881e3 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1201,39 +1201,31 @@ xfs_ioctl_setattr_check_extsize(
 	struct fsxattr		*fa)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-
-	if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(VFS_I(ip)->i_mode))
-		return -EINVAL;
-
-	if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
-	    !S_ISDIR(VFS_I(ip)->i_mode))
-		return -EINVAL;
+	xfs_extlen_t		size;
+	xfs_fsblock_t		extsize_fsb;
 
 	if (S_ISREG(VFS_I(ip)->i_mode) && ip->i_d.di_nextents &&
 	    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) != fa->fsx_extsize))
 		return -EINVAL;
 
-	if (fa->fsx_extsize != 0) {
-		xfs_extlen_t    size;
-		xfs_fsblock_t   extsize_fsb;
-
-		extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
-		if (extsize_fsb > MAXEXTLEN)
-			return -EINVAL;
+	if (fa->fsx_extsize == 0)
+		return 0;
 
-		if (XFS_IS_REALTIME_INODE(ip) ||
-		    (fa->fsx_xflags & FS_XFLAG_REALTIME)) {
-			size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
-		} else {
-			size = mp->m_sb.sb_blocksize;
-			if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
-				return -EINVAL;
-		}
+	extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
+	if (extsize_fsb > MAXEXTLEN)
+		return -EINVAL;
 
-		if (fa->fsx_extsize % size)
+	if (XFS_IS_REALTIME_INODE(ip) ||
+	    (fa->fsx_xflags & FS_XFLAG_REALTIME)) {
+		size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
+	} else {
+		size = mp->m_sb.sb_blocksize;
+		if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
 			return -EINVAL;
-	} else
-		fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
+	}
+
+	if (fa->fsx_extsize % size)
+		return -EINVAL;
 
 	return 0;
 }
@@ -1259,6 +1251,8 @@ xfs_ioctl_setattr_check_cowextsize(
 	struct fsxattr		*fa)
 {
 	struct xfs_mount	*mp = ip->i_mount;
+	xfs_extlen_t		size;
+	xfs_fsblock_t		cowextsize_fsb;
 
 	if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
 		return 0;
@@ -1267,25 +1261,19 @@ xfs_ioctl_setattr_check_cowextsize(
 	    ip->i_d.di_version != 3)
 		return -EINVAL;
 
-	if (!S_ISREG(VFS_I(ip)->i_mode) && !S_ISDIR(VFS_I(ip)->i_mode))
-		return -EINVAL;
-
-	if (fa->fsx_cowextsize != 0) {
-		xfs_extlen_t    size;
-		xfs_fsblock_t   cowextsize_fsb;
+	if (fa->fsx_cowextsize == 0)
+		return 0;
 
-		cowextsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_cowextsize);
-		if (cowextsize_fsb > MAXEXTLEN)
-			return -EINVAL;
+	cowextsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_cowextsize);
+	if (cowextsize_fsb > MAXEXTLEN)
+		return -EINVAL;
 
-		size = mp->m_sb.sb_blocksize;
-		if (cowextsize_fsb > mp->m_sb.sb_agblocks / 2)
-			return -EINVAL;
+	size = mp->m_sb.sb_blocksize;
+	if (cowextsize_fsb > mp->m_sb.sb_agblocks / 2)
+		return -EINVAL;
 
-		if (fa->fsx_cowextsize % size)
-			return -EINVAL;
-	} else
-		fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
+	if (fa->fsx_cowextsize % size)
+		return -EINVAL;
 
 	return 0;
 }


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Jfs-discussion] [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
  2019-06-11  4:45 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
@ 2019-06-11 13:41   ` Dave Kleikamp
  2019-06-12  0:35     ` Darrick J. Wong
  2019-06-12  0:42   ` [PATCH v2 " Darrick J. Wong
  1 sibling, 1 reply; 12+ messages in thread
From: Dave Kleikamp @ 2019-06-11 13:41 UTC (permalink / raw)
  To: Darrick J. Wong, matthew.garrett, yuchao0, tytso, shaggy,
	ard.biesheuvel, josef, clm, adilger.kernel, jk, jack, dsterba,
	jaegeuk, viro
  Cc: linux-xfs, jfs-discussion, linux-efi, linux-kernel,
	reiserfs-devel, linux-f2fs-devel, cluster-devel, linux-nilfs,
	linux-mtd, linux-btrfs, linux-fsdevel, linux-ext4, ocfs2-devel

On 6/10/19 11:45 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
> values so that we can standardize the implementations that follow ext4's
> flag values.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

 -- clip --

> diff --git a/fs/jfs/ioctl.c b/fs/jfs/ioctl.c
> index ba34dae8bd9f..c8446d2cd0c7 100644
> --- a/fs/jfs/ioctl.c
> +++ b/fs/jfs/ioctl.c
> @@ -98,6 +98,12 @@ long jfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		/* Lock against other parallel changes of flags */
>  		inode_lock(inode);
>  
> +		oldflags = jfs_map_ext2(jfs_inode->mode2 & JFS_FL_USER_VISIBLE,
> +					0);
> +		err = vfs_ioc_setflags_check(inode, oldflags, flags);
> +		if (err)
> +			goto setflags_out;

inode_unlock(inode) is not called on the error path.

> +
>  		oldflags = jfs_inode->mode2;
>  
>  		/*

This patch leaves jfs's open-coded version of the same check.

Thanks,
Shaggy

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Jfs-discussion] [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
  2019-06-11 13:41   ` [Jfs-discussion] " Dave Kleikamp
@ 2019-06-12  0:35     ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2019-06-12  0:35 UTC (permalink / raw)
  To: Dave Kleikamp
  Cc: matthew.garrett, yuchao0, tytso, shaggy, ard.biesheuvel, josef,
	clm, adilger.kernel, jk, jack, dsterba, jaegeuk, viro, linux-xfs,
	jfs-discussion, linux-efi, linux-kernel, reiserfs-devel,
	linux-f2fs-devel, cluster-devel, linux-nilfs, linux-mtd,
	linux-btrfs, linux-fsdevel, linux-ext4, ocfs2-devel

On Tue, Jun 11, 2019 at 08:41:06AM -0500, Dave Kleikamp wrote:
> On 6/10/19 11:45 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
> > values so that we can standardize the implementations that follow ext4's
> > flag values.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
>  -- clip --
> 
> > diff --git a/fs/jfs/ioctl.c b/fs/jfs/ioctl.c
> > index ba34dae8bd9f..c8446d2cd0c7 100644
> > --- a/fs/jfs/ioctl.c
> > +++ b/fs/jfs/ioctl.c
> > @@ -98,6 +98,12 @@ long jfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >  		/* Lock against other parallel changes of flags */
> >  		inode_lock(inode);
> >  
> > +		oldflags = jfs_map_ext2(jfs_inode->mode2 & JFS_FL_USER_VISIBLE,
> > +					0);
> > +		err = vfs_ioc_setflags_check(inode, oldflags, flags);
> > +		if (err)
> > +			goto setflags_out;
> 
> inode_unlock(inode) is not called on the error path.
> 
> > +
> >  		oldflags = jfs_inode->mode2;
> >  
> >  		/*
> 
> This patch leaves jfs's open-coded version of the same check.

Heh, thanks for pointing that out.  I'll fix both of those things.

--D

> Thanks,
> Shaggy

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
  2019-06-11  4:45 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
  2019-06-11 13:41   ` [Jfs-discussion] " Dave Kleikamp
@ 2019-06-12  0:42   ` Darrick J. Wong
  2019-06-20 13:34     ` Jan Kara
  1 sibling, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2019-06-12  0:42 UTC (permalink / raw)
  To: matthew.garrett, yuchao0, tytso, shaggy, ard.biesheuvel, josef,
	clm, adilger.kernel, jk, jack, dsterba, jaegeuk, viro
  Cc: cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
values so that we can standardize the implementations that follow ext4's
flag values.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: fix jfs locking and remove its opencoded flags check
---
 fs/btrfs/ioctl.c    |   13 +++++--------
 fs/efivarfs/file.c  |   18 +++++++++++++-----
 fs/ext2/ioctl.c     |   16 ++++------------
 fs/ext4/ioctl.c     |   13 +++----------
 fs/f2fs/file.c      |    7 ++++---
 fs/gfs2/file.c      |   42 +++++++++++++++++++++++++++++-------------
 fs/hfsplus/ioctl.c  |   21 ++++++++++++---------
 fs/inode.c          |   17 +++++++++++++++++
 fs/jfs/ioctl.c      |   22 +++++++---------------
 fs/nilfs2/ioctl.c   |    9 ++-------
 fs/ocfs2/ioctl.c    |   13 +++----------
 fs/reiserfs/ioctl.c |   10 ++++------
 fs/ubifs/ioctl.c    |   13 +++----------
 include/linux/fs.h  |    2 ++
 14 files changed, 108 insertions(+), 108 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6dafa857bbb9..f408aa93b0cf 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -187,7 +187,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	struct btrfs_inode *binode = BTRFS_I(inode);
 	struct btrfs_root *root = binode->root;
 	struct btrfs_trans_handle *trans;
-	unsigned int fsflags;
+	unsigned int fsflags, old_fsflags;
 	int ret;
 	const char *comp = NULL;
 	u32 binode_flags = binode->flags;
@@ -212,13 +212,10 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	inode_lock(inode);
 
 	fsflags = btrfs_mask_fsflags_for_type(inode, fsflags);
-	if ((fsflags ^ btrfs_inode_flags_to_fsflags(binode->flags)) &
-	    (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
-		if (!capable(CAP_LINUX_IMMUTABLE)) {
-			ret = -EPERM;
-			goto out_unlock;
-		}
-	}
+	old_fsflags = btrfs_inode_flags_to_fsflags(binode->flags);
+	ret = vfs_ioc_setflags_check(inode, old_fsflags, fsflags);
+	if (ret)
+		goto out_unlock;
 
 	if (fsflags & FS_SYNC_FL)
 		binode_flags |= BTRFS_INODE_SYNC;
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 8e568428c88b..f4f6c1bec132 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -110,16 +110,22 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	return size;
 }
 
-static int
-efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+static inline unsigned int efivarfs_getflags(struct inode *inode)
 {
-	struct inode *inode = file->f_mapping->host;
 	unsigned int i_flags;
 	unsigned int flags = 0;
 
 	i_flags = inode->i_flags;
 	if (i_flags & S_IMMUTABLE)
 		flags |= FS_IMMUTABLE_FL;
+	return flags;
+}
+
+static int
+efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+{
+	struct inode *inode = file->f_mapping->host;
+	unsigned int flags = efivarfs_getflags(inode);
 
 	if (copy_to_user(arg, &flags, sizeof(flags)))
 		return -EFAULT;
@@ -132,6 +138,7 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
 	struct inode *inode = file->f_mapping->host;
 	unsigned int flags;
 	unsigned int i_flags = 0;
+	unsigned int oldflags = efivarfs_getflags(inode);
 	int error;
 
 	if (!inode_owner_or_capable(inode))
@@ -143,8 +150,9 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
 	if (flags & ~FS_IMMUTABLE_FL)
 		return -EOPNOTSUPP;
 
-	if (!capable(CAP_LINUX_IMMUTABLE))
-		return -EPERM;
+	error = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (error)
+		return error;
 
 	if (flags & FS_IMMUTABLE_FL)
 		i_flags |= S_IMMUTABLE;
diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
index 0367c0039e68..88b3b9720023 100644
--- a/fs/ext2/ioctl.c
+++ b/fs/ext2/ioctl.c
@@ -60,18 +60,10 @@ long ext2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		}
 		oldflags = ei->i_flags;
 
-		/*
-		 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-		 * the relevant capability.
-		 *
-		 * This test looks nicer. Thanks to Pauline Middelink
-		 */
-		if ((flags ^ oldflags) & (EXT2_APPEND_FL | EXT2_IMMUTABLE_FL)) {
-			if (!capable(CAP_LINUX_IMMUTABLE)) {
-				inode_unlock(inode);
-				ret = -EPERM;
-				goto setflags_out;
-			}
+		ret = vfs_ioc_setflags_check(inode, oldflags, flags);
+		if (ret) {
+			inode_unlock(inode);
+			goto setflags_out;
 		}
 
 		flags = flags & EXT2_FL_USER_MODIFIABLE;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index e486e49b31ed..5126ee351a84 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -289,16 +289,9 @@ static int ext4_ioctl_setflags(struct inode *inode,
 	/* The JOURNAL_DATA flag is modifiable only by root */
 	jflag = flags & EXT4_JOURNAL_DATA_FL;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-	 * the relevant capability.
-	 *
-	 * This test looks nicer. Thanks to Pauline Middelink
-	 */
-	if ((flags ^ oldflags) & (EXT4_APPEND_FL | EXT4_IMMUTABLE_FL)) {
-		if (!capable(CAP_LINUX_IMMUTABLE))
-			goto flags_out;
-	}
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		goto flags_out;
 
 	/*
 	 * The JOURNAL_DATA flag can only be changed by
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 45b45f37d347..a969d5497e03 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1670,6 +1670,7 @@ static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags)
 {
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	unsigned int oldflags;
+	int err;
 
 	/* Is it quota file? Do not allow user to mess with it */
 	if (IS_NOQUOTA(inode))
@@ -1679,9 +1680,9 @@ static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags)
 
 	oldflags = fi->i_flags;
 
-	if ((flags ^ oldflags) & (F2FS_APPEND_FL | F2FS_IMMUTABLE_FL))
-		if (!capable(CAP_LINUX_IMMUTABLE))
-			return -EPERM;
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		return err;
 
 	flags = flags & F2FS_FL_USER_MODIFIABLE;
 	flags |= oldflags & ~F2FS_FL_USER_MODIFIABLE;
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index d174b1f8fd08..99f53cf699c6 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -136,27 +136,36 @@ static struct {
 	{FS_JOURNAL_DATA_FL, GFS2_DIF_JDATA | GFS2_DIF_INHERIT_JDATA},
 };
 
+static inline u32 gfs2_gfsflags_to_fsflags(struct inode *inode, u32 gfsflags)
+{
+	int i;
+	u32 fsflags = 0;
+
+	if (S_ISDIR(inode->i_mode))
+		gfsflags &= ~GFS2_DIF_JDATA;
+	else
+		gfsflags &= ~GFS2_DIF_INHERIT_JDATA;
+
+	for (i = 0; i < ARRAY_SIZE(fsflag_gfs2flag); i++)
+		if (gfsflags & fsflag_gfs2flag[i].gfsflag)
+			fsflags |= fsflag_gfs2flag[i].fsflag;
+	return fsflags;
+}
+
 static int gfs2_get_flags(struct file *filp, u32 __user *ptr)
 {
 	struct inode *inode = file_inode(filp);
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_holder gh;
-	int i, error;
-	u32 gfsflags, fsflags = 0;
+	int error;
+	u32 fsflags;
 
 	gfs2_holder_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
 	error = gfs2_glock_nq(&gh);
 	if (error)
 		goto out_uninit;
 
-	gfsflags = ip->i_diskflags;
-	if (S_ISDIR(inode->i_mode))
-		gfsflags &= ~GFS2_DIF_JDATA;
-	else
-		gfsflags &= ~GFS2_DIF_INHERIT_JDATA;
-	for (i = 0; i < ARRAY_SIZE(fsflag_gfs2flag); i++)
-		if (gfsflags & fsflag_gfs2flag[i].gfsflag)
-			fsflags |= fsflag_gfs2flag[i].fsflag;
+	fsflags = gfs2_gfsflags_to_fsflags(inode, ip->i_diskflags);
 
 	if (put_user(fsflags, ptr))
 		error = -EFAULT;
@@ -200,9 +209,11 @@ void gfs2_set_inode_flags(struct inode *inode)
  * @filp: file pointer
  * @reqflags: The flags to set
  * @mask: Indicates which flags are valid
+ * @fsflags: The FS_* inode flags passed in
  *
  */
-static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
+static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask,
+			     const u32 fsflags)
 {
 	struct inode *inode = file_inode(filp);
 	struct gfs2_inode *ip = GFS2_I(inode);
@@ -210,7 +221,7 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
 	struct buffer_head *bh;
 	struct gfs2_holder gh;
 	int error;
-	u32 new_flags, flags;
+	u32 new_flags, flags, oldflags;
 
 	error = mnt_want_write_file(filp);
 	if (error)
@@ -220,6 +231,11 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
 	if (error)
 		goto out_drop_write;
 
+	oldflags = gfs2_gfsflags_to_fsflags(inode, ip->i_diskflags);
+	error = vfs_ioc_setflags_check(inode, oldflags, fsflags);
+	if (error)
+		goto out;
+
 	error = -EACCES;
 	if (!inode_owner_or_capable(inode))
 		goto out;
@@ -308,7 +324,7 @@ static int gfs2_set_flags(struct file *filp, u32 __user *ptr)
 		mask &= ~(GFS2_DIF_TOPDIR | GFS2_DIF_INHERIT_JDATA);
 	}
 
-	return do_gfs2_set_flags(filp, gfsflags, mask);
+	return do_gfs2_set_flags(filp, gfsflags, mask, fsflags);
 }
 
 static int gfs2_getlabel(struct file *filp, char __user *label)
diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
index 5e6502ef7415..862a3c9481d7 100644
--- a/fs/hfsplus/ioctl.c
+++ b/fs/hfsplus/ioctl.c
@@ -57,9 +57,8 @@ static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
 	return 0;
 }
 
-static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
+static inline unsigned int hfsplus_getflags(struct inode *inode)
 {
-	struct inode *inode = file_inode(file);
 	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	unsigned int flags = 0;
 
@@ -69,6 +68,13 @@ static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
 		flags |= FS_APPEND_FL;
 	if (hip->userflags & HFSPLUS_FLG_NODUMP)
 		flags |= FS_NODUMP_FL;
+	return flags;
+}
+
+static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
+{
+	struct inode *inode = file_inode(file);
+	unsigned int flags = hfsplus_getflags(inode);
 
 	return put_user(flags, user_flags);
 }
@@ -78,6 +84,7 @@ static int hfsplus_ioctl_setflags(struct file *file, int __user *user_flags)
 	struct inode *inode = file_inode(file);
 	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	unsigned int flags, new_fl = 0;
+	unsigned int oldflags = hfsplus_getflags(inode);
 	int err = 0;
 
 	err = mnt_want_write_file(file);
@@ -96,13 +103,9 @@ static int hfsplus_ioctl_setflags(struct file *file, int __user *user_flags)
 
 	inode_lock(inode);
 
-	if ((flags & (FS_IMMUTABLE_FL|FS_APPEND_FL)) ||
-	    inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
-		if (!capable(CAP_LINUX_IMMUTABLE)) {
-			err = -EPERM;
-			goto out_unlock_inode;
-		}
-	}
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		goto out_unlock_inode;
 
 	/* don't silently ignore unsupported ext2 flags */
 	if (flags & ~(FS_IMMUTABLE_FL|FS_APPEND_FL|FS_NODUMP_FL)) {
diff --git a/fs/inode.c b/fs/inode.c
index df6542ec3b88..0ce60b720608 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2170,3 +2170,20 @@ struct timespec64 current_time(struct inode *inode)
 	return timespec64_trunc(now, inode->i_sb->s_time_gran);
 }
 EXPORT_SYMBOL(current_time);
+
+/* Generic function to check FS_IOC_SETFLAGS values. */
+int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags)
+{
+	/*
+	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
+	 * the relevant capability.
+	 *
+	 * This test looks nicer. Thanks to Pauline Middelink
+	 */
+	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
+	    !capable(CAP_LINUX_IMMUTABLE))
+		return -EPERM;
+
+	return 0;
+}
+EXPORT_SYMBOL(vfs_ioc_setflags_check);
diff --git a/fs/jfs/ioctl.c b/fs/jfs/ioctl.c
index ba34dae8bd9f..b485c2d7620f 100644
--- a/fs/jfs/ioctl.c
+++ b/fs/jfs/ioctl.c
@@ -98,24 +98,16 @@ long jfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		/* Lock against other parallel changes of flags */
 		inode_lock(inode);
 
-		oldflags = jfs_inode->mode2;
-
-		/*
-		 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-		 * the relevant capability.
-		 */
-		if ((oldflags & JFS_IMMUTABLE_FL) ||
-			((flags ^ oldflags) &
-			(JFS_APPEND_FL | JFS_IMMUTABLE_FL))) {
-			if (!capable(CAP_LINUX_IMMUTABLE)) {
-				inode_unlock(inode);
-				err = -EPERM;
-				goto setflags_out;
-			}
+		oldflags = jfs_map_ext2(jfs_inode->mode2 & JFS_FL_USER_VISIBLE,
+					0);
+		err = vfs_ioc_setflags_check(inode, oldflags, flags);
+		if (err) {
+			inode_unlock(inode);
+			goto setflags_out;
 		}
 
 		flags = flags & JFS_FL_USER_MODIFIABLE;
-		flags |= oldflags & ~JFS_FL_USER_MODIFIABLE;
+		flags |= jfs_inode->mode2 & ~JFS_FL_USER_MODIFIABLE;
 		jfs_inode->mode2 = flags;
 
 		jfs_set_inode_flags(inode);
diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index 9b96d79eea6c..0632336d2515 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -148,13 +148,8 @@ static int nilfs_ioctl_setflags(struct inode *inode, struct file *filp,
 
 	oldflags = NILFS_I(inode)->i_flags;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by the
-	 * relevant capability.
-	 */
-	ret = -EPERM;
-	if (((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) &&
-	    !capable(CAP_LINUX_IMMUTABLE))
+	ret = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (ret)
 		goto out;
 
 	ret = nilfs_transaction_begin(inode->i_sb, &ti, 0);
diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
index 994726ada857..467a2faf0305 100644
--- a/fs/ocfs2/ioctl.c
+++ b/fs/ocfs2/ioctl.c
@@ -106,16 +106,9 @@ static int ocfs2_set_inode_attr(struct inode *inode, unsigned flags,
 	flags = flags & mask;
 	flags |= oldflags & ~mask;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-	 * the relevant capability.
-	 */
-	status = -EPERM;
-	if ((oldflags & OCFS2_IMMUTABLE_FL) || ((flags ^ oldflags) &
-		(OCFS2_APPEND_FL | OCFS2_IMMUTABLE_FL))) {
-		if (!capable(CAP_LINUX_IMMUTABLE))
-			goto bail_unlock;
-	}
+	status = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (status)
+		goto bail_unlock;
 
 	handle = ocfs2_start_trans(osb, OCFS2_INODE_UPDATE_CREDITS);
 	if (IS_ERR(handle)) {
diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c
index acbbaf7a0bb2..92bcb1ecd994 100644
--- a/fs/reiserfs/ioctl.c
+++ b/fs/reiserfs/ioctl.c
@@ -74,13 +74,11 @@ long reiserfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 				err = -EPERM;
 				goto setflags_out;
 			}
-			if (((flags ^ REISERFS_I(inode)->
-			      i_attrs) & (REISERFS_IMMUTABLE_FL |
-					  REISERFS_APPEND_FL))
-			    && !capable(CAP_LINUX_IMMUTABLE)) {
-				err = -EPERM;
+			err = vfs_ioc_setflags_check(inode,
+						     REISERFS_I(inode)->i_attrs,
+						     flags);
+			if (err)
 				goto setflags_out;
-			}
 			if ((flags & REISERFS_NOTAIL_FL) &&
 			    S_ISREG(inode->i_mode)) {
 				int result;
diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
index 4f1a397fda69..bdea836fc38b 100644
--- a/fs/ubifs/ioctl.c
+++ b/fs/ubifs/ioctl.c
@@ -107,18 +107,11 @@ static int setflags(struct inode *inode, int flags)
 	if (err)
 		return err;
 
-	/*
-	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
-	 * the relevant capability.
-	 */
 	mutex_lock(&ui->ui_mutex);
 	oldflags = ubifs2ioctl(ui->flags);
-	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
-		if (!capable(CAP_LINUX_IMMUTABLE)) {
-			err = -EPERM;
-			goto out_unlock;
-		}
-	}
+	err = vfs_ioc_setflags_check(inode, oldflags, flags);
+	if (err)
+		goto out_unlock;
 
 	ui->flags = ioctl2ubifs(flags);
 	ubifs_set_inode_flags(inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f7fdfe93e25d..1825d055808c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3546,4 +3546,6 @@ static inline struct sock *io_uring_get_socket(struct file *file)
 }
 #endif
 
+int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
+
 #endif /* _LINUX_FS_H */

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
  2019-06-12  0:42   ` [PATCH v2 " Darrick J. Wong
@ 2019-06-20 13:34     ` Jan Kara
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2019-06-20 13:34 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: matthew.garrett, yuchao0, tytso, shaggy, ard.biesheuvel, josef,
	clm, adilger.kernel, jk, jack, dsterba, jaegeuk, viro,
	cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

On Tue 11-06-19 17:42:58, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
> values so that we can standardize the implementations that follow ext4's
> flag values.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> v2: fix jfs locking and remove its opencoded flags check
> ---
>  fs/btrfs/ioctl.c    |   13 +++++--------
>  fs/efivarfs/file.c  |   18 +++++++++++++-----
>  fs/ext2/ioctl.c     |   16 ++++------------
>  fs/ext4/ioctl.c     |   13 +++----------
>  fs/f2fs/file.c      |    7 ++++---
>  fs/gfs2/file.c      |   42 +++++++++++++++++++++++++++++-------------
>  fs/hfsplus/ioctl.c  |   21 ++++++++++++---------
>  fs/inode.c          |   17 +++++++++++++++++
>  fs/jfs/ioctl.c      |   22 +++++++---------------
>  fs/nilfs2/ioctl.c   |    9 ++-------
>  fs/ocfs2/ioctl.c    |   13 +++----------
>  fs/reiserfs/ioctl.c |   10 ++++------
>  fs/ubifs/ioctl.c    |   13 +++----------
>  include/linux/fs.h  |    2 ++
>  14 files changed, 108 insertions(+), 108 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 6dafa857bbb9..f408aa93b0cf 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -187,7 +187,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	struct btrfs_inode *binode = BTRFS_I(inode);
>  	struct btrfs_root *root = binode->root;
>  	struct btrfs_trans_handle *trans;
> -	unsigned int fsflags;
> +	unsigned int fsflags, old_fsflags;
>  	int ret;
>  	const char *comp = NULL;
>  	u32 binode_flags = binode->flags;
> @@ -212,13 +212,10 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	inode_lock(inode);
>  
>  	fsflags = btrfs_mask_fsflags_for_type(inode, fsflags);
> -	if ((fsflags ^ btrfs_inode_flags_to_fsflags(binode->flags)) &
> -	    (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
> -		if (!capable(CAP_LINUX_IMMUTABLE)) {
> -			ret = -EPERM;
> -			goto out_unlock;
> -		}
> -	}
> +	old_fsflags = btrfs_inode_flags_to_fsflags(binode->flags);
> +	ret = vfs_ioc_setflags_check(inode, old_fsflags, fsflags);
> +	if (ret)
> +		goto out_unlock;
>  
>  	if (fsflags & FS_SYNC_FL)
>  		binode_flags |= BTRFS_INODE_SYNC;
> diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> index 8e568428c88b..f4f6c1bec132 100644
> --- a/fs/efivarfs/file.c
> +++ b/fs/efivarfs/file.c
> @@ -110,16 +110,22 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
>  	return size;
>  }
>  
> -static int
> -efivarfs_ioc_getxflags(struct file *file, void __user *arg)
> +static inline unsigned int efivarfs_getflags(struct inode *inode)
>  {
> -	struct inode *inode = file->f_mapping->host;
>  	unsigned int i_flags;
>  	unsigned int flags = 0;
>  
>  	i_flags = inode->i_flags;
>  	if (i_flags & S_IMMUTABLE)
>  		flags |= FS_IMMUTABLE_FL;
> +	return flags;
> +}
> +
> +static int
> +efivarfs_ioc_getxflags(struct file *file, void __user *arg)
> +{
> +	struct inode *inode = file->f_mapping->host;
> +	unsigned int flags = efivarfs_getflags(inode);
>  
>  	if (copy_to_user(arg, &flags, sizeof(flags)))
>  		return -EFAULT;
> @@ -132,6 +138,7 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
>  	struct inode *inode = file->f_mapping->host;
>  	unsigned int flags;
>  	unsigned int i_flags = 0;
> +	unsigned int oldflags = efivarfs_getflags(inode);
>  	int error;
>  
>  	if (!inode_owner_or_capable(inode))
> @@ -143,8 +150,9 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
>  	if (flags & ~FS_IMMUTABLE_FL)
>  		return -EOPNOTSUPP;
>  
> -	if (!capable(CAP_LINUX_IMMUTABLE))
> -		return -EPERM;
> +	error = vfs_ioc_setflags_check(inode, oldflags, flags);
> +	if (error)
> +		return error;
>  
>  	if (flags & FS_IMMUTABLE_FL)
>  		i_flags |= S_IMMUTABLE;
> diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
> index 0367c0039e68..88b3b9720023 100644
> --- a/fs/ext2/ioctl.c
> +++ b/fs/ext2/ioctl.c
> @@ -60,18 +60,10 @@ long ext2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		}
>  		oldflags = ei->i_flags;
>  
> -		/*
> -		 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> -		 * the relevant capability.
> -		 *
> -		 * This test looks nicer. Thanks to Pauline Middelink
> -		 */
> -		if ((flags ^ oldflags) & (EXT2_APPEND_FL | EXT2_IMMUTABLE_FL)) {
> -			if (!capable(CAP_LINUX_IMMUTABLE)) {
> -				inode_unlock(inode);
> -				ret = -EPERM;
> -				goto setflags_out;
> -			}
> +		ret = vfs_ioc_setflags_check(inode, oldflags, flags);
> +		if (ret) {
> +			inode_unlock(inode);
> +			goto setflags_out;
>  		}
>  
>  		flags = flags & EXT2_FL_USER_MODIFIABLE;
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index e486e49b31ed..5126ee351a84 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -289,16 +289,9 @@ static int ext4_ioctl_setflags(struct inode *inode,
>  	/* The JOURNAL_DATA flag is modifiable only by root */
>  	jflag = flags & EXT4_JOURNAL_DATA_FL;
>  
> -	/*
> -	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> -	 * the relevant capability.
> -	 *
> -	 * This test looks nicer. Thanks to Pauline Middelink
> -	 */
> -	if ((flags ^ oldflags) & (EXT4_APPEND_FL | EXT4_IMMUTABLE_FL)) {
> -		if (!capable(CAP_LINUX_IMMUTABLE))
> -			goto flags_out;
> -	}
> +	err = vfs_ioc_setflags_check(inode, oldflags, flags);
> +	if (err)
> +		goto flags_out;
>  
>  	/*
>  	 * The JOURNAL_DATA flag can only be changed by
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 45b45f37d347..a969d5497e03 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1670,6 +1670,7 @@ static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags)
>  {
>  	struct f2fs_inode_info *fi = F2FS_I(inode);
>  	unsigned int oldflags;
> +	int err;
>  
>  	/* Is it quota file? Do not allow user to mess with it */
>  	if (IS_NOQUOTA(inode))
> @@ -1679,9 +1680,9 @@ static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags)
>  
>  	oldflags = fi->i_flags;
>  
> -	if ((flags ^ oldflags) & (F2FS_APPEND_FL | F2FS_IMMUTABLE_FL))
> -		if (!capable(CAP_LINUX_IMMUTABLE))
> -			return -EPERM;
> +	err = vfs_ioc_setflags_check(inode, oldflags, flags);
> +	if (err)
> +		return err;
>  
>  	flags = flags & F2FS_FL_USER_MODIFIABLE;
>  	flags |= oldflags & ~F2FS_FL_USER_MODIFIABLE;
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index d174b1f8fd08..99f53cf699c6 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -136,27 +136,36 @@ static struct {
>  	{FS_JOURNAL_DATA_FL, GFS2_DIF_JDATA | GFS2_DIF_INHERIT_JDATA},
>  };
>  
> +static inline u32 gfs2_gfsflags_to_fsflags(struct inode *inode, u32 gfsflags)
> +{
> +	int i;
> +	u32 fsflags = 0;
> +
> +	if (S_ISDIR(inode->i_mode))
> +		gfsflags &= ~GFS2_DIF_JDATA;
> +	else
> +		gfsflags &= ~GFS2_DIF_INHERIT_JDATA;
> +
> +	for (i = 0; i < ARRAY_SIZE(fsflag_gfs2flag); i++)
> +		if (gfsflags & fsflag_gfs2flag[i].gfsflag)
> +			fsflags |= fsflag_gfs2flag[i].fsflag;
> +	return fsflags;
> +}
> +
>  static int gfs2_get_flags(struct file *filp, u32 __user *ptr)
>  {
>  	struct inode *inode = file_inode(filp);
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	struct gfs2_holder gh;
> -	int i, error;
> -	u32 gfsflags, fsflags = 0;
> +	int error;
> +	u32 fsflags;
>  
>  	gfs2_holder_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
>  	error = gfs2_glock_nq(&gh);
>  	if (error)
>  		goto out_uninit;
>  
> -	gfsflags = ip->i_diskflags;
> -	if (S_ISDIR(inode->i_mode))
> -		gfsflags &= ~GFS2_DIF_JDATA;
> -	else
> -		gfsflags &= ~GFS2_DIF_INHERIT_JDATA;
> -	for (i = 0; i < ARRAY_SIZE(fsflag_gfs2flag); i++)
> -		if (gfsflags & fsflag_gfs2flag[i].gfsflag)
> -			fsflags |= fsflag_gfs2flag[i].fsflag;
> +	fsflags = gfs2_gfsflags_to_fsflags(inode, ip->i_diskflags);
>  
>  	if (put_user(fsflags, ptr))
>  		error = -EFAULT;
> @@ -200,9 +209,11 @@ void gfs2_set_inode_flags(struct inode *inode)
>   * @filp: file pointer
>   * @reqflags: The flags to set
>   * @mask: Indicates which flags are valid
> + * @fsflags: The FS_* inode flags passed in
>   *
>   */
> -static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
> +static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask,
> +			     const u32 fsflags)
>  {
>  	struct inode *inode = file_inode(filp);
>  	struct gfs2_inode *ip = GFS2_I(inode);
> @@ -210,7 +221,7 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
>  	struct buffer_head *bh;
>  	struct gfs2_holder gh;
>  	int error;
> -	u32 new_flags, flags;
> +	u32 new_flags, flags, oldflags;
>  
>  	error = mnt_want_write_file(filp);
>  	if (error)
> @@ -220,6 +231,11 @@ static int do_gfs2_set_flags(struct file *filp, u32 reqflags, u32 mask)
>  	if (error)
>  		goto out_drop_write;
>  
> +	oldflags = gfs2_gfsflags_to_fsflags(inode, ip->i_diskflags);
> +	error = vfs_ioc_setflags_check(inode, oldflags, fsflags);
> +	if (error)
> +		goto out;
> +
>  	error = -EACCES;
>  	if (!inode_owner_or_capable(inode))
>  		goto out;
> @@ -308,7 +324,7 @@ static int gfs2_set_flags(struct file *filp, u32 __user *ptr)
>  		mask &= ~(GFS2_DIF_TOPDIR | GFS2_DIF_INHERIT_JDATA);
>  	}
>  
> -	return do_gfs2_set_flags(filp, gfsflags, mask);
> +	return do_gfs2_set_flags(filp, gfsflags, mask, fsflags);
>  }
>  
>  static int gfs2_getlabel(struct file *filp, char __user *label)
> diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
> index 5e6502ef7415..862a3c9481d7 100644
> --- a/fs/hfsplus/ioctl.c
> +++ b/fs/hfsplus/ioctl.c
> @@ -57,9 +57,8 @@ static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
>  	return 0;
>  }
>  
> -static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
> +static inline unsigned int hfsplus_getflags(struct inode *inode)
>  {
> -	struct inode *inode = file_inode(file);
>  	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
>  	unsigned int flags = 0;
>  
> @@ -69,6 +68,13 @@ static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
>  		flags |= FS_APPEND_FL;
>  	if (hip->userflags & HFSPLUS_FLG_NODUMP)
>  		flags |= FS_NODUMP_FL;
> +	return flags;
> +}
> +
> +static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
> +{
> +	struct inode *inode = file_inode(file);
> +	unsigned int flags = hfsplus_getflags(inode);
>  
>  	return put_user(flags, user_flags);
>  }
> @@ -78,6 +84,7 @@ static int hfsplus_ioctl_setflags(struct file *file, int __user *user_flags)
>  	struct inode *inode = file_inode(file);
>  	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
>  	unsigned int flags, new_fl = 0;
> +	unsigned int oldflags = hfsplus_getflags(inode);
>  	int err = 0;
>  
>  	err = mnt_want_write_file(file);
> @@ -96,13 +103,9 @@ static int hfsplus_ioctl_setflags(struct file *file, int __user *user_flags)
>  
>  	inode_lock(inode);
>  
> -	if ((flags & (FS_IMMUTABLE_FL|FS_APPEND_FL)) ||
> -	    inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
> -		if (!capable(CAP_LINUX_IMMUTABLE)) {
> -			err = -EPERM;
> -			goto out_unlock_inode;
> -		}
> -	}
> +	err = vfs_ioc_setflags_check(inode, oldflags, flags);
> +	if (err)
> +		goto out_unlock_inode;
>  
>  	/* don't silently ignore unsupported ext2 flags */
>  	if (flags & ~(FS_IMMUTABLE_FL|FS_APPEND_FL|FS_NODUMP_FL)) {
> diff --git a/fs/inode.c b/fs/inode.c
> index df6542ec3b88..0ce60b720608 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2170,3 +2170,20 @@ struct timespec64 current_time(struct inode *inode)
>  	return timespec64_trunc(now, inode->i_sb->s_time_gran);
>  }
>  EXPORT_SYMBOL(current_time);
> +
> +/* Generic function to check FS_IOC_SETFLAGS values. */
> +int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags)
> +{
> +	/*
> +	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> +	 * the relevant capability.
> +	 *
> +	 * This test looks nicer. Thanks to Pauline Middelink
> +	 */
> +	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
> +	    !capable(CAP_LINUX_IMMUTABLE))
> +		return -EPERM;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(vfs_ioc_setflags_check);
> diff --git a/fs/jfs/ioctl.c b/fs/jfs/ioctl.c
> index ba34dae8bd9f..b485c2d7620f 100644
> --- a/fs/jfs/ioctl.c
> +++ b/fs/jfs/ioctl.c
> @@ -98,24 +98,16 @@ long jfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		/* Lock against other parallel changes of flags */
>  		inode_lock(inode);
>  
> -		oldflags = jfs_inode->mode2;
> -
> -		/*
> -		 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> -		 * the relevant capability.
> -		 */
> -		if ((oldflags & JFS_IMMUTABLE_FL) ||
> -			((flags ^ oldflags) &
> -			(JFS_APPEND_FL | JFS_IMMUTABLE_FL))) {
> -			if (!capable(CAP_LINUX_IMMUTABLE)) {
> -				inode_unlock(inode);
> -				err = -EPERM;
> -				goto setflags_out;
> -			}
> +		oldflags = jfs_map_ext2(jfs_inode->mode2 & JFS_FL_USER_VISIBLE,
> +					0);
> +		err = vfs_ioc_setflags_check(inode, oldflags, flags);
> +		if (err) {
> +			inode_unlock(inode);
> +			goto setflags_out;
>  		}
>  
>  		flags = flags & JFS_FL_USER_MODIFIABLE;
> -		flags |= oldflags & ~JFS_FL_USER_MODIFIABLE;
> +		flags |= jfs_inode->mode2 & ~JFS_FL_USER_MODIFIABLE;
>  		jfs_inode->mode2 = flags;
>  
>  		jfs_set_inode_flags(inode);
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index 9b96d79eea6c..0632336d2515 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -148,13 +148,8 @@ static int nilfs_ioctl_setflags(struct inode *inode, struct file *filp,
>  
>  	oldflags = NILFS_I(inode)->i_flags;
>  
> -	/*
> -	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by the
> -	 * relevant capability.
> -	 */
> -	ret = -EPERM;
> -	if (((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) &&
> -	    !capable(CAP_LINUX_IMMUTABLE))
> +	ret = vfs_ioc_setflags_check(inode, oldflags, flags);
> +	if (ret)
>  		goto out;
>  
>  	ret = nilfs_transaction_begin(inode->i_sb, &ti, 0);
> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
> index 994726ada857..467a2faf0305 100644
> --- a/fs/ocfs2/ioctl.c
> +++ b/fs/ocfs2/ioctl.c
> @@ -106,16 +106,9 @@ static int ocfs2_set_inode_attr(struct inode *inode, unsigned flags,
>  	flags = flags & mask;
>  	flags |= oldflags & ~mask;
>  
> -	/*
> -	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> -	 * the relevant capability.
> -	 */
> -	status = -EPERM;
> -	if ((oldflags & OCFS2_IMMUTABLE_FL) || ((flags ^ oldflags) &
> -		(OCFS2_APPEND_FL | OCFS2_IMMUTABLE_FL))) {
> -		if (!capable(CAP_LINUX_IMMUTABLE))
> -			goto bail_unlock;
> -	}
> +	status = vfs_ioc_setflags_check(inode, oldflags, flags);
> +	if (status)
> +		goto bail_unlock;
>  
>  	handle = ocfs2_start_trans(osb, OCFS2_INODE_UPDATE_CREDITS);
>  	if (IS_ERR(handle)) {
> diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c
> index acbbaf7a0bb2..92bcb1ecd994 100644
> --- a/fs/reiserfs/ioctl.c
> +++ b/fs/reiserfs/ioctl.c
> @@ -74,13 +74,11 @@ long reiserfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  				err = -EPERM;
>  				goto setflags_out;
>  			}
> -			if (((flags ^ REISERFS_I(inode)->
> -			      i_attrs) & (REISERFS_IMMUTABLE_FL |
> -					  REISERFS_APPEND_FL))
> -			    && !capable(CAP_LINUX_IMMUTABLE)) {
> -				err = -EPERM;
> +			err = vfs_ioc_setflags_check(inode,
> +						     REISERFS_I(inode)->i_attrs,
> +						     flags);
> +			if (err)
>  				goto setflags_out;
> -			}
>  			if ((flags & REISERFS_NOTAIL_FL) &&
>  			    S_ISREG(inode->i_mode)) {
>  				int result;
> diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
> index 4f1a397fda69..bdea836fc38b 100644
> --- a/fs/ubifs/ioctl.c
> +++ b/fs/ubifs/ioctl.c
> @@ -107,18 +107,11 @@ static int setflags(struct inode *inode, int flags)
>  	if (err)
>  		return err;
>  
> -	/*
> -	 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> -	 * the relevant capability.
> -	 */
>  	mutex_lock(&ui->ui_mutex);
>  	oldflags = ubifs2ioctl(ui->flags);
> -	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
> -		if (!capable(CAP_LINUX_IMMUTABLE)) {
> -			err = -EPERM;
> -			goto out_unlock;
> -		}
> -	}
> +	err = vfs_ioc_setflags_check(inode, oldflags, flags);
> +	if (err)
> +		goto out_unlock;
>  
>  	ui->flags = ioctl2ubifs(flags);
>  	ubifs_set_inode_flags(inode);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index f7fdfe93e25d..1825d055808c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3546,4 +3546,6 @@ static inline struct sock *io_uring_get_socket(struct file *file)
>  }
>  #endif
>  
> +int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
> +
>  #endif /* _LINUX_FS_H */
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR
  2019-06-11  4:45 ` [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR Darrick J. Wong
@ 2019-06-20 13:38   ` Jan Kara
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2019-06-20 13:38 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: matthew.garrett, yuchao0, tytso, shaggy, ard.biesheuvel, josef,
	clm, adilger.kernel, jk, jack, dsterba, jaegeuk, viro,
	cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

On Mon 10-06-19 21:45:49, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a generic checking function for the incoming FS_IOC_FSSETXATTR
> fsxattr values so that we can standardize some of the implementation
> behaviors.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/btrfs/ioctl.c   |   21 +++++++++-------
>  fs/ext4/ioctl.c    |   27 ++++++++++++++------
>  fs/f2fs/file.c     |   26 ++++++++++++++-----
>  fs/inode.c         |   17 +++++++++++++
>  fs/xfs/xfs_ioctl.c |   70 ++++++++++++++++++++++++++++++----------------------
>  include/linux/fs.h |    3 ++
>  6 files changed, 111 insertions(+), 53 deletions(-)
> 
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index f408aa93b0cf..7ddda5b4b6a6 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -366,6 +366,13 @@ static int check_xflags(unsigned int flags)
>  	return 0;
>  }
>  
> +static void __btrfs_ioctl_fsgetxattr(struct btrfs_inode *binode,
> +				     struct fsxattr *fa)
> +{
> +	memset(fa, 0, sizeof(*fa));
> +	fa->fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
> +}
> +
>  /*
>   * Set the xflags from the internal inode flags. The remaining items of fsxattr
>   * are zeroed.
> @@ -375,8 +382,7 @@ static int btrfs_ioctl_fsgetxattr(struct file *file, void __user *arg)
>  	struct btrfs_inode *binode = BTRFS_I(file_inode(file));
>  	struct fsxattr fa;
>  
> -	memset(&fa, 0, sizeof(fa));
> -	fa.fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
> +	__btrfs_ioctl_fsgetxattr(binode, &fa);
>  
>  	if (copy_to_user(arg, &fa, sizeof(fa)))
>  		return -EFAULT;
> @@ -390,7 +396,7 @@ static int btrfs_ioctl_fssetxattr(struct file *file, void __user *arg)
>  	struct btrfs_inode *binode = BTRFS_I(inode);
>  	struct btrfs_root *root = binode->root;
>  	struct btrfs_trans_handle *trans;
> -	struct fsxattr fa;
> +	struct fsxattr fa, old_fa;
>  	unsigned old_flags;
>  	unsigned old_i_flags;
>  	int ret = 0;
> @@ -421,13 +427,10 @@ static int btrfs_ioctl_fssetxattr(struct file *file, void __user *arg)
>  	old_flags = binode->flags;
>  	old_i_flags = inode->i_flags;
>  
> -	/* We need the capabilities to change append-only or immutable inode */
> -	if (((old_flags & (BTRFS_INODE_APPEND | BTRFS_INODE_IMMUTABLE)) ||
> -	     (fa.fsx_xflags & (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE))) &&
> -	    !capable(CAP_LINUX_IMMUTABLE)) {
> -		ret = -EPERM;
> +	__btrfs_ioctl_fsgetxattr(binode, &old_fa);
> +	ret = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
> +	if (ret)
>  		goto out_unlock;
> -	}
>  
>  	if (fa.fsx_xflags & FS_XFLAG_SYNC)
>  		binode->flags |= BTRFS_INODE_SYNC;
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 5126ee351a84..c2f48c90ca45 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -721,6 +721,19 @@ static int ext4_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
>  	return 0;
>  }
>  
> +static void ext4_fsgetxattr(struct inode *inode, struct fsxattr *fa)
> +{
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +
> +	memset(fa, 0, sizeof(struct fsxattr));
> +	fa->fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & EXT4_FL_USER_VISIBLE);
> +
> +	if (ext4_has_feature_project(inode->i_sb)) {
> +		fa->fsx_projid = (__u32)from_kprojid(&init_user_ns,
> +				ei->i_projid);
> +	}
> +}
> +
>  long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>  	struct inode *inode = file_inode(filp);
> @@ -1089,13 +1102,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	{
>  		struct fsxattr fa;
>  
> -		memset(&fa, 0, sizeof(struct fsxattr));
> -		fa.fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & EXT4_FL_USER_VISIBLE);
> -
> -		if (ext4_has_feature_project(inode->i_sb)) {
> -			fa.fsx_projid = (__u32)from_kprojid(&init_user_ns,
> -				EXT4_I(inode)->i_projid);
> -		}
> +		ext4_fsgetxattr(inode, &fa);
>  
>  		if (copy_to_user((struct fsxattr __user *)arg,
>  				 &fa, sizeof(fa)))
> @@ -1104,7 +1111,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	}
>  	case EXT4_IOC_FSSETXATTR:
>  	{
> -		struct fsxattr fa;
> +		struct fsxattr fa, old_fa;
>  		int err;
>  
>  		if (copy_from_user(&fa, (struct fsxattr __user *)arg,
> @@ -1127,7 +1134,11 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			return err;
>  
>  		inode_lock(inode);
> +		ext4_fsgetxattr(inode, &old_fa);
>  		err = ext4_ioctl_check_project(inode, &fa);
> +		if (err)
> +			goto out;
> +		err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
>  		if (err)
>  			goto out;
>  		flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index a969d5497e03..f707de6bd4a8 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2773,19 +2773,26 @@ static inline unsigned long f2fs_xflags_to_iflags(__u32 xflags)
>  	return iflags;
>  }
>  
> -static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
> +static void __f2fs_ioc_fsgetxattr(struct inode *inode,
> +				  struct fsxattr *fa)
>  {
> -	struct inode *inode = file_inode(filp);
>  	struct f2fs_inode_info *fi = F2FS_I(inode);
> -	struct fsxattr fa;
>  
> -	memset(&fa, 0, sizeof(struct fsxattr));
> -	fa.fsx_xflags = f2fs_iflags_to_xflags(fi->i_flags &
> +	memset(fa, 0, sizeof(struct fsxattr));
> +	fa->fsx_xflags = f2fs_iflags_to_xflags(fi->i_flags &
>  				F2FS_FL_USER_VISIBLE);
>  
>  	if (f2fs_sb_has_project_quota(F2FS_I_SB(inode)))
> -		fa.fsx_projid = (__u32)from_kprojid(&init_user_ns,
> +		fa->fsx_projid = (__u32)from_kprojid(&init_user_ns,
>  							fi->i_projid);
> +}
> +
> +static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
> +{
> +	struct inode *inode = file_inode(filp);
> +	struct fsxattr fa;
> +
> +	__f2fs_ioc_fsgetxattr(inode, &fa);
>  
>  	if (copy_to_user((struct fsxattr __user *)arg, &fa, sizeof(fa)))
>  		return -EFAULT;
> @@ -2820,7 +2827,7 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
>  {
>  	struct inode *inode = file_inode(filp);
>  	struct f2fs_inode_info *fi = F2FS_I(inode);
> -	struct fsxattr fa;
> +	struct fsxattr fa, old_fa;
>  	unsigned int flags;
>  	int err;
>  
> @@ -2844,6 +2851,11 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
>  
>  	inode_lock(inode);
>  	err = f2fs_ioctl_check_project(inode, &fa);
> +	if (err)
> +		goto out;
> +
> +	__f2fs_ioc_fsgetxattr(inode, &old_fa);
> +	err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
>  	if (err)
>  		goto out;
>  	flags = (fi->i_flags & ~F2FS_FL_XFLAG_VISIBLE) |
> diff --git a/fs/inode.c b/fs/inode.c
> index 0ce60b720608..026955258a47 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2187,3 +2187,20 @@ int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags)
>  	return 0;
>  }
>  EXPORT_SYMBOL(vfs_ioc_setflags_check);
> +
> +/* Generic function to check FS_IOC_FSSETXATTR values. */
> +int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
> +			     struct fsxattr *fa)
> +{
> +	/*
> +	 * Can't modify an immutable/append-only file unless we have
> +	 * appropriate permission.
> +	 */
> +	if ((old_fa->fsx_xflags ^ fa->fsx_xflags) &
> +			(FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND) &&
> +	    !capable(CAP_LINUX_IMMUTABLE))
> +		return -EPERM;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d7dfc13f30f5..08c24f2f55c3 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -879,37 +879,45 @@ xfs_di2lxflags(
>  	return flags;
>  }
>  
> -STATIC int
> -xfs_ioc_fsgetxattr(
> -	xfs_inode_t		*ip,
> -	int			attr,
> -	void			__user *arg)
> +static void
> +__xfs_ioc_fsgetxattr(
> +	struct xfs_inode	*ip,
> +	bool			attr,
> +	struct fsxattr		*fa)
>  {
> -	struct fsxattr		fa;
> -
> -	memset(&fa, 0, sizeof(struct fsxattr));
> -
> -	xfs_ilock(ip, XFS_ILOCK_SHARED);
> -	fa.fsx_xflags = xfs_ip2xflags(ip);
> -	fa.fsx_extsize = ip->i_d.di_extsize << ip->i_mount->m_sb.sb_blocklog;
> -	fa.fsx_cowextsize = ip->i_d.di_cowextsize <<
> +	memset(fa, 0, sizeof(struct fsxattr));
> +	fa->fsx_xflags = xfs_ip2xflags(ip);
> +	fa->fsx_extsize = ip->i_d.di_extsize << ip->i_mount->m_sb.sb_blocklog;
> +	fa->fsx_cowextsize = ip->i_d.di_cowextsize <<
>  			ip->i_mount->m_sb.sb_blocklog;
> -	fa.fsx_projid = xfs_get_projid(ip);
> +	fa->fsx_projid = xfs_get_projid(ip);
>  
>  	if (attr) {
>  		if (ip->i_afp) {
>  			if (ip->i_afp->if_flags & XFS_IFEXTENTS)
> -				fa.fsx_nextents = xfs_iext_count(ip->i_afp);
> +				fa->fsx_nextents = xfs_iext_count(ip->i_afp);
>  			else
> -				fa.fsx_nextents = ip->i_d.di_anextents;
> +				fa->fsx_nextents = ip->i_d.di_anextents;
>  		} else
> -			fa.fsx_nextents = 0;
> +			fa->fsx_nextents = 0;
>  	} else {
>  		if (ip->i_df.if_flags & XFS_IFEXTENTS)
> -			fa.fsx_nextents = xfs_iext_count(&ip->i_df);
> +			fa->fsx_nextents = xfs_iext_count(&ip->i_df);
>  		else
> -			fa.fsx_nextents = ip->i_d.di_nextents;
> +			fa->fsx_nextents = ip->i_d.di_nextents;
>  	}
> +}
> +
> +STATIC int
> +xfs_ioc_fsgetxattr(
> +	xfs_inode_t		*ip,
> +	int			attr,
> +	void			__user *arg)
> +{
> +	struct fsxattr		fa;
> +
> +	xfs_ilock(ip, XFS_ILOCK_SHARED);
> +	__xfs_ioc_fsgetxattr(ip, attr, &fa);
>  	xfs_iunlock(ip, XFS_ILOCK_SHARED);
>  
>  	if (copy_to_user(arg, &fa, sizeof(fa)))
> @@ -1035,15 +1043,6 @@ xfs_ioctl_setattr_xflags(
>  	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
>  		return -EINVAL;
>  
> -	/*
> -	 * Can't modify an immutable/append-only file unless
> -	 * we have appropriate permission.
> -	 */
> -	if (((ip->i_d.di_flags & (XFS_DIFLAG_IMMUTABLE | XFS_DIFLAG_APPEND)) ||
> -	     (fa->fsx_xflags & (FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND))) &&
> -	    !capable(CAP_LINUX_IMMUTABLE))
> -		return -EPERM;
> -
>  	/* diflags2 only valid for v3 inodes. */
>  	di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
>  	if (di_flags2 && ip->i_d.di_version < 3)
> @@ -1323,6 +1322,7 @@ xfs_ioctl_setattr(
>  	xfs_inode_t		*ip,
>  	struct fsxattr		*fa)
>  {
> +	struct fsxattr		old_fa;
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_trans	*tp;
>  	struct xfs_dquot	*udqp = NULL;
> @@ -1370,7 +1370,6 @@ xfs_ioctl_setattr(
>  		goto error_free_dquots;
>  	}
>  
> -
>  	if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
>  	    xfs_get_projid(ip) != fa->fsx_projid) {
>  		code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL, pdqp,
> @@ -1379,6 +1378,11 @@ xfs_ioctl_setattr(
>  			goto error_trans_cancel;
>  	}
>  
> +	__xfs_ioc_fsgetxattr(ip, false, &old_fa);
> +	code = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, fa);
> +	if (code)
> +		goto error_trans_cancel;
> +
>  	code = xfs_ioctl_setattr_check_extsize(ip, fa);
>  	if (code)
>  		goto error_trans_cancel;
> @@ -1489,6 +1493,7 @@ xfs_ioc_setxflags(
>  {
>  	struct xfs_trans	*tp;
>  	struct fsxattr		fa;
> +	struct fsxattr		old_fa;
>  	unsigned int		flags;
>  	int			join_flags = 0;
>  	int			error;
> @@ -1524,6 +1529,13 @@ xfs_ioc_setxflags(
>  		goto out_drop_write;
>  	}
>  
> +	__xfs_ioc_fsgetxattr(ip, false, &old_fa);
> +	error = vfs_ioc_fssetxattr_check(VFS_I(ip), &old_fa, &fa);
> +	if (error) {
> +		xfs_trans_cancel(tp);
> +		goto out_drop_write;
> +	}
> +
>  	error = xfs_ioctl_setattr_xflags(tp, ip, &fa);
>  	if (error) {
>  		xfs_trans_cancel(tp);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1825d055808c..8dad3c80b611 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3548,4 +3548,7 @@ static inline struct sock *io_uring_get_socket(struct file *file)
>  
>  int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
>  
> +int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
> +			     struct fsxattr *fa);
> +
>  #endif /* _LINUX_FS_H */
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info
  2019-06-11  4:45 ` [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info Darrick J. Wong
@ 2019-06-20 13:41   ` Jan Kara
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2019-06-20 13:41 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: matthew.garrett, yuchao0, tytso, shaggy, ard.biesheuvel, josef,
	clm, adilger.kernel, jk, jack, dsterba, jaegeuk, viro,
	cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

On Mon 10-06-19 21:45:57, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Standardize the project id checks for FSSETXATTR.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ioctl.c    |   27 ---------------------------
>  fs/f2fs/file.c     |   27 ---------------------------
>  fs/inode.c         |   13 +++++++++++++
>  fs/xfs/xfs_ioctl.c |   15 ---------------
>  4 files changed, 13 insertions(+), 69 deletions(-)
> 
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index c2f48c90ca45..6aa1df1918f7 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -697,30 +697,6 @@ static long ext4_ioctl_group_add(struct file *file,
>  	return err;
>  }
>  
> -static int ext4_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
> -{
> -	/*
> -	 * Project Quota ID state is only allowed to change from within the init
> -	 * namespace. Enforce that restriction only if we are trying to change
> -	 * the quota ID state. Everything else is allowed in user namespaces.
> -	 */
> -	if (current_user_ns() == &init_user_ns)
> -		return 0;
> -
> -	if (__kprojid_val(EXT4_I(inode)->i_projid) != fa->fsx_projid)
> -		return -EINVAL;
> -
> -	if (ext4_test_inode_flag(inode, EXT4_INODE_PROJINHERIT)) {
> -		if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
> -			return -EINVAL;
> -	} else {
> -		if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
> -			return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>  static void ext4_fsgetxattr(struct inode *inode, struct fsxattr *fa)
>  {
>  	struct ext4_inode_info *ei = EXT4_I(inode);
> @@ -1135,9 +1111,6 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  
>  		inode_lock(inode);
>  		ext4_fsgetxattr(inode, &old_fa);
> -		err = ext4_ioctl_check_project(inode, &fa);
> -		if (err)
> -			goto out;
>  		err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
>  		if (err)
>  			goto out;
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f707de6bd4a8..183ed1ac60e1 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2799,30 +2799,6 @@ static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
>  	return 0;
>  }
>  
> -static int f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
> -{
> -	/*
> -	 * Project Quota ID state is only allowed to change from within the init
> -	 * namespace. Enforce that restriction only if we are trying to change
> -	 * the quota ID state. Everything else is allowed in user namespaces.
> -	 */
> -	if (current_user_ns() == &init_user_ns)
> -		return 0;
> -
> -	if (__kprojid_val(F2FS_I(inode)->i_projid) != fa->fsx_projid)
> -		return -EINVAL;
> -
> -	if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) {
> -		if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
> -			return -EINVAL;
> -	} else {
> -		if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
> -			return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>  static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
>  {
>  	struct inode *inode = file_inode(filp);
> @@ -2850,9 +2826,6 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
>  		return err;
>  
>  	inode_lock(inode);
> -	err = f2fs_ioctl_check_project(inode, &fa);
> -	if (err)
> -		goto out;
>  
>  	__f2fs_ioc_fsgetxattr(inode, &old_fa);
>  	err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
> diff --git a/fs/inode.c b/fs/inode.c
> index 026955258a47..40ecd3a6a188 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2201,6 +2201,19 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
>  	    !capable(CAP_LINUX_IMMUTABLE))
>  		return -EPERM;
>  
> +	/*
> +	 * Project Quota ID state is only allowed to change from within the init
> +	 * namespace. Enforce that restriction only if we are trying to change
> +	 * the quota ID state. Everything else is allowed in user namespaces.
> +	 */
> +	if (current_user_ns() != &init_user_ns) {
> +		if (old_fa->fsx_projid != fa->fsx_projid)
> +			return -EINVAL;
> +		if ((old_fa->fsx_xflags ^ fa->fsx_xflags) &
> +				FS_XFLAG_PROJINHERIT)
> +			return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 08c24f2f55c3..82961de98900 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1299,21 +1299,6 @@ xfs_ioctl_setattr_check_projid(
>  	if (fa->fsx_projid > (uint16_t)-1 &&
>  	    !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
>  		return -EINVAL;
> -
> -	/*
> -	 * Project Quota ID state is only allowed to change from within the init
> -	 * namespace. Enforce that restriction only if we are trying to change
> -	 * the quota ID state. Everything else is allowed in user namespaces.
> -	 */
> -	if (current_user_ns() == &init_user_ns)
> -		return 0;
> -
> -	if (xfs_get_projid(ip) != fa->fsx_projid)
> -		return -EINVAL;
> -	if ((fa->fsx_xflags & FS_XFLAG_PROJINHERIT) !=
> -	    (ip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT))
> -		return -EINVAL;
> -
>  	return 0;
>  }
>  
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints
  2019-06-11  4:46 ` [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints Darrick J. Wong
@ 2019-06-20 13:46   ` Jan Kara
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2019-06-20 13:46 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: matthew.garrett, yuchao0, tytso, shaggy, ard.biesheuvel, josef,
	clm, adilger.kernel, jk, jack, dsterba, jaegeuk, viro,
	cluster-devel, jfs-discussion, linux-efi, reiserfs-devel,
	linux-kernel, linux-f2fs-devel, linux-xfs, linux-nilfs,
	linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4, linux-btrfs

On Mon 10-06-19 21:46:05, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the extent size hint checks that aren't xfs-specific to the vfs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/inode.c         |   18 +++++++++++++
>  fs/xfs/xfs_ioctl.c |   70 ++++++++++++++++++++++------------------------------
>  2 files changed, 47 insertions(+), 41 deletions(-)
> 
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 40ecd3a6a188..a3757051fd55 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2214,6 +2214,24 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
>  			return -EINVAL;
>  	}
>  
> +	/* Check extent size hints. */
> +	if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
> +		return -EINVAL;
> +
> +	if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
> +			!S_ISDIR(inode->i_mode))
> +		return -EINVAL;
> +
> +	if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
> +	    !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> +		return -EINVAL;
> +
> +	/* Extent size hints of zero turn off the flags. */
> +	if (fa->fsx_extsize == 0)
> +		fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
> +	if (fa->fsx_cowextsize == 0)
> +		fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 82961de98900..b494e7e881e3 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1201,39 +1201,31 @@ xfs_ioctl_setattr_check_extsize(
>  	struct fsxattr		*fa)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> -
> -	if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(VFS_I(ip)->i_mode))
> -		return -EINVAL;
> -
> -	if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
> -	    !S_ISDIR(VFS_I(ip)->i_mode))
> -		return -EINVAL;
> +	xfs_extlen_t		size;
> +	xfs_fsblock_t		extsize_fsb;
>  
>  	if (S_ISREG(VFS_I(ip)->i_mode) && ip->i_d.di_nextents &&
>  	    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) != fa->fsx_extsize))
>  		return -EINVAL;
>  
> -	if (fa->fsx_extsize != 0) {
> -		xfs_extlen_t    size;
> -		xfs_fsblock_t   extsize_fsb;
> -
> -		extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
> -		if (extsize_fsb > MAXEXTLEN)
> -			return -EINVAL;
> +	if (fa->fsx_extsize == 0)
> +		return 0;
>  
> -		if (XFS_IS_REALTIME_INODE(ip) ||
> -		    (fa->fsx_xflags & FS_XFLAG_REALTIME)) {
> -			size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
> -		} else {
> -			size = mp->m_sb.sb_blocksize;
> -			if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
> -				return -EINVAL;
> -		}
> +	extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
> +	if (extsize_fsb > MAXEXTLEN)
> +		return -EINVAL;
>  
> -		if (fa->fsx_extsize % size)
> +	if (XFS_IS_REALTIME_INODE(ip) ||
> +	    (fa->fsx_xflags & FS_XFLAG_REALTIME)) {
> +		size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
> +	} else {
> +		size = mp->m_sb.sb_blocksize;
> +		if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
>  			return -EINVAL;
> -	} else
> -		fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
> +	}
> +
> +	if (fa->fsx_extsize % size)
> +		return -EINVAL;
>  
>  	return 0;
>  }
> @@ -1259,6 +1251,8 @@ xfs_ioctl_setattr_check_cowextsize(
>  	struct fsxattr		*fa)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_extlen_t		size;
> +	xfs_fsblock_t		cowextsize_fsb;
>  
>  	if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
>  		return 0;
> @@ -1267,25 +1261,19 @@ xfs_ioctl_setattr_check_cowextsize(
>  	    ip->i_d.di_version != 3)
>  		return -EINVAL;
>  
> -	if (!S_ISREG(VFS_I(ip)->i_mode) && !S_ISDIR(VFS_I(ip)->i_mode))
> -		return -EINVAL;
> -
> -	if (fa->fsx_cowextsize != 0) {
> -		xfs_extlen_t    size;
> -		xfs_fsblock_t   cowextsize_fsb;
> +	if (fa->fsx_cowextsize == 0)
> +		return 0;
>  
> -		cowextsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_cowextsize);
> -		if (cowextsize_fsb > MAXEXTLEN)
> -			return -EINVAL;
> +	cowextsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_cowextsize);
> +	if (cowextsize_fsb > MAXEXTLEN)
> +		return -EINVAL;
>  
> -		size = mp->m_sb.sb_blocksize;
> -		if (cowextsize_fsb > mp->m_sb.sb_agblocks / 2)
> -			return -EINVAL;
> +	size = mp->m_sb.sb_blocksize;
> +	if (cowextsize_fsb > mp->m_sb.sb_agblocks / 2)
> +		return -EINVAL;
>  
> -		if (fa->fsx_cowextsize % size)
> -			return -EINVAL;
> -	} else
> -		fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> +	if (fa->fsx_cowextsize % size)
> +		return -EINVAL;
>  
>  	return 0;
>  }
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-06-20 13:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-11  4:45 [PATCH 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
2019-06-11  4:45 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
2019-06-11 13:41   ` [Jfs-discussion] " Dave Kleikamp
2019-06-12  0:35     ` Darrick J. Wong
2019-06-12  0:42   ` [PATCH v2 " Darrick J. Wong
2019-06-20 13:34     ` Jan Kara
2019-06-11  4:45 ` [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR Darrick J. Wong
2019-06-20 13:38   ` Jan Kara
2019-06-11  4:45 ` [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info Darrick J. Wong
2019-06-20 13:41   ` Jan Kara
2019-06-11  4:46 ` [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints Darrick J. Wong
2019-06-20 13:46   ` Jan Kara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).