| Message ID | 1559795545-17290-1-git-send-email-wshilong1991@gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/2] ext4: only set project inherit bit for directory | expand |
On Thu, Jun 06, 2019 at 01:32:24PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > It doesn't make any sense to have project inherit bits > for regular files, even though this won't cause any > problem, but it is better fix this. > > Cc: Andreas Dilger <adilger@dilger.ca> > Signed-off-by: Wang Shilong <wshilong@ddn.com> It's good to be maintaining consistent behavior with XFS. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> (applies to both ext4 & f2fs patches) --D > --- > fs/ext4/ext4.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 1cb67859e051..ceb74093e138 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -421,7 +421,8 @@ struct flex_groups { > EXT4_PROJINHERIT_FL | EXT4_CASEFOLD_FL) > > /* Flags that are appropriate for regular files (all but dir-specific ones). */ > -#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL)) > +#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL |\ > + EXT4_PROJINHERIT_FL)) > > /* Flags that are appropriate for non-directories/regular files. */ > #define EXT4_OTHER_FLMASK (EXT4_NODUMP_FL | EXT4_NOATIME_FL) > -- > 2.21.0 >
On Jun 5, 2019, at 10:32 PM, Wang Shilong <wangshilong1991@gmail.com> wrote: > > From: Wang Shilong <wshilong@ddn.com> > > It doesn't make any sense to have project inherit bits > for regular files, even though this won't cause any > problem, but it is better fix this. > > Cc: Andreas Dilger <adilger@dilger.ca> > Signed-off-by: Wang Shilong <wshilong@ddn.com> Reviewed-by: Andreas Dilger <adilger@dilger.ca> > --- > fs/ext4/ext4.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 1cb67859e051..ceb74093e138 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -421,7 +421,8 @@ struct flex_groups { > EXT4_PROJINHERIT_FL | EXT4_CASEFOLD_FL) > > /* Flags that are appropriate for regular files (all but dir-specific ones). */ > -#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL)) > +#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL |\ > + EXT4_PROJINHERIT_FL)) > > /* Flags that are appropriate for non-directories/regular files. */ > #define EXT4_OTHER_FLMASK (EXT4_NODUMP_FL | EXT4_NOATIME_FL) > -- > 2.21.0 > Cheers, Andreas
On Thu, Jun 06, 2019 at 01:32:24PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > It doesn't make any sense to have project inherit bits > for regular files, even though this won't cause any > problem, but it is better fix this. > > Cc: Andreas Dilger <adilger@dilger.ca> > Signed-off-by: Wang Shilong <wshilong@ddn.com> > --- > fs/ext4/ext4.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 1cb67859e051..ceb74093e138 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -421,7 +421,8 @@ struct flex_groups { > EXT4_PROJINHERIT_FL | EXT4_CASEFOLD_FL) > > /* Flags that are appropriate for regular files (all but dir-specific ones). */ > -#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL)) > +#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL |\ > + EXT4_PROJINHERIT_FL)) > > /* Flags that are appropriate for non-directories/regular files. */ > #define EXT4_OTHER_FLMASK (EXT4_NODUMP_FL | EXT4_NOATIME_FL) > -- > 2.21.0 Won't this break 'chattr' on files that already have this flag set? FS_IOC_GETFLAGS will return this flag, so 'chattr' will pass it back to FS_IOC_SETFLAGS which will return EOPNOTSUPP due to this: if (ext4_mask_flags(inode->i_mode, flags) != flags) return -EOPNOTSUPP; - Eric
Hi, > -- > 2.21.0 Won't this break 'chattr' on files that already have this flag set? FS_IOC_GETFLAGS will return this flag, so 'chattr' will pass it back to FS_IOC_SETFLAGS which will return EOPNOTSUPP due to this: if (ext4_mask_flags(inode->i_mode, flags) != flags) return -EOPNOTSUPP; >>>> You are right for this and we also need take care of this in EXT4_IOC_FSSETXATTR/ this is a bit strange behavior as chattr read existed flags but could not set them again, there are several possible ways that I could think of to fix the issue? 1) change chattr to filter Project inherit bit before call FS_IOC_SETFLAGS 2) we automatically fixed the flag before mask check, something like: if reg: flags &= ~PROJECT_INHERT; if (ext4_mask_flags(inode->i_mode, flags) != flags) return -EOPNOTSUPP; But this might be not good.. I would prefer solution 1) What do you think?
On Fri, Jun 07, 2019 at 07:51:18AM +0000, Wang Shilong wrote: > Hi, > > > -- > > 2.21.0 > > Won't this break 'chattr' on files that already have this flag set? > FS_IOC_GETFLAGS will return this flag, so 'chattr' will pass it back to > FS_IOC_SETFLAGS which will return EOPNOTSUPP due to this: > > if (ext4_mask_flags(inode->i_mode, flags) != flags) > return -EOPNOTSUPP; > > >>>> > > You are right for this and we also need take care of this in EXT4_IOC_FSSETXATTR/ > this is a bit strange behavior as chattr read existed flags > but could not set them again, there are several possible ways that I could think > of to fix the issue? > > 1) change chattr to filter Project inherit bit before call FS_IOC_SETFLAGS > > 2) we automatically fixed the flag before mask check, something like: > if reg: > flags &= ~PROJECT_INHERT; > if (ext4_mask_flags(inode->i_mode, flags) != flags) > return -EOPNOTSUPP; > But this might be not good.. > > I would prefer solution 1) > What do you think? Existing versions of chattr can't be changed, and people don't necessarily upgrade the kernel and e2fsprogs at the same time. So (1) wouldn't really work. A better solution might be to make FS_IOC_GETFLAGS and FS_IOC_FSGETXATTR never return the project inherit flag on regular files. - Eric
Hi, > You are right for this and we also need take care of this in EXT4_IOC_FSSETXATTR/ > this is a bit strange behavior as chattr read existed flags > but could not set them again, there are several possible ways that I could think > of to fix the issue? > > 1) change chattr to filter Project inherit bit before call FS_IOC_SETFLAGS > > 2) we automatically fixed the flag before mask check, something like: > if reg: > flags &= ~PROJECT_INHERT; > if (ext4_mask_flags(inode->i_mode, flags) != flags) > return -EOPNOTSUPP; > But this might be not good.. > > I would prefer solution 1) > What do you think? Existing versions of chattr can't be changed, and people don't necessarily upgrade the kernel and e2fsprogs at the same time. So (1) wouldn't really work. A better solution might be to make FS_IOC_GETFLAGS and FS_IOC_FSGETXATTR never return the project inherit flag on regular files. - Eric >>>>>> How about fix it in __ext4_iget(): ei->i_flags = le32_to_cpu(raw_inode->i_flags); if (S_ISREG(inode->i_mode)) ei->i_flags &= ~EXT4_PROJINHERIT_FL; This way will give a big chance flag will be automatically fixed next time whenever inode is dirtied. Thanks, Shilong
On Thu, Jun 06, 2019 at 01:32:24PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > It doesn't make any sense to have project inherit bits > for regular files, even though this won't cause any > problem, but it is better fix this. > > Cc: Andreas Dilger <adilger@dilger.ca> > Signed-off-by: Wang Shilong <wshilong@ddn.com> Thanks, applied to the ext4 tree. - Ted
On Fri, Jun 07, 2019 at 11:14:52AM -0700, Eric Biggers wrote: > > Existing versions of chattr can't be changed, and people don't necessarily > upgrade the kernel and e2fsprogs at the same time. So (1) wouldn't really work. > > A better solution might be to make FS_IOC_GETFLAGS and FS_IOC_FSGETXATTR never > return the project inherit flag on regular files. I've amended this patch by adding the following to fix it for FS_IOC_GETFLAGS (which is chattr uses): diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index 7af835ac8d23..74648d42c69b 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -779,6 +779,8 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) return ext4_ioc_getfsmap(sb, (void __user *)arg); case EXT4_IOC_GETFLAGS: flags = ei->i_flags & EXT4_FL_USER_VISIBLE; + if (S_ISREG(inode->i_mode)) + flags &= ~EXT4_PROJINHERIT_FL; return put_user(flags, (int __user *) arg); case EXT4_IOC_SETFLAGS: { int err; - Ted
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 1cb67859e051..ceb74093e138 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -421,7 +421,8 @@ struct flex_groups { EXT4_PROJINHERIT_FL | EXT4_CASEFOLD_FL) /* Flags that are appropriate for regular files (all but dir-specific ones). */ -#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL)) +#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL | EXT4_CASEFOLD_FL |\ + EXT4_PROJINHERIT_FL)) /* Flags that are appropriate for non-directories/regular files. */ #define EXT4_OTHER_FLMASK (EXT4_NODUMP_FL | EXT4_NOATIME_FL)