Message-ID: <CAKFNMonHxWSxW72Af8F_3Mhx39iO5hgVKAhTjpHVKsTBOQ-Z8g@mail.gmail.com>
Date: Wed, 24 Sep 2025 21:18:46 +0900
From: Ryusuke Konishi <konishi.ryusuke@...il.com>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: linux-nilfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nilfs2: replace vmalloc + copy_from_user with vmemdup_user

On Tue, Sep 23, 2025 at 9:10 PM Thorsten Blum wrote:
>
> Replace vmalloc() followed by copy_from_user() with vmemdup_user() to
> improve nilfs_ioctl_clean_segments() and nilfs_ioctl_set_suinfo(). Use
> kvfree() to free the buffers created by vmemdup_user().
>
> Use u64_to_user_ptr() instead of manually casting the pointers and
> remove the obsolete 'out_free' label.
>
> No functional changes intended.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> ---

Thank you for your patch proposal.

This patch involves rewriting using vmemdup_user(), u64_to_user_ptr(),
and kvfree(), which to me appear to be equivalent transformations that
improve readability.

However, I would like to actually perform build tests in different
environments (to confirm in multiple environments that the typecheck
for __u64 in u64_to_user_ptr passes without any problems, and that no
side effects appear with the static coding checker).  Also, since it
touches parts related to the operation of user-space programs such as
nilfs_cleanerd and nilfs-resize, I would like to confirm that actual
device tests pass while changing the allocation sizes.

So, rather than rushing it into the upcoming merge window, please let
me schedule this to be sent upstream in the cycle after that.

If there are any issues, I would like to provide feedback.

Regards,
Ryusuke Konishi

>  fs/nilfs2/ioctl.c | 35 ++++++++++-------------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index 3288c3b4be9e..e17b8da66491 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -49,7 +49,7 @@ static int nilfs_ioctl_wrap_copy(struct the_nilfs *nilfs,
>                                                    void *, size_t, size_t))
>  {
>         void *buf;
> -       void __user *base = (void __user *)(unsigned long)argv->v_base;
> +       void __user *base = u64_to_user_ptr(argv->v_base);
>         size_t maxmembs, total, n;
>         ssize_t nr;
>         int ret, i;
> @@ -836,7 +836,6 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
>                 sizeof(struct nilfs_bdesc),
>                 sizeof(__u64),
>         };
> -       void __user *base;
>         void *kbufs[5];
>         struct the_nilfs *nilfs;
>         size_t len, nsegs;
> @@ -863,7 +862,7 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
>          * use kmalloc() for its buffer because the memory used for the
>          * segment numbers is small enough.
>          */
> -       kbufs[4] = memdup_array_user((void __user *)(unsigned long)argv[4].v_base,
> +       kbufs[4] = memdup_array_user(u64_to_user_ptr(argv[4].v_base),
>                                      nsegs, sizeof(__u64));
>         if (IS_ERR(kbufs[4])) {
>                 ret = PTR_ERR(kbufs[4]);
> @@ -883,20 +882,14 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
>                         goto out_free;
>
>                 len = argv[n].v_size * argv[n].v_nmembs;
> -               base = (void __user *)(unsigned long)argv[n].v_base;
>                 if (len == 0) {
>                         kbufs[n] = NULL;
>                         continue;
>                 }
>
> -               kbufs[n] = vmalloc(len);
> -               if (!kbufs[n]) {
> -                       ret = -ENOMEM;
> -                       goto out_free;
> -               }
> -               if (copy_from_user(kbufs[n], base, len)) {
> -                       ret = -EFAULT;
> -                       vfree(kbufs[n]);
> +               kbufs[n] = vmemdup_user(u64_to_user_ptr(argv[n].v_base), len);
> +               if (IS_ERR(kbufs[n])) {
> +                       ret = PTR_ERR(kbufs[n]);
>                         goto out_free;
>                 }
>         }
> @@ -928,7 +921,7 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
>
>  out_free:
>         while (--n >= 0)
> -               vfree(kbufs[n]);
> +               kvfree(kbufs[n]);
>         kfree(kbufs[4]);
>  out:
>         mnt_drop_write_file(filp);
> @@ -1181,7 +1174,6 @@ static int nilfs_ioctl_set_suinfo(struct inode *inode, struct file *filp,
>         struct nilfs_transaction_info ti;
>         struct nilfs_argv argv;
>         size_t len;
> -       void __user *base;
>         void *kbuf;
>         int ret;
>
> @@ -1212,18 +1204,12 @@ static int nilfs_ioctl_set_suinfo(struct inode *inode, struct file *filp,
>                 goto out;
>         }
>
> -       base = (void __user *)(unsigned long)argv.v_base;
> -       kbuf = vmalloc(len);
> -       if (!kbuf) {
> -               ret = -ENOMEM;
> +       kbuf = vmemdup_user(u64_to_user_ptr(argv.v_base), len);
> +       if (IS_ERR(kbuf)) {
> +               ret = PTR_ERR(kbuf);
>                 goto out;
>         }
>
> -       if (copy_from_user(kbuf, base, len)) {
> -               ret = -EFAULT;
> -               goto out_free;
> -       }
> -
>         nilfs_transaction_begin(inode->i_sb, &ti, 0);
>         ret = nilfs_sufile_set_suinfo(nilfs->ns_sufile, kbuf, argv.v_size,
>                         argv.v_nmembs);
> @@ -1232,8 +1218,7 @@ static int nilfs_ioctl_set_suinfo(struct inode *inode, struct file *filp,
>         else
>                 nilfs_transaction_commit(inode->i_sb); /* never fails */
>
> -out_free:
> -       vfree(kbuf);
> +       kvfree(kbuf);
>  out:
>         mnt_drop_write_file(filp);
>         return ret;
> --
> 2.51.0
>

Powered by blists - more mailing lists