Re: [PATCH for-next 4/4] io_uring/rsrc: optimise registered huge pages

From: Mark Rutland
Date: Thu Mar 16 2023 - 08:12:34 EST

Next message: Vlastimil Babka: "Re: [PATCH v2 1/2] mm: compaction: consider the number of scanning compound pages in isolate fail path"
Previous message: Emanuele Giuseppe Esposito: "Re: [PATCH 0/2] KVM: x86: Fix kvm/queue breakage on clang"
Next in thread: Pavel Begunkov: "Re: [PATCH for-next 4/4] io_uring/rsrc: optimise registered huge pages"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

Since v6.3-rc1, when fuzzing arm64 with Syzkaller, I see a bunch of bad page
state splats with either "nonzero pincount" or "nonzero compound_pincount", and
bisecting led me to this patch / commit:

57bebf807e2abcf8 ("io_uring/rsrc: optimise registered huge pages")

The splats look like:

| BUG: Bad page state in process kworker/u8:0 pfn:5c001
| page:00000000bfda61c8 refcount:0 mapcount:0 mapping:0000000000000000 index:0x20001 pfn:0x5c001
| head:0000000011409842 order:9 entire_mapcount:0 nr_pages_mapped:0 pincount:1
| anon flags: 0x3fffc00000b0004(uptodate|head|mappedtodisk|swapbacked|node=0|zone=0|lastcpupid=0xffff)
| raw: 03fffc0000000000 fffffc0000700001 ffffffff00700903 0000000100000000
| raw: 0000000000000200 0000000000000000 00000000ffffffff 0000000000000000
| head: 03fffc00000b0004 dead000000000100 dead000000000122 ffff00000a809dc1
| head: 0000000000020000 0000000000000000 00000000ffffffff 0000000000000000
| page dumped because: nonzero pincount
| CPU: 3 PID: 9 Comm: kworker/u8:0 Not tainted 6.3.0-rc2-00001-gc6811bf0cd87 #1
| Hardware name: linux,dummy-virt (DT)
| Workqueue: events_unbound io_ring_exit_work
| Call trace:
| dump_backtrace+0x13c/0x208
| show_stack+0x34/0x58
| dump_stack_lvl+0x150/0x1a8
| dump_stack+0x20/0x30
| bad_page+0xec/0x238
| free_tail_pages_check+0x280/0x350
| free_pcp_prepare+0x60c/0x830
| free_unref_page+0x50/0x498
| free_compound_page+0xcc/0x100
| free_transhuge_page+0x1f0/0x2b8
| destroy_large_folio+0x80/0xc8
| __folio_put+0xc4/0xf8
| gup_put_folio+0xd0/0x250
| unpin_user_page+0xcc/0x128
| io_buffer_unmap+0xec/0x2c0
| __io_sqe_buffers_unregister+0xa4/0x1e0
| io_ring_exit_work+0x68c/0x1188
| process_one_work+0x91c/0x1a58
| worker_thread+0x48c/0xe30
| kthread+0x278/0x2f0
| ret_from_fork+0x10/0x20

Syzkaller gave me a reliable C reproducer, which I used to bisect (note:
DEBUG_VM needs to be selected):

| // autogenerated by syzkaller (https://github.com/google/syzkaller)
|
| #define _GNU_SOURCE
|
| #include <endian.h>
| #include <stdint.h>
| #include <stdio.h>
| #include <stdlib.h>
| #include <string.h>
| #include <sys/mman.h>
| #include <sys/syscall.h>
| #include <sys/types.h>
| #include <unistd.h>
|
| #ifndef __NR_io_uring_register
| #define __NR_io_uring_register 427
| #endif
| #ifndef __NR_io_uring_setup
| #define __NR_io_uring_setup 425
| #endif
| #ifndef __NR_mmap
| #define __NR_mmap 222
| #endif
|
| #define SIZEOF_IO_URING_SQE 64
| #define SIZEOF_IO_URING_CQE 16
| #define SQ_HEAD_OFFSET 0
| #define SQ_TAIL_OFFSET 64
| #define SQ_RING_MASK_OFFSET 256
| #define SQ_RING_ENTRIES_OFFSET 264
| #define SQ_FLAGS_OFFSET 276
| #define SQ_DROPPED_OFFSET 272
| #define CQ_HEAD_OFFSET 128
| #define CQ_TAIL_OFFSET 192
| #define CQ_RING_MASK_OFFSET 260
| #define CQ_RING_ENTRIES_OFFSET 268
| #define CQ_RING_OVERFLOW_OFFSET 284
| #define CQ_FLAGS_OFFSET 280
| #define CQ_CQES_OFFSET 320
|
| struct io_sqring_offsets {
| uint32_t head;
| uint32_t tail;
| uint32_t ring_mask;
| uint32_t ring_entries;
| uint32_t flags;
| uint32_t dropped;
| uint32_t array;
| uint32_t resv1;
| uint64_t resv2;
| };
|
| struct io_cqring_offsets {
| uint32_t head;
| uint32_t tail;
| uint32_t ring_mask;
| uint32_t ring_entries;
| uint32_t overflow;
| uint32_t cqes;
| uint64_t resv[2];
| };
|
| struct io_uring_params {
| uint32_t sq_entries;
| uint32_t cq_entries;
| uint32_t flags;
| uint32_t sq_thread_cpu;
| uint32_t sq_thread_idle;
| uint32_t features;
| uint32_t resv[4];
| struct io_sqring_offsets sq_off;
| struct io_cqring_offsets cq_off;
| };
|
| #define IORING_OFF_SQ_RING 0
| #define IORING_OFF_SQES 0x10000000ULL
|
| static long syz_io_uring_setup(volatile long a0, volatile long a1, volatile long a2, volatile long a3, volatile long a4, volatile long a5)
| {
| uint32_t entries = (uint32_t)a0;
| struct io_uring_params* setup_params = (struct io_uring_params*)a1;
| void* vma1 = (void*)a2;
| void* vma2 = (void*)a3;
| void** ring_ptr_out = (void**)a4;
| void** sqes_ptr_out = (void**)a5;
| uint32_t fd_io_uring = syscall(__NR_io_uring_setup, entries, setup_params);
| uint32_t sq_ring_sz = setup_params->sq_off.array + setup_params->sq_entries * sizeof(uint32_t);
| uint32_t cq_ring_sz = setup_params->cq_off.cqes + setup_params->cq_entries * SIZEOF_IO_URING_CQE;
| uint32_t ring_sz = sq_ring_sz > cq_ring_sz ? sq_ring_sz : cq_ring_sz;
| *ring_ptr_out = mmap(vma1, ring_sz, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE | MAP_FIXED, fd_io_uring, IORING_OFF_SQ_RING);
| uint32_t sqes_sz = setup_params->sq_entries * SIZEOF_IO_URING_SQE;
| *sqes_ptr_out = mmap(vma2, sqes_sz, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE | MAP_FIXED, fd_io_uring, IORING_OFF_SQES);
| return fd_io_uring;
| }
|
| uint64_t r[1] = {0xffffffffffffffff};
|
| int main(void)
| {
| syscall(__NR_mmap, 0x1ffff000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
| syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 7ul, 0x32ul, -1, 0ul);
| syscall(__NR_mmap, 0x21000000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
| intptr_t res = 0;
| *(uint32_t*)0x20000684 = 0;
| *(uint32_t*)0x20000688 = 0;
| *(uint32_t*)0x2000068c = 0;
| *(uint32_t*)0x20000690 = 0;
| *(uint32_t*)0x20000698 = -1;
| memset((void*)0x2000069c, 0, 12);
| res = -1;
| res = syz_io_uring_setup(0x2fd6, 0x20000680, 0x20ffd000, 0x20ffc000, 0x20000700, 0x20000740);
| if (res != -1)
| r[0] = res;
| *(uint64_t*)0x20002840 = 0;
| *(uint64_t*)0x20002848 = 0;
| *(uint64_t*)0x20002850 = 0x20000840;
| *(uint64_t*)0x20002858 = 0x1000;
| syscall(__NR_io_uring_register, r[0], 0ul, 0x20002840ul, 2ul);
| return 0;
| }

Thanks,
Mark.

On Wed, Feb 22, 2023 at 02:36:51PM +0000, Pavel Begunkov wrote:
> When registering huge pages, internally io_uring will split them into
> many PAGE_SIZE bvec entries. That's bad for performance as drivers need
> to eventually dma-map the data and will do it individually for each bvec
> entry. Coalesce huge pages into one large bvec.
>
> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
> ---
> io_uring/rsrc.c | 38 ++++++++++++++++++++++++++++++++------
> 1 file changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> index ebbd2cea7582..aab1bc6883e9 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -1210,6 +1210,7 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov,
> unsigned long off;
> size_t size;
> int ret, nr_pages, i;
> + struct folio *folio;
>
> *pimu = ctx->dummy_ubuf;
> if (!iov->iov_base)
> @@ -1224,6 +1225,21 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov,
> goto done;
> }
>
> + /* If it's a huge page, try to coalesce them into a single bvec entry */
> + if (nr_pages > 1) {
> + folio = page_folio(pages[0]);
> + for (i = 1; i < nr_pages; i++) {
> + if (page_folio(pages[i]) != folio) {
> + folio = NULL;
> + break;
> + }
> + }
> + if (folio) {
> + folio_put_refs(folio, nr_pages - 1);
> + nr_pages = 1;
> + }
> + }
> +
> imu = kvmalloc(struct_size(imu, bvec, nr_pages), GFP_KERNEL);
> if (!imu)
> goto done;
> @@ -1236,6 +1252,17 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov,
>
> off = (unsigned long) iov->iov_base & ~PAGE_MASK;
> size = iov->iov_len;
> + /* store original address for later verification */
> + imu->ubuf = (unsigned long) iov->iov_base;
> + imu->ubuf_end = imu->ubuf + iov->iov_len;
> + imu->nr_bvecs = nr_pages;
> + *pimu = imu;
> + ret = 0;
> +
> + if (folio) {
> + bvec_set_page(&imu->bvec[0], pages[0], size, off);
> + goto done;
> + }
> for (i = 0; i < nr_pages; i++) {
> size_t vec_len;
>
> @@ -1244,12 +1271,6 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov,
> off = 0;
> size -= vec_len;
> }
> - /* store original address for later verification */
> - imu->ubuf = (unsigned long) iov->iov_base;
> - imu->ubuf_end = imu->ubuf + iov->iov_len;
> - imu->nr_bvecs = nr_pages;
> - *pimu = imu;
> - ret = 0;
> done:
> if (ret)
> kvfree(imu);
> @@ -1364,6 +1385,11 @@ int io_import_fixed(int ddir, struct iov_iter *iter,
> const struct bio_vec *bvec = imu->bvec;
>
> if (offset <= bvec->bv_len) {
> + /*
> + * Note, huge pages buffers consists of one large
> + * bvec entry and should always go this way. The other
> + * branch doesn't expect non PAGE_SIZE'd chunks.
> + */
> iter->bvec = bvec;
> iter->nr_segs = bvec->bv_len;
> iter->count -= offset;
> --
> 2.39.1
>
>

Next message: Vlastimil Babka: "Re: [PATCH v2 1/2] mm: compaction: consider the number of scanning compound pages in isolate fail path"
Previous message: Emanuele Giuseppe Esposito: "Re: [PATCH 0/2] KVM: x86: Fix kvm/queue breakage on clang"
Next in thread: Pavel Begunkov: "Re: [PATCH for-next 4/4] io_uring/rsrc: optimise registered huge pages"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]