This initialy started with
[PATCH 1/6] proc: use vmalloc for our kernel buffer
Which came about because we were getting page alloc failures when cat tried to
do a read with a 64kib buffer, triggering an order 4 allocation. We need to
switch to kvmalloc for this buffer to avoid these high order allocations. Then
Christoph suggested renaming vmemdup_user to kvmemdup_user, so then we have this
[PATCH 2/6] tree-wide: rename vmemdup_user to kvmemdup_user
And then finally Viro noticed that if we allocate an extra byte for the NULL
terminator then we can use scnprintf() in a few places, and thus the next set of
patches
[PATCH 3/6] proc: allocate count + 1 for our read buffer
[PATCH 4/6] sysctl: make proc_put_long() use scnprintf
[PATCH 5/6] parport: rework procfs handlers to take advantage of the
[PATCH 6/6] sunrpc: rework proc handlers to take advantage of the new
There's one case that I didn't convert, _proc_do_string, and that's because it's
one of the few places that takes into account ppos, and so we'll skip forward in
the string we're provided with from the caller. In this case it makes sense to
just leave it the way it is. I'm pretty sure I caught all the other people who
directly mess with the buffer, but there's around 800 ->proc_handler's, and my
eyes started to glaze over after a while.
Josef
Since
sysctl: pass kernel pointers to ->proc_handler
we have been pre-allocating a buffer to copy the data from the proc
handlers into, and then copying that to userspace. The problem is this
just blind kmalloc()'s the buffer size passed in from the read, which in
the case of our 'cat' binary was 64kib. Order-4 allocations are not
awesome, and since we can potentially allocate up to our maximum order,
use vmalloc for these buffers.
Fixes: 32927393dc1c ("sysctl: pass kernel pointers to ->proc_handler")
Signed-off-by: Josef Bacik <[email protected]>
---
fs/proc/proc_sysctl.c | 6 +++---
include/linux/string.h | 1 +
mm/util.c | 27 +++++++++++++++++++++++++++
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 6c1166ccdaea..8e19bad83b45 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -571,13 +571,13 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
goto out;
if (write) {
- kbuf = memdup_user_nul(ubuf, count);
+ kbuf = kvmemdup_user_nul(ubuf, count);
if (IS_ERR(kbuf)) {
error = PTR_ERR(kbuf);
goto out;
}
} else {
- kbuf = kzalloc(count, GFP_KERNEL);
+ kbuf = kvzalloc(count, GFP_KERNEL);
if (!kbuf)
goto out;
}
@@ -600,7 +600,7 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
error = count;
out_free_buf:
- kfree(kbuf);
+ kvfree(kbuf);
out:
sysctl_head_finish(head);
diff --git a/include/linux/string.h b/include/linux/string.h
index 9b7a0632e87a..21bb6d3d88c4 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -12,6 +12,7 @@
extern char *strndup_user(const char __user *, long);
extern void *memdup_user(const void __user *, size_t);
extern void *vmemdup_user(const void __user *, size_t);
+extern void *kvmemdup_user_nul(const void __user *, size_t);
extern void *memdup_user_nul(const void __user *, size_t);
/*
diff --git a/mm/util.c b/mm/util.c
index 5ef378a2a038..cf454d57d3e2 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -208,6 +208,33 @@ void *vmemdup_user(const void __user *src, size_t len)
}
EXPORT_SYMBOL(vmemdup_user);
+/**
+ * kvmemdup_user_nul - duplicate memory region from user space and NUL-terminate
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ *
+ * Return: an ERR_PTR() on failure. Result may be not
+ * physically contiguous. Use kvfree() to free.
+ */
+void *kvmemdup_user_nul(const void __user *src, size_t len)
+{
+ char *p;
+
+ p = kvmalloc(len + 1, GFP_USER);
+ if (!p)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(p, src, len)) {
+ kvfree(p);
+ return ERR_PTR(-EFAULT);
+ }
+ p[len] = '\0';
+
+ return p;
+}
+EXPORT_SYMBOL(kvmemdup_user_nul);
+
/**
* strndup_user - duplicate an existing string from user space
* @s: The string to duplicate
--
2.24.1
On Thu, Aug 13, 2020 at 05:04:06PM -0400, Josef Bacik wrote:
> Since
>
> sysctl: pass kernel pointers to ->proc_handler
>
> we have been pre-allocating a buffer to copy the data from the proc
> handlers into, and then copying that to userspace. The problem is this
> just blind kmalloc()'s the buffer size passed in from the read, which in
> the case of our 'cat' binary was 64kib. Order-4 allocations are not
> awesome, and since we can potentially allocate up to our maximum order,
> use vmalloc for these buffers.
Maybe the subject should read ".. also use vmalloc" as we still default
to kmalloc for small allocations?
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>