| Message ID | 1475671375-22969-2-git-send-email-jack@suse.cz |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed 05-10-16 14:42:54, Jan Kara wrote: > Currently we just silently ignore flags that we don't understand (or > that cannot be manipulated) through EXT4_IOC_SETFLAGS and > EXT4_IOC_FSSETXATTR ioctls. This makes it problematic for the unused > flags to be used in future (some app may be inadvertedly setting them > and we won't notice until the flag gets used). Also this is inconsistent > with other filesystems like XFS or BTRFS which return EOPNOTSUPP when > they see a flag they cannot set. > > ext4 has the additional problem that there are flags which are returned > by EXT4_IOC_GETFLAGS ioctl but which cannot be modified via > EXT4_IOC_SETFLAGS. So we have to be careful to ignore value of these > flags and not fail the ioctl when they are set (as e.g. chattr(1) passes > flags returned from EXT4_IOC_GETFLAGS to EXT4_IOC_SETFLAGS without any > masking and thus we'd break this utility). > > Signed-off-by: Jan Kara <jack@suse.cz> I've found out this does not quite work because EXT4_FL_USER_MODIFIABLE does not contain all user modifiable flags - at least EXT4_JOURNAL_FL and EXT4_EXTENTS_FL have special handling and can be modified even if they are not in EXT4_FL_USER_MODIFIABLE bitmask. So this patch needs more tweaking. Honza > --- > fs/ext4/ext4.h | 1 + > fs/ext4/ioctl.c | 28 +++++++++++++++++++++++----- > 2 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index ea31931386ec..a77a15df15b0 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -395,6 +395,7 @@ struct flex_groups { > #define EXT4_FL_USER_VISIBLE 0x304BDFFF /* User visible flags */ > #define EXT4_FL_USER_MODIFIABLE 0x204380FF /* User modifiable flags */ > > +/* Flags we can manipulate with through EXT4_IOC_FSSETXATTR */ > #define EXT4_FL_XFLAG_VISIBLE (EXT4_SYNC_FL | \ > EXT4_IMMUTABLE_FL | \ > EXT4_APPEND_FL | \ > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index 1bb7df5e4536..540bababfec1 100644 > --- a/fs/ext4/ioctl.c > +++ b/fs/ext4/ioctl.c > @@ -412,6 +412,10 @@ static inline __u32 ext4_iflags_to_xflags(unsigned long iflags) > return xflags; > } > > +#define EXT4_SUPPORTED_FS_XFLAGS (FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | \ > + FS_XFLAG_APPEND | FS_XFLAG_NODUMP | \ > + FS_XFLAG_NOATIME | FS_XFLAG_PROJINHERIT) > + > /* Transfer xflags flags to internal */ > static inline unsigned long ext4_xflags_to_iflags(__u32 xflags) > { > @@ -456,12 +460,22 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > if (get_user(flags, (int __user *) arg)) > return -EFAULT; > > + if (flags & ~EXT4_FL_USER_VISIBLE) > + return -EOPNOTSUPP; > + /* > + * chattr(1) grabs flags via GETFLAGS, modifies the result and > + * passes that to SETFLAGS. So we cannot easily make SETFLAGS > + * more restrictive than just silently masking off visible but > + * not settable flags as we always did. > + */ > + flags &= EXT4_FL_USER_MODIFIABLE; > + if (ext4_mask_flags(inode->i_mode, flags) != flags) > + return -EOPNOTSUPP; > + > err = mnt_want_write_file(filp); > if (err) > return err; > > - flags = ext4_mask_flags(inode->i_mode, flags); > - > inode_lock(inode); > err = ext4_ioctl_setflags(inode, flags); > inode_unlock(inode); > @@ -866,13 +880,17 @@ resizefs_out: > if (!inode_owner_or_capable(inode)) > return -EACCES; > > + if (fa.fsx_xflags & ~EXT4_SUPPORTED_FS_XFLAGS) > + return -EOPNOTSUPP; > + > + flags = ext4_xflags_to_iflags(fa.fsx_xflags); > + if (ext4_mask_flags(inode->i_mode, flags) != flags) > + return -EOPNOTSUPP; > + > err = mnt_want_write_file(filp); > if (err) > return err; > > - flags = ext4_xflags_to_iflags(fa.fsx_xflags); > - flags = ext4_mask_flags(inode->i_mode, flags); > - > inode_lock(inode); > flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) | > (flags & EXT4_FL_XFLAG_VISIBLE); > -- > 2.6.6 >
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index ea31931386ec..a77a15df15b0 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -395,6 +395,7 @@ struct flex_groups { #define EXT4_FL_USER_VISIBLE 0x304BDFFF /* User visible flags */ #define EXT4_FL_USER_MODIFIABLE 0x204380FF /* User modifiable flags */ +/* Flags we can manipulate with through EXT4_IOC_FSSETXATTR */ #define EXT4_FL_XFLAG_VISIBLE (EXT4_SYNC_FL | \ EXT4_IMMUTABLE_FL | \ EXT4_APPEND_FL | \ diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index 1bb7df5e4536..540bababfec1 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -412,6 +412,10 @@ static inline __u32 ext4_iflags_to_xflags(unsigned long iflags) return xflags; } +#define EXT4_SUPPORTED_FS_XFLAGS (FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | \ + FS_XFLAG_APPEND | FS_XFLAG_NODUMP | \ + FS_XFLAG_NOATIME | FS_XFLAG_PROJINHERIT) + /* Transfer xflags flags to internal */ static inline unsigned long ext4_xflags_to_iflags(__u32 xflags) { @@ -456,12 +460,22 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) if (get_user(flags, (int __user *) arg)) return -EFAULT; + if (flags & ~EXT4_FL_USER_VISIBLE) + return -EOPNOTSUPP; + /* + * chattr(1) grabs flags via GETFLAGS, modifies the result and + * passes that to SETFLAGS. So we cannot easily make SETFLAGS + * more restrictive than just silently masking off visible but + * not settable flags as we always did. + */ + flags &= EXT4_FL_USER_MODIFIABLE; + if (ext4_mask_flags(inode->i_mode, flags) != flags) + return -EOPNOTSUPP; + err = mnt_want_write_file(filp); if (err) return err; - flags = ext4_mask_flags(inode->i_mode, flags); - inode_lock(inode); err = ext4_ioctl_setflags(inode, flags); inode_unlock(inode); @@ -866,13 +880,17 @@ resizefs_out: if (!inode_owner_or_capable(inode)) return -EACCES; + if (fa.fsx_xflags & ~EXT4_SUPPORTED_FS_XFLAGS) + return -EOPNOTSUPP; + + flags = ext4_xflags_to_iflags(fa.fsx_xflags); + if (ext4_mask_flags(inode->i_mode, flags) != flags) + return -EOPNOTSUPP; + err = mnt_want_write_file(filp); if (err) return err; - flags = ext4_xflags_to_iflags(fa.fsx_xflags); - flags = ext4_mask_flags(inode->i_mode, flags); - inode_lock(inode); flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) | (flags & EXT4_FL_XFLAG_VISIBLE);
Currently we just silently ignore flags that we don't understand (or that cannot be manipulated) through EXT4_IOC_SETFLAGS and EXT4_IOC_FSSETXATTR ioctls. This makes it problematic for the unused flags to be used in future (some app may be inadvertedly setting them and we won't notice until the flag gets used). Also this is inconsistent with other filesystems like XFS or BTRFS which return EOPNOTSUPP when they see a flag they cannot set. ext4 has the additional problem that there are flags which are returned by EXT4_IOC_GETFLAGS ioctl but which cannot be modified via EXT4_IOC_SETFLAGS. So we have to be careful to ignore value of these flags and not fail the ioctl when they are set (as e.g. chattr(1) passes flags returned from EXT4_IOC_GETFLAGS to EXT4_IOC_SETFLAGS without any masking and thus we'd break this utility). Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/ext4.h | 1 + fs/ext4/ioctl.c | 28 +++++++++++++++++++++++----- 2 files changed, 24 insertions(+), 5 deletions(-)