[PATCH v3 2/2] f2fs: fix compat F2FS_IOC_{MOVE,GARBAGE_COLLECT}_RANGE

[PATCH v3 2/2] f2fs: fix compat F2FS_IOC_{MOVE,GARBAGE_COLLECT}_RANGE

[Chao Yu]

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

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v3:
- clean up codes: move duplicated code into common function
 fs/f2fs/file.c            | 107 +++++++++++++++++++++++++++++---------
 include/uapi/linux/f2fs.h |  25 +++++++++
 2 files changed, 106 insertions(+), 26 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 22ae8ae0072f..e2ac797b70c0 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2480,26 +2480,24 @@ static int f2fs_ioc_gc(struct file *filp, unsigned long arg)
 	return ret;
 }
 
-static int f2fs_ioc_gc_range(struct file *filp, unsigned long arg)
+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;
+
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-
-	if (copy_from_user(&range, (struct f2fs_gc_range __user *)arg,
-							sizeof(range)))
-		return -EFAULT;
-
 	if (f2fs_readonly(sbi->sb))
 		return -EROFS;
 
-	end = range.start + range.len;
-	if (end < range.start || range.start < MAIN_BLKADDR(sbi) ||
+	end = range->start + range->len;
+	if (end < range->start || range->start < MAIN_BLKADDR(sbi) ||
 					end >= MAX_BLKADDR(sbi))
 		return -EINVAL;
 
@@ -2508,7 +2506,7 @@ static int f2fs_ioc_gc_range(struct file *filp, unsigned long arg)
 		return ret;
 
 do_more:
-	if (!range.sync) {
+	if (!range->sync) {
 		if (!down_write_trylock(&sbi->gc_lock)) {
 			ret = -EBUSY;
 			goto out;
@@ -2517,20 +2515,30 @@ static int f2fs_ioc_gc_range(struct file *filp, unsigned long arg)
 		down_write(&sbi->gc_lock);
 	}
 
-	ret = f2fs_gc(sbi, range.sync, true, GET_SEGNO(sbi, range.start));
+	ret = f2fs_gc(sbi, range->sync, true, GET_SEGNO(sbi, range->start));
 	if (ret) {
 		if (ret == -EBUSY)
 			ret = -EAGAIN;
 		goto out;
 	}
-	range.start += BLKS_PER_SEC(sbi);
-	if (range.start <= end)
+	range->start += BLKS_PER_SEC(sbi);
+	if (range->start <= end)
 		goto do_more;
 out:
 	mnt_drop_write_file(filp);
 	return ret;
 }
 
+static int f2fs_ioc_gc_range(struct file *filp, unsigned long arg)
+{
+	struct f2fs_gc_range range;
+
+	if (copy_from_user(&range, (struct f2fs_gc_range __user *)arg,
+							sizeof(range)))
+		return -EFAULT;
+	return __f2fs_ioc_gc_range(filp, &range);
+}
+
 static int f2fs_ioc_write_checkpoint(struct file *filp, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -2867,21 +2875,23 @@ static int f2fs_move_file_range(struct file *file_in, loff_t pos_in,
 	return ret;
 }
 
-static int f2fs_ioc_move_range(struct file *filp, unsigned long arg)
+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;
+
 	if (!(filp->f_mode & FMODE_READ) ||
 			!(filp->f_mode & FMODE_WRITE))
 		return -EBADF;
 
-	if (copy_from_user(&range, (struct f2fs_move_range __user *)arg,
-							sizeof(range)))
-		return -EFAULT;
-
-	dst = fdget(range.dst_fd);
+	dst = fdget(range->dst_fd);
 	if (!dst.file)
 		return -EBADF;
 
@@ -2894,8 +2904,8 @@ static int f2fs_ioc_move_range(struct file *filp, unsigned long arg)
 	if (err)
 		goto err_out;
 
-	err = f2fs_move_file_range(filp, range.pos_in, dst.file,
-					range.pos_out, range.len);
+	err = f2fs_move_file_range(filp, range->pos_in, dst.file,
+					range->pos_out, range->len);
 
 	mnt_drop_write_file(filp);
 err_out:
@@ -2903,6 +2913,16 @@ static int f2fs_ioc_move_range(struct file *filp, unsigned long arg)
 	return err;
 }
 
+static int f2fs_ioc_move_range(struct file *filp, unsigned long arg)
+{
+	struct f2fs_move_range range;
+
+	if (copy_from_user(&range, (struct f2fs_move_range __user *)arg,
+							sizeof(range)))
+		return -EFAULT;
+	return __f2fs_ioc_move_range(filp, &range);
+}
+
 static int f2fs_ioc_flush_device(struct file *filp, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -4230,6 +4250,39 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 }
 
 #ifdef CONFIG_COMPAT
+static int f2fs_compat_ioc_gc_range(struct file *file, unsigned long arg)
+{
+	struct compat_f2fs_gc_range __user *urange;
+	struct f2fs_gc_range range;
+	int err;
+
+	urange = compat_ptr(arg);
+	err = get_user(range.sync, &urange->sync);
+	err |= get_user(range.start, &urange->start);
+	err |= get_user(range.len, &urange->len);
+	if (err)
+		return -EFAULT;
+
+	return __f2fs_ioc_gc_range(file, &range);
+}
+
+static int f2fs_compat_ioc_move_range(struct file *file, unsigned long arg)
+{
+	struct compat_f2fs_move_range __user *urange;
+	struct f2fs_move_range range;
+	int err;
+
+	urange = compat_ptr(arg);
+	err = get_user(range.dst_fd, &urange->dst_fd);
+	err |= get_user(range.pos_in, &urange->pos_in);
+	err |= get_user(range.pos_out, &urange->pos_out);
+	err |= get_user(range.len, &urange->len);
+	if (err)
+		return -EFAULT;
+
+	return __f2fs_ioc_move_range(file, &range);
+}
+
 long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	switch (cmd) {
@@ -4242,6 +4295,10 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case FS_IOC32_GETVERSION:
 		cmd = FS_IOC_GETVERSION;
 		break;
+	case F2FS_IOC32_GARBAGE_COLLECT_RANGE:
+		return f2fs_compat_ioc_gc_range(file, arg);
+	case F2FS_IOC32_MOVE_RANGE:
+		return f2fs_compat_ioc_move_range(file, arg);
 	case F2FS_IOC_START_ATOMIC_WRITE:
 	case F2FS_IOC_COMMIT_ATOMIC_WRITE:
 	case F2FS_IOC_START_VOLATILE_WRITE:
@@ -4259,10 +4316,8 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case FS_IOC_GET_ENCRYPTION_KEY_STATUS:
 	case FS_IOC_GET_ENCRYPTION_NONCE:
 	case F2FS_IOC_GARBAGE_COLLECT:
-	case F2FS_IOC_GARBAGE_COLLECT_RANGE:
 	case F2FS_IOC_WRITE_CHECKPOINT:
 	case F2FS_IOC_DEFRAGMENT:
-	case F2FS_IOC_MOVE_RANGE:
 	case F2FS_IOC_FLUSH_DEVICE:
 	case F2FS_IOC_GET_FEATURES:
 	case FS_IOC_FSGETXATTR:
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 */
-- 
2.26.2

Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix compat F2FS_IOC_{MOVE, GARBAGE_COLLECT}_RANGE

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix compat F2FS_IOC_{MOVE, GARBAGE_COLLECT}_RANGE

[Chao Yu]

On 2020/11/6 8:05, Eric Biggers wrote:
> 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.

That's a historical problem, as in last many years, we (f2fs community) don't have
other long-term reviewers except Jaegeuk and me, so we have unwritten rule: only
resending changed patch in patchset.

IMO, that helps to skip traversing unchanged patches, and focusing reviewing on the
real change lines, and certainly we have its context in mind.

Personally, I prefer to revise, resend or review patch{,es} of patchset have real
change line in f2fs mailing list, anyway we can discuss about the rule here.

> 
> 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>
Sure, although I attached the link includes original report email, in where it
points out who "Eric" is.

> 
>> +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?

Quoted

 > It would be better to have __f2fs_ioc_gc_range() handle the f2fs_cp_error(),
 > f2fs_is_checkpoint_ready(), capable(), and f2fs_readonly() checks, so that they
 > don't have to be duplicated in the native and compat cases.

 > Similarly for "move range".

I missed to check the detail, and just follow, I can clean up it.

> 
>> +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?

Better.

I can move them before their use.

> 
> - Eric
> .
> 

Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix compat F2FS_IOC_{MOVE, GARBAGE_COLLECT}_RANGE

[Jaegeuk Kim]

On 11/06, Chao Yu wrote:
> On 2020/11/6 8:05, Eric Biggers wrote:
> > 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.
> 
> That's a historical problem, as in last many years, we (f2fs community) don't have
> other long-term reviewers except Jaegeuk and me, so we have unwritten rule: only
> resending changed patch in patchset.
> 
> IMO, that helps to skip traversing unchanged patches, and focusing reviewing on the
> real change lines, and certainly we have its context in mind.
> 
> Personally, I prefer to revise, resend or review patch{,es} of patchset have real
> change line in f2fs mailing list, anyway we can discuss about the rule here.

Chao, I think we need to change this to resend whole patch-set again, since
it's a bit difficult to catch which part of patches were the latest one.

> 
> > 
> > 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>
> Sure, although I attached the link includes original report email, in where it
> points out who "Eric" is.
> 
> > 
> > > +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?
> 
> Quoted
> 
> > It would be better to have __f2fs_ioc_gc_range() handle the f2fs_cp_error(),
> > f2fs_is_checkpoint_ready(), capable(), and f2fs_readonly() checks, so that they
> > don't have to be duplicated in the native and compat cases.
> 
> > Similarly for "move range".
> 
> I missed to check the detail, and just follow, I can clean up it.
> 
> > 
> > > +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?
> 
> Better.
> 
> I can move them before their use.
> 
> > 
> > - Eric
> > .
> > 

Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix compat F2FS_IOC_{MOVE, GARBAGE_COLLECT}_RANGE

[Chao Yu]

On 2020/11/7 5:40, Jaegeuk Kim wrote:
> On 11/06, Chao Yu wrote:
>> On 2020/11/6 8:05, Eric Biggers wrote:
>>> 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.
>>
>> That's a historical problem, as in last many years, we (f2fs community) don't have
>> other long-term reviewers except Jaegeuk and me, so we have unwritten rule: only
>> resending changed patch in patchset.
>>
>> IMO, that helps to skip traversing unchanged patches, and focusing reviewing on the
>> real change lines, and certainly we have its context in mind.
>>
>> Personally, I prefer to revise, resend or review patch{,es} of patchset have real
>> change line in f2fs mailing list, anyway we can discuss about the rule here.
> 
> Chao, I think we need to change this to resend whole patch-set again, since
> it's a bit difficult to catch which part of patches were the latest one.

Oh, I've no objection, if it really helps you.

+Daeho,

Thanks,

> 
>>
>>>
>>> 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>
>> Sure, although I attached the link includes original report email, in where it
>> points out who "Eric" is.
>>
>>>
>>>> +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?
>>
>> Quoted
>>
>>> It would be better to have __f2fs_ioc_gc_range() handle the f2fs_cp_error(),
>>> f2fs_is_checkpoint_ready(), capable(), and f2fs_readonly() checks, so that they
>>> don't have to be duplicated in the native and compat cases.
>>
>>> Similarly for "move range".
>>
>> I missed to check the detail, and just follow, I can clean up it.
>>
>>>
>>>> +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?
>>
>> Better.
>>
>> I can move them before their use.
>>
>>>
>>> - Eric
>>> .
>>>
> .
>