Eric reported a ioctl bug in below link:
https://lore.kernel.org/linux-f2fs-devel/[email protected]/
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
Reported-by: Eric Biggers <[email protected]>
Signed-off-by: Chao Yu <[email protected]>
---
v4:
- add Reported-by tag
- avoid redundant codes for cleanup
- move compat definitions to .c
fs/f2fs/file.c | 127 +++++++++++++++++++++++++++++++++++++++----------
1 file changed, 101 insertions(+), 26 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 22ae8ae0072f..1464a8d8e969 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2480,26 +2480,19 @@ 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 (!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 +2501,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 +2510,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,9 +2870,9 @@ 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 fd dst;
int err;
@@ -2877,11 +2880,7 @@ static int f2fs_ioc_move_range(struct file *filp, unsigned long arg)
!(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 +2893,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 +2902,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 +4239,70 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
}
#ifdef CONFIG_COMPAT
+#if defined(__KERNEL__)
+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
+static int f2fs_compat_ioc_gc_range(struct file *file, unsigned long arg)
+{
+ struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
+ struct compat_f2fs_gc_range __user *urange;
+ struct f2fs_gc_range range;
+ int err;
+
+ if (unlikely(f2fs_cp_error(sbi)))
+ return -EIO;
+ if (!f2fs_is_checkpoint_ready(sbi))
+ return -ENOSPC;
+
+ 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);
+}
+
+#if defined(__KERNEL__)
+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
+static int f2fs_compat_ioc_move_range(struct file *file, unsigned long arg)
+{
+ struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
+ struct compat_f2fs_move_range __user *urange;
+ struct f2fs_move_range range;
+ int err;
+
+ if (unlikely(f2fs_cp_error(sbi)))
+ return -EIO;
+ if (!f2fs_is_checkpoint_ready(sbi))
+ return -ENOSPC;
+
+ 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 +4315,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 +4336,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:
--
2.26.2
On Fri, Nov 06, 2020 at 02:53:31PM +0800, Chao Yu wrote:
> +#if defined(__KERNEL__)
> +struct compat_f2fs_gc_range {
> + u32 sync;
> + compat_u64 start;
> + compat_u64 len;
> +};
There's no need to use '#if defined(__KERNEL__)' in kernel source files.
Likewise for compat_f2fs_move_range.
> +static int f2fs_compat_ioc_gc_range(struct file *file, unsigned long arg)
> +{
> + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
> + struct compat_f2fs_gc_range __user *urange;
> + struct f2fs_gc_range range;
> + int err;
> +
> + if (unlikely(f2fs_cp_error(sbi)))
> + return -EIO;
> + if (!f2fs_is_checkpoint_ready(sbi))
> + return -ENOSPC;
I still don't understand why this checkpoint-related stuff is getting added
here, and only to the compat versions of the ioctls. It wasn't in the original
version. If they are needed then they should be added to __f2fs_ioc_gc_range()
and __f2fs_ioc_move_range() (preferably by a separate patch) so that they are
done for both the native and compat versions of these ioctls.
- Eric
On 2020/11/7 2:03, Eric Biggers wrote:
> On Fri, Nov 06, 2020 at 02:53:31PM +0800, Chao Yu wrote:
>> +#if defined(__KERNEL__)
>> +struct compat_f2fs_gc_range {
>> + u32 sync;
>> + compat_u64 start;
>> + compat_u64 len;
>> +};
>
> There's no need to use '#if defined(__KERNEL__)' in kernel source files.
>
> Likewise for compat_f2fs_move_range.
Correct.
>
>> +static int f2fs_compat_ioc_gc_range(struct file *file, unsigned long arg)
>> +{
>> + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
>> + struct compat_f2fs_gc_range __user *urange;
>> + struct f2fs_gc_range range;
>> + int err;
>> +
>> + if (unlikely(f2fs_cp_error(sbi)))
>> + return -EIO;
>> + if (!f2fs_is_checkpoint_ready(sbi))
>> + return -ENOSPC;
>
> I still don't understand why this checkpoint-related stuff is getting added
> here, and only to the compat versions of the ioctls. It wasn't in the original
> version. If they are needed then they should be added to __f2fs_ioc_gc_range()
> and __f2fs_ioc_move_range() (preferably by a separate patch) so that they are
If so, cp-related stuff will be checked redundantly in both f2fs_ioctl() and
__f2fs_ioc_xxx() function for native GC_RANGE and MOVE_RANGE ioctls, it's not
needed.
Thanks,
> done for both the native and compat versions of these ioctls.
>
> - Eric
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
On Sat, Nov 07, 2020 at 05:25:23PM +0800, Chao Yu wrote:
> On 2020/11/7 2:03, Eric Biggers wrote:
> > On Fri, Nov 06, 2020 at 02:53:31PM +0800, Chao Yu wrote:
> > > +#if defined(__KERNEL__)
> > > +struct compat_f2fs_gc_range {
> > > + u32 sync;
> > > + compat_u64 start;
> > > + compat_u64 len;
> > > +};
> >
> > There's no need to use '#if defined(__KERNEL__)' in kernel source files.
> >
> > Likewise for compat_f2fs_move_range.
>
> Correct.
>
> >
> > > +static int f2fs_compat_ioc_gc_range(struct file *file, unsigned long arg)
> > > +{
> > > + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
> > > + struct compat_f2fs_gc_range __user *urange;
> > > + struct f2fs_gc_range range;
> > > + int err;
> > > +
> > > + if (unlikely(f2fs_cp_error(sbi)))
> > > + return -EIO;
> > > + if (!f2fs_is_checkpoint_ready(sbi))
> > > + return -ENOSPC;
> >
> > I still don't understand why this checkpoint-related stuff is getting added
> > here, and only to the compat versions of the ioctls. It wasn't in the original
> > version. If they are needed then they should be added to __f2fs_ioc_gc_range()
> > and __f2fs_ioc_move_range() (preferably by a separate patch) so that they are
>
> If so, cp-related stuff will be checked redundantly in both f2fs_ioctl() and
> __f2fs_ioc_xxx() function for native GC_RANGE and MOVE_RANGE ioctls, it's
> not needed.
>
Oh I see, the cp-related checks are at the beginning of f2fs_ioctl() too.
In that case a much better approach would be to add __f2fs_ioctl() which is
called by f2fs_ioctl() and f2fs_compat_ioctl(), and have f2fs_ioctl() and
f2fs_compat_ioctl() do the cp-related checks but not __f2fs_ioctl().
I feel that's still not entirely correct, because ENOTTY should take precedence
over EIO or ENOSPC from the cp-related stuff. But at least it would be
consistent between the native and compat ioctls, and the cp-related checks
wouldn't have to be duplicated in random ioctls...
- Eric
On 2020/11/8 1:16, Eric Biggers wrote:
> On Sat, Nov 07, 2020 at 05:25:23PM +0800, Chao Yu wrote:
>> On 2020/11/7 2:03, Eric Biggers wrote:
>>> On Fri, Nov 06, 2020 at 02:53:31PM +0800, Chao Yu wrote:
>>>> +#if defined(__KERNEL__)
>>>> +struct compat_f2fs_gc_range {
>>>> + u32 sync;
>>>> + compat_u64 start;
>>>> + compat_u64 len;
>>>> +};
>>>
>>> There's no need to use '#if defined(__KERNEL__)' in kernel source files.
>>>
>>> Likewise for compat_f2fs_move_range.
>>
>> Correct.
>>
>>>
>>>> +static int f2fs_compat_ioc_gc_range(struct file *file, unsigned long arg)
>>>> +{
>>>> + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
>>>> + struct compat_f2fs_gc_range __user *urange;
>>>> + struct f2fs_gc_range range;
>>>> + int err;
>>>> +
>>>> + if (unlikely(f2fs_cp_error(sbi)))
>>>> + return -EIO;
>>>> + if (!f2fs_is_checkpoint_ready(sbi))
>>>> + return -ENOSPC;
>>>
>>> I still don't understand why this checkpoint-related stuff is getting added
>>> here, and only to the compat versions of the ioctls. It wasn't in the original
>>> version. If they are needed then they should be added to __f2fs_ioc_gc_range()
>>> and __f2fs_ioc_move_range() (preferably by a separate patch) so that they are
>>
>> If so, cp-related stuff will be checked redundantly in both f2fs_ioctl() and
>> __f2fs_ioc_xxx() function for native GC_RANGE and MOVE_RANGE ioctls, it's
>> not needed.
>>
>
> Oh I see, the cp-related checks are at the beginning of f2fs_ioctl() too.
>
> In that case a much better approach would be to add __f2fs_ioctl() which is
> called by f2fs_ioctl() and f2fs_compat_ioctl(), and have f2fs_ioctl() and
> f2fs_compat_ioctl() do the cp-related checks but not __f2fs_ioctl().
Will this cleanup make sense to you?
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 458724c00d98..1439577108c2 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -4249,16 +4249,10 @@ struct compat_f2fs_gc_range {
static int f2fs_compat_ioc_gc_range(struct file *file, unsigned long arg)
{
- struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
struct compat_f2fs_gc_range __user *urange;
struct f2fs_gc_range range;
int err;
- if (unlikely(f2fs_cp_error(sbi)))
- return -EIO;
- if (!f2fs_is_checkpoint_ready(sbi))
- return -ENOSPC;
-
urange = compat_ptr(arg);
err = get_user(range.sync, &urange->sync);
err |= get_user(range.start, &urange->start);
@@ -4280,16 +4274,10 @@ struct compat_f2fs_move_range {
static int f2fs_compat_ioc_move_range(struct file *file, unsigned long arg)
{
- struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
struct compat_f2fs_move_range __user *urange;
struct f2fs_move_range range;
int err;
- if (unlikely(f2fs_cp_error(sbi)))
- return -EIO;
- if (!f2fs_is_checkpoint_ready(sbi))
- return -ENOSPC;
-
urange = compat_ptr(arg);
err = get_user(range.dst_fd, &urange->dst_fd);
err |= get_user(range.pos_in, &urange->pos_in);
@@ -4301,6 +4289,27 @@ static int f2fs_compat_ioc_move_range(struct file *file, unsigned long arg)
return __f2fs_ioc_move_range(file, &range);
}
+static long __f2fs_compat_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(file));
+
+ if (unlikely(f2fs_cp_error(sbi)))
+ return -EIO;
+ if (!f2fs_is_checkpoint_ready(sbi))
+ return -ENOSPC;
+
+ switch (cmd) {
+ 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);
+ default:
+ return -ENOIOCTLCMD;
+ }
+ return 0;
+}
+
long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
switch (cmd) {
@@ -4314,9 +4323,8 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
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);
+ return __f2fs_compat_ioctl(file, cmd, arg);
case F2FS_IOC_START_ATOMIC_WRITE:
case F2FS_IOC_COMMIT_ATOMIC_WRITE:
case F2FS_IOC_START_VOLATILE_WRITE:
Thanks,
>
> I feel that's still not entirely correct, because ENOTTY should take precedence
> over EIO or ENOSPC from the cp-related stuff. But at least it would be
> consistent between the native and compat ioctls, and the cp-related checks
> wouldn't have to be duplicated in random ioctls...
>
> - Eric
> .
>
On Mon, Nov 09, 2020 at 10:29:25AM +0800, Chao Yu wrote:
> > Oh I see, the cp-related checks are at the beginning of f2fs_ioctl() too.
> >
> > In that case a much better approach would be to add __f2fs_ioctl() which is
> > called by f2fs_ioctl() and f2fs_compat_ioctl(), and have f2fs_ioctl() and
> > f2fs_compat_ioctl() do the cp-related checks but not __f2fs_ioctl().
>
> Will this cleanup make sense to you?
I think it would be better to do it the way I suggested.
- Eric
On 2020/11/10 2:12, Eric Biggers wrote:
> On Mon, Nov 09, 2020 at 10:29:25AM +0800, Chao Yu wrote:
>>> Oh I see, the cp-related checks are at the beginning of f2fs_ioctl() too.
>>>
>>> In that case a much better approach would be to add __f2fs_ioctl() which is
>>> called by f2fs_ioctl() and f2fs_compat_ioctl(), and have f2fs_ioctl() and
>>> f2fs_compat_ioctl() do the cp-related checks but not __f2fs_ioctl().
>>
>> Will this cleanup make sense to you?
>
> I think it would be better to do it the way I suggested.
Updated in v5.
Thanks,
>
> - Eric
> .
>