| Message ID | 20190701110603.5abcbb2c@canb.auug.org.au (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | linux-next: manual merge of the xfs tree with the f2fs tree | expand |
Hi Stephen, Could you add my iomap-for-next and vfs-for-next branches to linux-next, please? They can be found here: git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#iomap-for-next git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#vfs-for-next I've decided that trying to munge all that through the xfs for-next branch is too much insanity and splitting them up will help me prevent my head from falling off. --Darrick
Hi Darrick, On Mon, 1 Jul 2019 08:35:52 -0700 "Darrick J. Wong" <darrick.wong@oracle.com> wrote: > > Could you add my iomap-for-next and vfs-for-next branches to linux-next, > please? They can be found here: > > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#iomap-for-next > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#vfs-for-next > > I've decided that trying to munge all that through the xfs for-next > branch is too much insanity and splitting them up will help me prevent > my head from falling off. Just out of interest, do you intend to send these directly to Linus, or via another tree?
Hi Darrick, On Mon, 1 Jul 2019 08:35:52 -0700 "Darrick J. Wong" <darrick.wong@oracle.com> wrote: > > Could you add my iomap-for-next and vfs-for-next branches to linux-next, > please? They can be found here: > > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#iomap-for-next > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#vfs-for-next > > I've decided that trying to munge all that through the xfs for-next > branch is too much insanity and splitting them up will help me prevent > my head from falling off. Added from today. Thanks for adding your subsystem tree as a participant of linux-next. As you may know, this is not a judgement of your code. The purpose of linux-next is for integration testing and to lower the impact of conflicts between subsystems in the next merge window. You will need to ensure that the patches/commits in your tree/series have been: * submitted under GPL v2 (or later) and include the Contributor's Signed-off-by, * posted to the relevant mailing list, * reviewed by you (or another maintainer of your subsystem tree), * successfully unit tested, and * destined for the current or next Linux merge window. Basically, this should be just what you would send to Linus (or ask him to fetch). It is allowed to be rebased if you deem it necessary.
On Tue, Jul 02, 2019 at 01:42:10AM +1000, Stephen Rothwell wrote: > Hi Darrick, > > On Mon, 1 Jul 2019 08:35:52 -0700 "Darrick J. Wong" <darrick.wong@oracle.com> wrote: > > > > Could you add my iomap-for-next and vfs-for-next branches to linux-next, > > please? They can be found here: > > > > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#iomap-for-next > > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git#vfs-for-next > > > > I've decided that trying to munge all that through the xfs for-next > > branch is too much insanity and splitting them up will help me prevent > > my head from falling off. > > Just out of interest, do you intend to send these directly to Linus, or > via another tree? The iomap stuff I'd definitely send directly to Linus. For vfs stuff, it depends. For work in the core vfs I'll probably just keep sending patches through Al, but for treewide things like ioctl cleanups I suspect it'll be easier to let them soak in -next and then send them directly. I also dropped the f2fs parts of the setflags/fssetxattr patches because while I think your fix from yesterday's for-next is correct, I prefer to let Eric Biggers' cleanup land through the f2fs tree before I try to clean up f2fs again. There shouldn't be any harm in letting that lag a little while longer. --D > -- > Cheers, > Stephen Rothwell
[+f2fs mailing list] On Mon, Jul 01, 2019 at 11:06:03AM +1000, Stephen Rothwell wrote: > Hi all, > > Today's linux-next merge of the xfs tree got a conflict in: > > fs/f2fs/file.c > > between commit: > > 360985573b55 ("f2fs: separate f2fs i_flags from fs_flags and ext4 i_flags") > > from the f2fs tree and commits: > > de2baa49bbae ("vfs: create a generic checking and prep function for FS_IOC_SETFLAGS") > 3dd3ba36a8ee ("vfs: create a generic checking function for FS_IOC_FSSETXATTR") > > from the xfs tree. > > I fixed it up (I think - see below) and can carry the fix as necessary. > This is now fixed as far as linux-next is concerned, but any non trivial > conflicts should be mentioned to your upstream maintainer when your tree > is submitted for merging. You may also want to consider cooperating > with the maintainer of the conflicting tree to minimise any particularly > complex conflicts. > > -- > Cheers, > Stephen Rothwell > > diff --cc fs/f2fs/file.c > index e7c368db8185,8799468724f9..000000000000 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@@ -1645,22 -1648,45 +1645,23 @@@ static int f2fs_file_flush(struct file > return 0; > } > > -static int f2fs_ioc_getflags(struct file *filp, unsigned long arg) > -{ > - struct inode *inode = file_inode(filp); > - struct f2fs_inode_info *fi = F2FS_I(inode); > - unsigned int flags = fi->i_flags; > - > - if (IS_ENCRYPTED(inode)) > - flags |= F2FS_ENCRYPT_FL; > - if (f2fs_has_inline_data(inode) || f2fs_has_inline_dentry(inode)) > - flags |= F2FS_INLINE_DATA_FL; > - if (is_inode_flag_set(inode, FI_PIN_FILE)) > - flags |= F2FS_NOCOW_FL; > - > - flags &= F2FS_FL_USER_VISIBLE; > - > - return put_user(flags, (int __user *)arg); > -} > - > -static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags) > +static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) > { > struct f2fs_inode_info *fi = F2FS_I(inode); > - unsigned int oldflags; > + u32 oldflags; > + int err; > > /* Is it quota file? Do not allow user to mess with it */ > if (IS_NOQUOTA(inode)) > return -EPERM; > > - flags = f2fs_mask_flags(inode->i_mode, flags); > - > oldflags = fi->i_flags; > > - if ((iflags ^ oldflags) & (F2FS_APPEND_FL | F2FS_IMMUTABLE_FL)) > - if (!capable(CAP_LINUX_IMMUTABLE)) > - return -EPERM; > - err = vfs_ioc_setflags_prepare(inode, oldflags, flags); > ++ err = vfs_ioc_setflags_prepare(inode, oldflags, iflags); > + if (err) > + return err; I don't think this is the correct resolution. Now f2fs_setflags_common() is meant to take the f2fs on-disk i_flags, which aren't necessarily the same as the flags passed to the FS_IOC_SETFLAGS ioctl. So it's not appropriate to call vfs_ioc_setflags_prepare() in it. It should be in f2fs_ioc_setflags() instead. I've pushed up what I think is the correct resolution to https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/log/?h=f2fs-setflags-resolved Here is my diff of fs/f2fs/file.c from f2fs/dev. Darrick, can you check that this is what you would have done if you had patched f2fs/dev instead of v5.2? diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index e7c368db81851f..64f157f2e8d5e4 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -1648,19 +1648,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) { struct f2fs_inode_info *fi = F2FS_I(inode); - u32 oldflags; /* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA(inode)) return -EPERM; - oldflags = fi->i_flags; - - if ((iflags ^ oldflags) & (F2FS_APPEND_FL | F2FS_IMMUTABLE_FL)) - if (!capable(CAP_LINUX_IMMUTABLE)) - return -EPERM; - - fi->i_flags = iflags | (oldflags & ~mask); + fi->i_flags = iflags | (fi->i_flags & ~mask); if (fi->i_flags & F2FS_PROJINHERIT_FL) set_inode_flag(inode, FI_PROJ_INHERIT); @@ -1765,7 +1758,8 @@ static int f2fs_ioc_getflags(struct file *filp, unsigned long arg) static int f2fs_ioc_setflags(struct file *filp, unsigned long arg) { struct inode *inode = file_inode(filp); - u32 fsflags; + struct f2fs_inode_info *fi = F2FS_I(inode); + u32 fsflags, old_fsflags; u32 iflags; int ret; @@ -1789,8 +1783,14 @@ static int f2fs_ioc_setflags(struct file *filp, unsigned long arg) inode_lock(inode); + old_fsflags = f2fs_iflags_to_fsflags(fi->i_flags); + ret = vfs_ioc_setflags_prepare(inode, old_fsflags, fsflags); + if (ret) + goto out; + ret = f2fs_setflags_common(inode, iflags, f2fs_fsflags_to_iflags(F2FS_SETTABLE_FS_FL)); +out: inode_unlock(inode); mnt_drop_write_file(filp); return ret; @@ -2850,52 +2850,32 @@ static inline u32 f2fs_xflags_to_iflags(u32 xflags) return iflags; } -static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg) +static void f2fs_fill_fsxattr(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); + simple_fill_fsxattr(fa, f2fs_iflags_to_xflags(fi->i_flags)); if (f2fs_sb_has_project_quota(F2FS_I_SB(inode))) - fa.fsx_projid = (__u32)from_kprojid(&init_user_ns, - fi->i_projid); - - if (copy_to_user((struct fsxattr __user *)arg, &fa, sizeof(fa))) - return -EFAULT; - return 0; + fa->fsx_projid = from_kprojid(&init_user_ns, fi->i_projid); } -static int f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa) +static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg) { - /* - * 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; + struct inode *inode = file_inode(filp); + struct fsxattr fa; - 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; - } + f2fs_fill_fsxattr(inode, &fa); + if (copy_to_user((struct fsxattr __user *)arg, &fa, sizeof(fa))) + return -EFAULT; return 0; } static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg) { struct inode *inode = file_inode(filp); - struct fsxattr fa; + struct fsxattr fa, old_fa; u32 iflags; int err; @@ -2918,7 +2898,9 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg) return err; inode_lock(inode); - err = f2fs_ioctl_check_project(inode, &fa); + + f2fs_fill_fsxattr(inode, &old_fa); + err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa); if (err) goto out; err = f2fs_setflags_common(inode, iflags,
On Mon, Jul 01, 2019 at 09:31:46AM -0700, Eric Biggers wrote: > [+f2fs mailing list] > > On Mon, Jul 01, 2019 at 11:06:03AM +1000, Stephen Rothwell wrote: > > Hi all, > > > > Today's linux-next merge of the xfs tree got a conflict in: > > > > fs/f2fs/file.c > > > > between commit: > > > > 360985573b55 ("f2fs: separate f2fs i_flags from fs_flags and ext4 i_flags") > > > > from the f2fs tree and commits: > > > > de2baa49bbae ("vfs: create a generic checking and prep function for FS_IOC_SETFLAGS") > > 3dd3ba36a8ee ("vfs: create a generic checking function for FS_IOC_FSSETXATTR") > > > > from the xfs tree. > > > > I fixed it up (I think - see below) and can carry the fix as necessary. > > This is now fixed as far as linux-next is concerned, but any non trivial > > conflicts should be mentioned to your upstream maintainer when your tree > > is submitted for merging. You may also want to consider cooperating > > with the maintainer of the conflicting tree to minimise any particularly > > complex conflicts. > > > > -- > > Cheers, > > Stephen Rothwell > > > > diff --cc fs/f2fs/file.c > > index e7c368db8185,8799468724f9..000000000000 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@@ -1645,22 -1648,45 +1645,23 @@@ static int f2fs_file_flush(struct file > > return 0; > > } > > > > -static int f2fs_ioc_getflags(struct file *filp, unsigned long arg) > > -{ > > - struct inode *inode = file_inode(filp); > > - struct f2fs_inode_info *fi = F2FS_I(inode); > > - unsigned int flags = fi->i_flags; > > - > > - if (IS_ENCRYPTED(inode)) > > - flags |= F2FS_ENCRYPT_FL; > > - if (f2fs_has_inline_data(inode) || f2fs_has_inline_dentry(inode)) > > - flags |= F2FS_INLINE_DATA_FL; > > - if (is_inode_flag_set(inode, FI_PIN_FILE)) > > - flags |= F2FS_NOCOW_FL; > > - > > - flags &= F2FS_FL_USER_VISIBLE; > > - > > - return put_user(flags, (int __user *)arg); > > -} > > - > > -static int __f2fs_ioc_setflags(struct inode *inode, unsigned int flags) > > +static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) > > { > > struct f2fs_inode_info *fi = F2FS_I(inode); > > - unsigned int oldflags; > > + u32 oldflags; > > + int err; > > > > /* Is it quota file? Do not allow user to mess with it */ > > if (IS_NOQUOTA(inode)) > > return -EPERM; > > > > - flags = f2fs_mask_flags(inode->i_mode, flags); > > - > > oldflags = fi->i_flags; > > > > - if ((iflags ^ oldflags) & (F2FS_APPEND_FL | F2FS_IMMUTABLE_FL)) > > - if (!capable(CAP_LINUX_IMMUTABLE)) > > - return -EPERM; > > - err = vfs_ioc_setflags_prepare(inode, oldflags, flags); > > ++ err = vfs_ioc_setflags_prepare(inode, oldflags, iflags); > > + if (err) > > + return err; > > I don't think this is the correct resolution. Now f2fs_setflags_common() is > meant to take the f2fs on-disk i_flags, which aren't necessarily the same as the > flags passed to the FS_IOC_SETFLAGS ioctl. So it's not appropriate to call > vfs_ioc_setflags_prepare() in it. It should be in f2fs_ioc_setflags() instead. > > I've pushed up what I think is the correct resolution to > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/log/?h=f2fs-setflags-resolved > > Here is my diff of fs/f2fs/file.c from f2fs/dev. Darrick, can you check that > this is what you would have done if you had patched f2fs/dev instead of v5.2? Yep, that's roughly what I was planning to do to connect f2fs to the checking functions. I withdrew the fs/f2fs/ parts of the setflags/fssetxattr cleanups from the branch with the intent to fix it after f2fs-dev lands, because I think this is too messy a merge resolution to push through Linus. Would you mind resending this with a proper subject line and SOB? :) --D > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index e7c368db81851f..64f157f2e8d5e4 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -1648,19 +1648,12 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) > { > struct f2fs_inode_info *fi = F2FS_I(inode); > - u32 oldflags; > > /* Is it quota file? Do not allow user to mess with it */ > if (IS_NOQUOTA(inode)) > return -EPERM; > > - oldflags = fi->i_flags; > - > - if ((iflags ^ oldflags) & (F2FS_APPEND_FL | F2FS_IMMUTABLE_FL)) > - if (!capable(CAP_LINUX_IMMUTABLE)) > - return -EPERM; > - > - fi->i_flags = iflags | (oldflags & ~mask); > + fi->i_flags = iflags | (fi->i_flags & ~mask); > > if (fi->i_flags & F2FS_PROJINHERIT_FL) > set_inode_flag(inode, FI_PROJ_INHERIT); > @@ -1765,7 +1758,8 @@ static int f2fs_ioc_getflags(struct file *filp, unsigned long arg) > static int f2fs_ioc_setflags(struct file *filp, unsigned long arg) > { > struct inode *inode = file_inode(filp); > - u32 fsflags; > + struct f2fs_inode_info *fi = F2FS_I(inode); > + u32 fsflags, old_fsflags; > u32 iflags; > int ret; > > @@ -1789,8 +1783,14 @@ static int f2fs_ioc_setflags(struct file *filp, unsigned long arg) > > inode_lock(inode); > > + old_fsflags = f2fs_iflags_to_fsflags(fi->i_flags); > + ret = vfs_ioc_setflags_prepare(inode, old_fsflags, fsflags); > + if (ret) > + goto out; > + > ret = f2fs_setflags_common(inode, iflags, > f2fs_fsflags_to_iflags(F2FS_SETTABLE_FS_FL)); > +out: > inode_unlock(inode); > mnt_drop_write_file(filp); > return ret; > @@ -2850,52 +2850,32 @@ static inline u32 f2fs_xflags_to_iflags(u32 xflags) > return iflags; > } > > -static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg) > +static void f2fs_fill_fsxattr(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); > + simple_fill_fsxattr(fa, f2fs_iflags_to_xflags(fi->i_flags)); > > if (f2fs_sb_has_project_quota(F2FS_I_SB(inode))) > - fa.fsx_projid = (__u32)from_kprojid(&init_user_ns, > - fi->i_projid); > - > - if (copy_to_user((struct fsxattr __user *)arg, &fa, sizeof(fa))) > - return -EFAULT; > - return 0; > + fa->fsx_projid = from_kprojid(&init_user_ns, fi->i_projid); > } > > -static int f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa) > +static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg) > { > - /* > - * 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; > + struct inode *inode = file_inode(filp); > + struct fsxattr fa; > > - 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; > - } > + f2fs_fill_fsxattr(inode, &fa); > > + if (copy_to_user((struct fsxattr __user *)arg, &fa, sizeof(fa))) > + return -EFAULT; > return 0; > } > > static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg) > { > struct inode *inode = file_inode(filp); > - struct fsxattr fa; > + struct fsxattr fa, old_fa; > u32 iflags; > int err; > > @@ -2918,7 +2898,9 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg) > return err; > > inode_lock(inode); > - err = f2fs_ioctl_check_project(inode, &fa); > + > + f2fs_fill_fsxattr(inode, &old_fa); > + err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa); > if (err) > goto out; > err = f2fs_setflags_common(inode, iflags,
diff --cc fs/f2fs/file.c index e7c368db8185,8799468724f9..000000000000 --- a/fs/f2fs/file.c
Hi all, Today's linux-next merge of the xfs tree got a conflict in: fs/f2fs/file.c between commit: 360985573b55 ("f2fs: separate f2fs i_flags from fs_flags and ext4 i_flags") from the f2fs tree and commits: de2baa49bbae ("vfs: create a generic checking and prep function for FS_IOC_SETFLAGS") 3dd3ba36a8ee ("vfs: create a generic checking function for FS_IOC_FSSETXATTR") from the xfs tree. I fixed it up (I think - see below) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts.