* [PATCH] nilfs2: replace vmalloc + copy_from_user with vmemdup_user
@ 2025-09-23 12:09 Thorsten Blum
2025-09-24 12:18 ` Ryusuke Konishi
0 siblings, 1 reply; 4+ messages in thread
From: Thorsten Blum @ 2025-09-23 12:09 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: Thorsten Blum, linux-nilfs, linux-kernel
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@linux.dev>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] nilfs2: replace vmalloc + copy_from_user with vmemdup_user
2025-09-23 12:09 [PATCH] nilfs2: replace vmalloc + copy_from_user with vmemdup_user Thorsten Blum
@ 2025-09-24 12:18 ` Ryusuke Konishi
2025-09-24 13:45 ` Thorsten Blum
0 siblings, 1 reply; 4+ messages in thread
From: Ryusuke Konishi @ 2025-09-24 12:18 UTC (permalink / raw)
To: Thorsten Blum; +Cc: linux-nilfs, linux-kernel
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@linux.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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nilfs2: replace vmalloc + copy_from_user with vmemdup_user
2025-09-24 12:18 ` Ryusuke Konishi
@ 2025-09-24 13:45 ` Thorsten Blum
0 siblings, 0 replies; 4+ messages in thread
From: Thorsten Blum @ 2025-09-24 13:45 UTC (permalink / raw)
To: Ryusuke Konishi; +Cc: linux-nilfs, linux-kernel
Hi Ryusuke,
On 24. Sep 2025, at 14:18, Ryusuke Konishi wrote:
> ...
> So, rather than rushing it into the upcoming merge window, please let
> me schedule this to be sent upstream in the cycle after that.
Sounds good as this was meant for the next cycle anyway.
Thanks,
Thorsten
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] nilfs2: replace vmalloc + copy_from_user with vmemdup_user
@ 2025-10-30 15:46 Ryusuke Konishi
0 siblings, 0 replies; 4+ messages in thread
From: Ryusuke Konishi @ 2025-10-30 15:46 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-nilfs, Thorsten Blum, linux-kernel
From: Thorsten Blum <thorsten.blum@linux.dev>
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@linux.dev>
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
---
Hi Andrew,
Please queue this for the next cycle.
This updates the ioctl handling code of nilfs2 to follow current
practices.
Thanks,
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.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-30 15:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-23 12:09 [PATCH] nilfs2: replace vmalloc + copy_from_user with vmemdup_user Thorsten Blum
2025-09-24 12:18 ` Ryusuke Konishi
2025-09-24 13:45 ` Thorsten Blum
-- strict thread matches above, loose matches on Subject: below --
2025-10-30 15:46 Ryusuke Konishi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).