Messages in this thread | | | Date | Thu, 5 Nov 2020 16:05:50 -0800 | From | Eric Biggers <> | Subject | Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix compat F2FS_IOC_{MOVE, GARBAGE_COLLECT}_RANGE |
| |
This patch is marked 2/2, but it seems you sent it out on its own. Patch series are supposed to be resend in full; otherwise people can see just one patch and have no context.
On Thu, Nov 05, 2020 at 09:09:34AM +0800, Chao Yu wrote: > Eric reported a ioctl bug in below link: > > https://lore.kernel.org/linux-f2fs-devel/20201103032234.GB2875@sol.localdomain/ > > That said, on some 32-bit architectures, u64 has only 32-bit alignment, > notably i386 and x86_32, so that size of struct f2fs_gc_range compiled > in x86_32 is 20 bytes, however the size in x86_64 is 24 bytes, binary > compiled in x86_32 can not call F2FS_IOC_GARBAGE_COLLECT_RANGE successfully > due to mismatched value of ioctl command in between binary and f2fs > module, similarly, F2FS_IOC_MOVE_RANGE will fail too. > > In this patch we introduce two ioctls for compatibility of above special > 32-bit binary: > - F2FS_IOC32_GARBAGE_COLLECT_RANGE > - F2FS_IOC32_MOVE_RANGE >
It would be good to add a proper reported-by line, otherwise it's not clear who "Eric" is (there are lots of Erics):
Reported-by: Eric Biggers <ebiggers@google.com>
> +static int __f2fs_ioc_gc_range(struct file *filp, struct f2fs_gc_range *range) > { > - struct inode *inode = file_inode(filp); > - struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > - struct f2fs_gc_range range; > + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(filp)); > u64 end; > int ret; > > + if (unlikely(f2fs_cp_error(sbi))) > + return -EIO; > + if (!f2fs_is_checkpoint_ready(sbi)) > + return -ENOSPC;
These two checkpoint-related checks weren't present in the original version. Is that intentional?
> +static int __f2fs_ioc_move_range(struct file *filp, > + struct f2fs_move_range *range) > { > - struct f2fs_move_range range; > + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(filp)); > struct fd dst; > int err; > > + if (unlikely(f2fs_cp_error(sbi))) > + return -EIO; > + if (!f2fs_is_checkpoint_ready(sbi)) > + return -ENOSPC; > +
Likewise here.
> diff --git a/include/uapi/linux/f2fs.h b/include/uapi/linux/f2fs.h > index f00199a2e38b..8c14e88a9645 100644 > --- a/include/uapi/linux/f2fs.h > +++ b/include/uapi/linux/f2fs.h > @@ -5,6 +5,10 @@ > #include <linux/types.h> > #include <linux/ioctl.h> > > +#ifdef __KERNEL__ > +#include <linux/compat.h> > +#endif > + > /* > * f2fs-specific ioctl commands > */ > @@ -65,6 +69,16 @@ struct f2fs_gc_range { > __u64 len; > }; > > +#if defined(__KERNEL__) && defined(CONFIG_COMPAT) > +struct compat_f2fs_gc_range { > + u32 sync; > + compat_u64 start; > + compat_u64 len; > +}; > +#define F2FS_IOC32_GARBAGE_COLLECT_RANGE _IOW(F2FS_IOCTL_MAGIC, 11,\ > + struct compat_f2fs_gc_range) > +#endif > + > struct f2fs_defragment { > __u64 start; > __u64 len; > @@ -77,6 +91,17 @@ struct f2fs_move_range { > __u64 len; /* size to move */ > }; > > +#if defined(__KERNEL__) && defined(CONFIG_COMPAT) > +struct compat_f2fs_move_range { > + u32 dst_fd; > + compat_u64 pos_in; > + compat_u64 pos_out; > + compat_u64 len; > +}; > +#define F2FS_IOC32_MOVE_RANGE _IOWR(F2FS_IOCTL_MAGIC, 9, \ > + struct compat_f2fs_move_range) > +#endif > + > struct f2fs_flush_device { > __u32 dev_num; /* device number to flush */ > __u32 segments; /* # of segments to flush */ > --
Did you consider instead putting these compat definitions in an internal kernel header, or even just in the .c file, to avoid cluttering up the UAPI header?
- Eric
| |