* [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
2019-06-21 23:56 [PATCH v2 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
@ 2019-06-21 23:56 ` Darrick J. Wong
2019-06-25 10:54 ` Christoph Hellwig
2019-06-25 17:12 ` David Sterba
2019-06-21 23:56 ` [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR Darrick J. Wong
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: Darrick J. Wong @ 2019-06-21 23:56 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, Jan Kara,
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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
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 4348cfb14562..403ddbb81335 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2190,3 +2190,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 79ffa2958bd8..4d6feee18778 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3555,4 +3555,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] 15+ messages in thread
* Re: [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
2019-06-21 23:56 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
@ 2019-06-25 10:54 ` Christoph Hellwig
2019-06-25 17:12 ` David Sterba
1 sibling, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2019-06-25 10:54 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, Jan Kara,
reiserfs-devel, linux-kernel, linux-f2fs-devel, linux-xfs,
linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4,
linux-btrfs
On Fri, Jun 21, 2019 at 04:56:21PM -0700, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
2019-06-21 23:56 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
2019-06-25 10:54 ` Christoph Hellwig
@ 2019-06-25 17:12 ` David Sterba
2019-06-25 18:17 ` Darrick J. Wong
1 sibling, 1 reply; 15+ messages in thread
From: David Sterba @ 2019-06-25 17:12 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, Jan Kara,
reiserfs-devel, linux-kernel, linux-f2fs-devel, linux-xfs,
linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4,
linux-btrfs
On Fri, Jun 21, 2019 at 04:56:21PM -0700, 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.
I checked a few samples what's the type of the flags, there are unsigned
types while the proposed VFS functions take signed type.
> +int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
Specifically ext4 uses unsigned type and his was the original API that
got copied so I'd think that it should unsigned everywhere.
> fs/btrfs/ioctl.c | 13 +++++--------
For the btrfs bits
Acked-by: David Sterba <dsterba@suse.com>
and besides the signedness, the rest of the changes look good to me.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS
2019-06-25 17:12 ` David Sterba
@ 2019-06-25 18:17 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2019-06-25 18:17 UTC (permalink / raw)
To: dsterba, matthew.garrett, yuchao0, tytso, shaggy, ard.biesheuvel,
josef, clm, adilger.kernel, jk, jack, dsterba, jaegeuk, viro,
cluster-devel, jfs-discussion, linux-efi, Jan Kara,
reiserfs-devel, linux-kernel, linux-f2fs-devel, linux-xfs,
linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4,
linux-btrfs
On Tue, Jun 25, 2019 at 07:12:54PM +0200, David Sterba wrote:
> On Fri, Jun 21, 2019 at 04:56:21PM -0700, 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.
>
> I checked a few samples what's the type of the flags, there are unsigned
> types while the proposed VFS functions take signed type.
>
> > +int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
>
> Specifically ext4 uses unsigned type and his was the original API that
> got copied so I'd think that it should unsigned everywhere.
Yeah, I'll change it.
> > fs/btrfs/ioctl.c | 13 +++++--------
>
> For the btrfs bits
>
> Acked-by: David Sterba <dsterba@suse.com>
>
> and besides the signedness, the rest of the changes look good to me.
Thanks for the look around! I'll have a new revision with all changes
out by the end of the day. :)
--D
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR
2019-06-21 23:56 [PATCH v2 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
2019-06-21 23:56 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
@ 2019-06-21 23:56 ` Darrick J. Wong
2019-06-25 10:57 ` Christoph Hellwig
2019-06-21 23:56 ` [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info Darrick J. Wong
2019-06-21 23:56 ` [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints Darrick J. Wong
3 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-06-21 23:56 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, Jan Kara,
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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
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 403ddbb81335..ddfe60679b53 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2207,3 +2207,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 4d6feee18778..0c3ef24afe22 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3557,4 +3557,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] 15+ messages in thread
* Re: [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR
2019-06-21 23:56 ` [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR Darrick J. Wong
@ 2019-06-25 10:57 ` Christoph Hellwig
2019-06-25 17:02 ` David Sterba
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2019-06-25 10:57 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, Jan Kara,
reiserfs-devel, linux-kernel, linux-f2fs-devel, linux-xfs,
linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4,
linux-btrfs
On Fri, Jun 21, 2019 at 04:56:29PM -0700, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
> 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);
Is there really much of a point in this helper? Epeciall as
the zeroing could easily be done in the variable declaration
line using
struct fsxattr fa = { };
> + memset(fa, 0, sizeof(struct fsxattr));
> + fa->fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & EXT4_FL_USER_VISIBLE);
Overly lone line.
> + if (ext4_has_feature_project(inode->i_sb)) {
> + fa->fsx_projid = (__u32)from_kprojid(&init_user_ns,
> + ei->i_projid);
The cast here looks bogus. Same comment for f2fs.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR
2019-06-25 10:57 ` Christoph Hellwig
@ 2019-06-25 17:02 ` David Sterba
2019-06-25 17:16 ` Darrick J. Wong
0 siblings, 1 reply; 15+ messages in thread
From: David Sterba @ 2019-06-25 17:02 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Darrick J. Wong, matthew.garrett, yuchao0, tytso, shaggy,
ard.biesheuvel, josef, clm, adilger.kernel, jk, jack, dsterba,
jaegeuk, viro, cluster-devel, jfs-discussion, linux-efi,
Jan Kara, reiserfs-devel, linux-kernel, linux-f2fs-devel,
linux-xfs, linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel,
linux-ext4, linux-btrfs
On Tue, Jun 25, 2019 at 03:57:25AM -0700, Christoph Hellwig wrote:
> On Fri, Jun 21, 2019 at 04:56:29PM -0700, 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>
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > ---
> > 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);
>
> Is there really much of a point in this helper? Epeciall as
> the zeroing could easily be done in the variable declaration
> line using
>
> struct fsxattr fa = { };
Agreed, not counting the initialization the wrapper is merely another
name for btrfs_inode_flags_to_xflags. I also find it slightly confusing
that __btrfs_ioctl_fsgetxattr name is too close to the ioctl callback
implementation btrfs_ioctl_fsgetxattr but only does some initialization.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR
2019-06-25 17:02 ` David Sterba
@ 2019-06-25 17:16 ` Darrick J. Wong
2019-06-25 17:33 ` David Sterba
0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-06-25 17:16 UTC (permalink / raw)
To: dsterba, Christoph Hellwig, matthew.garrett, yuchao0, tytso,
shaggy, ard.biesheuvel, josef, clm, adilger.kernel, jk, jack,
dsterba, jaegeuk, viro, cluster-devel, jfs-discussion, linux-efi,
Jan Kara, reiserfs-devel, linux-kernel, linux-f2fs-devel,
linux-xfs, linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel,
linux-ext4, linux-btrfs
On Tue, Jun 25, 2019 at 07:02:48PM +0200, David Sterba wrote:
> On Tue, Jun 25, 2019 at 03:57:25AM -0700, Christoph Hellwig wrote:
> > On Fri, Jun 21, 2019 at 04:56:29PM -0700, 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>
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > ---
> > > 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);
> >
> > Is there really much of a point in this helper? Epeciall as
> > the zeroing could easily be done in the variable declaration
> > line using
> >
> > struct fsxattr fa = { };
>
> Agreed, not counting the initialization the wrapper is merely another
> name for btrfs_inode_flags_to_xflags. I also find it slightly confusing
> that __btrfs_ioctl_fsgetxattr name is too close to the ioctl callback
> implementation btrfs_ioctl_fsgetxattr but only does some initialization.
Ok; it's easily enough changed to:
struct fsxattr old_fa = {
.fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags),
};
--D
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR
2019-06-25 17:16 ` Darrick J. Wong
@ 2019-06-25 17:33 ` David Sterba
0 siblings, 0 replies; 15+ messages in thread
From: David Sterba @ 2019-06-25 17:33 UTC (permalink / raw)
To: Darrick J. Wong
Cc: dsterba, Christoph Hellwig, matthew.garrett, yuchao0, tytso,
shaggy, ard.biesheuvel, josef, clm, adilger.kernel, jk, jack,
dsterba, jaegeuk, viro, cluster-devel, jfs-discussion, linux-efi,
Jan Kara, reiserfs-devel, linux-kernel, linux-f2fs-devel,
linux-xfs, linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel,
linux-ext4, linux-btrfs
On Tue, Jun 25, 2019 at 10:16:16AM -0700, Darrick J. Wong wrote:
> On Tue, Jun 25, 2019 at 07:02:48PM +0200, David Sterba wrote:
> > On Tue, Jun 25, 2019 at 03:57:25AM -0700, Christoph Hellwig wrote:
> > > On Fri, Jun 21, 2019 at 04:56:29PM -0700, 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>
> > > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > > 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);
> > >
> > > Is there really much of a point in this helper? Epeciall as
> > > the zeroing could easily be done in the variable declaration
> > > line using
> > >
> > > struct fsxattr fa = { };
> >
> > Agreed, not counting the initialization the wrapper is merely another
> > name for btrfs_inode_flags_to_xflags. I also find it slightly confusing
> > that __btrfs_ioctl_fsgetxattr name is too close to the ioctl callback
> > implementation btrfs_ioctl_fsgetxattr but only does some initialization.
>
> Ok; it's easily enough changed to:
>
> struct fsxattr old_fa = {
> .fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags),
> };
Works for me, thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info
2019-06-21 23:56 [PATCH v2 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
2019-06-21 23:56 ` [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS Darrick J. Wong
2019-06-21 23:56 ` [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR Darrick J. Wong
@ 2019-06-21 23:56 ` Darrick J. Wong
2019-06-25 10:57 ` Christoph Hellwig
2019-06-21 23:56 ` [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints Darrick J. Wong
3 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-06-21 23:56 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, Jan Kara,
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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
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 ddfe60679b53..f1ffa9605078 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2221,6 +2221,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] 15+ messages in thread
* Re: [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info
2019-06-21 23:56 ` [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info Darrick J. Wong
@ 2019-06-25 10:57 ` Christoph Hellwig
0 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2019-06-25 10:57 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, Jan Kara,
reiserfs-devel, linux-kernel, linux-f2fs-devel, linux-xfs,
linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4,
linux-btrfs
On Fri, Jun 21, 2019 at 04:56:37PM -0700, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints
2019-06-21 23:56 [PATCH v2 0/4] vfs: clean up SETFLAGS and FSSETXATTR option processing Darrick J. Wong
` (2 preceding siblings ...)
2019-06-21 23:56 ` [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info Darrick J. Wong
@ 2019-06-21 23:56 ` Darrick J. Wong
2019-06-25 10:58 ` Christoph Hellwig
3 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-06-21 23:56 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, Jan Kara,
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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
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 f1ffa9605078..6374ad2ef25b 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2234,6 +2234,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] 15+ messages in thread
* Re: [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints
2019-06-21 23:56 ` [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints Darrick J. Wong
@ 2019-06-25 10:58 ` Christoph Hellwig
0 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2019-06-25 10:58 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, Jan Kara,
reiserfs-devel, linux-kernel, linux-f2fs-devel, linux-xfs,
linux-nilfs, linux-mtd, ocfs2-devel, linux-fsdevel, linux-ext4,
linux-btrfs
On Fri, Jun 21, 2019 at 04:56:45PM -0700, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ 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
0 siblings, 0 replies; 15+ 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] 15+ messages in thread