Re: [PATCH] virtiofs: limit the length of ITER_KVEC dio by max_nopage_rw
From: Bernd Schubert
Date: Wed Jan 10 2024 - 17:34:37 EST
On 1/10/24 02:16, Hou Tao wrote:
Hi,
On 1/9/2024 9:11 PM, Bernd Schubert wrote:
On 1/3/24 11:59, Hou Tao wrote:
From: Hou Tao <houtao1@xxxxxxxxxx>
When trying to insert a 10MB kernel module kept in a virtiofs with cache
disabled, the following warning was reported:
------------[ cut here ]------------
WARNING: CPU: 2 PID: 439 at mm/page_alloc.c:4544 ......
Modules linked in:
CPU: 2 PID: 439 Comm: insmod Not tainted 6.7.0-rc7+ #33
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), ......
RIP: 0010:__alloc_pages+0x2c4/0x360
......
Call Trace:
<TASK>
? __warn+0x8f/0x150
? __alloc_pages+0x2c4/0x360
__kmalloc_large_node+0x86/0x160
__kmalloc+0xcd/0x140
virtio_fs_enqueue_req+0x240/0x6d0
virtio_fs_wake_pending_and_unlock+0x7f/0x190
queue_request_and_unlock+0x58/0x70
fuse_simple_request+0x18b/0x2e0
fuse_direct_io+0x58a/0x850
fuse_file_read_iter+0xdb/0x130
__kernel_read+0xf3/0x260
kernel_read+0x45/0x60
kernel_read_file+0x1ad/0x2b0
init_module_from_file+0x6a/0xe0
idempotent_init_module+0x179/0x230
__x64_sys_finit_module+0x5d/0xb0
do_syscall_64+0x36/0xb0
entry_SYSCALL_64_after_hwframe+0x6e/0x76
......
</TASK>
---[ end trace 0000000000000000 ]---
The warning happened as follow. In copy_args_to_argbuf(), virtiofs uses
kmalloc-ed memory as bound buffer for fuse args, but
fuse_get_user_pages() only limits the length of fuse arg by max_read or
max_write for IOV_KVEC io (e.g., kernel_read_file from finit_module()).
For virtiofs, max_read is UINT_MAX, so a big read request which is about
I find this part of the explanation a bit confusing. I guess you
wanted to write something like
fuse_direct_io() -> fuse_get_user_pages() is limited by
fc->max_write/fc->max_read and fc->max_pages. For virtiofs max_pages
does not apply as ITER_KVEC is used. As virtiofs sets fc->max_read to
UINT_MAX basically no limit is applied at all.
Yes, what you said is just as expected but it is not the root cause of
the warning. The culprit of the warning is kmalloc() in
copy_args_to_argbuf() just as said in commit message. vmalloc() is also
not acceptable, because the physical memory needs to be contiguous. For
the problem, because there is no page involved, so there will be extra
sg available, maybe we can use these sg to break the big read/write
request into page.
Hmm ok, I was hoping that contiguous memory is not needed.
I see that ENOMEM is handled, but how that that perform (or even
complete) on a really badly fragmented system? I guess splitting into
smaller pages or at least adding some reserve kmem_cache (or even
mempool) would make sense?
I also wonder if it wouldn't it make sense to set a sensible limit in
virtio_fs_ctx_set_defaults() instead of introducing a new variable?
As said in the commit message:
A feasible solution is to limit the value of max_read for virtiofs, so
the length passed to kmalloc() will be limited. However it will affects
the max read size for ITER_IOVEC io and the value of max_write also needs
limitation.
It is a bit hard to set a reasonable value for both max_read and
max_write to handle both normal ITER_IOVEC io and ITER_KVEC io. And
considering ITER_KVEC io + dio case is uncommon, I think using a new
limitation is more reasonable.
For ITER_IOVEC max_pages applies - which is limited to
FUSE_MAX_MAX_PAGES - why can't this be used in virtio_fs_ctx_set_defaults?
@Miklos, is there a reason why there is no upper fc->max_{read,write}
limit in process_init_reply()? Shouldn't both be limited to
(FUSE_MAX_MAX_PAGES * PAGE_SIZE). Or any other reasonable limit?
Thanks,
Bernd
Also, I guess the issue is kmalloc_array() in virtio_fs_enqueue_req?
Wouldn't it make sense to use kvm_alloc_array/kvfree in that function?
Thanks,
Bernd
10MB is passed to copy_args_to_argbuf(), kmalloc() is called in turn
with len=10MB, and triggers the warning in __alloc_pages():
WARN_ON_ONCE_GFP(order > MAX_ORDER, gfp)).
A feasible solution is to limit the value of max_read for virtiofs, so
the length passed to kmalloc() will be limited. However it will affects
the max read size for ITER_IOVEC io and the value of max_write also
needs
limitation. So instead of limiting the values of max_read and max_write,
introducing max_nopage_rw to cap both the values of max_read and
max_write when the fuse dio read/write request is initiated from kernel.
Considering that fuse read/write request from kernel is uncommon and to
decrease the demand for large contiguous pages, set max_nopage_rw as
256KB instead of KMALLOC_MAX_SIZE - 4096 or similar.
Fixes: a62a8ef9d97d ("virtio-fs: add virtiofs filesystem")
Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
---
fs/fuse/file.c | 12 +++++++++++-
fs/fuse/fuse_i.h | 3 +++
fs/fuse/inode.c | 1 +
fs/fuse/virtio_fs.c | 6 ++++++
4 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a660f1f21540..f1beb7c0b782 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1422,6 +1422,16 @@ static int fuse_get_user_pages(struct
fuse_args_pages *ap, struct iov_iter *ii,
return ret < 0 ? ret : 0;
}
+static size_t fuse_max_dio_rw_size(const struct fuse_conn *fc,
+ const struct iov_iter *iter, int write)
+{
+ unsigned int nmax = write ? fc->max_write : fc->max_read;
+
+ if (iov_iter_is_kvec(iter))
+ nmax = min(nmax, fc->max_nopage_rw);
+ return nmax;
+}
+
ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
loff_t *ppos, int flags)
{
@@ -1432,7 +1442,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io,
struct iov_iter *iter,
struct inode *inode = mapping->host;
struct fuse_file *ff = file->private_data;
struct fuse_conn *fc = ff->fm->fc;
- size_t nmax = write ? fc->max_write : fc->max_read;
+ size_t nmax = fuse_max_dio_rw_size(fc, iter, write);
loff_t pos = *ppos;
size_t count = iov_iter_count(iter);
pgoff_t idx_from = pos >> PAGE_SHIFT;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 1df83eebda92..fc753cd34211 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -594,6 +594,9 @@ struct fuse_conn {
/** Constrain ->max_pages to this value during feature
negotiation */
unsigned int max_pages_limit;
+ /** Maximum read/write size when there is no page in request */
+ unsigned int max_nopage_rw;
+
/** Input queue */
struct fuse_iqueue iq;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 2a6d44f91729..4cbbcb4a4b71 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -923,6 +923,7 @@ void fuse_conn_init(struct fuse_conn *fc, struct
fuse_mount *fm,
fc->user_ns = get_user_ns(user_ns);
fc->max_pages = FUSE_DEFAULT_MAX_PAGES_PER_REQ;
fc->max_pages_limit = FUSE_MAX_MAX_PAGES;
+ fc->max_nopage_rw = UINT_MAX;
INIT_LIST_HEAD(&fc->mounts);
list_add(&fm->fc_entry, &fc->mounts);
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 5f1be1da92ce..3aac31d45198 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1452,6 +1452,12 @@ static int virtio_fs_get_tree(struct
fs_context *fsc)
/* Tell FUSE to split requests that exceed the virtqueue's size */
fc->max_pages_limit = min_t(unsigned int, fc->max_pages_limit,
virtqueue_size - FUSE_HEADER_OVERHEAD);
+ /* copy_args_to_argbuf() uses kmalloc-ed memory as bounce buffer
+ * for fuse args, so limit the total size of these args to prevent
+ * the warning in __alloc_pages() and decrease the demand for large
+ * contiguous pages.
+ */
+ fc->max_nopage_rw = min(fc->max_nopage_rw, 256U << 10);
fsc->s_fs_info = fm;
sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc);
.