Re: [PATCH v2 1/3] io_uring/rsrc: fix folio unpinning

[David Hildenbrand <david@redhat.com>]

On 24.06.25 15:40, Pavel Begunkov wrote:
> [  108.070381][   T14] kernel BUG at mm/gup.c:71!
> [  108.070502][   T14] Internal error: Oops - BUG: 00000000f2000800 [#1]  SMP
> [  108.123672][   T14] Hardware name: QEMU KVM Virtual Machine, BIOS edk2-20250221-8.fc42 02/21/2025
> [  108.127458][   T14] Workqueue: iou_exit io_ring_exit_work
> [  108.174205][   T14] Call trace:
> [  108.175649][   T14]  sanity_check_pinned_pages+0x7cc/0x7d0 (P)
> [  108.178138][   T14]  unpin_user_page+0x80/0x10c
> [  108.180189][   T14]  io_release_ubuf+0x84/0xf8
> [  108.182196][   T14]  io_free_rsrc_node+0x250/0x57c
> [  108.184345][   T14]  io_rsrc_data_free+0x148/0x298
> [  108.186493][   T14]  io_sqe_buffers_unregister+0x84/0xa0
> [  108.188991][   T14]  io_ring_ctx_free+0x48/0x480
> [  108.191057][   T14]  io_ring_exit_work+0x764/0x7d8
> [  108.193207][   T14]  process_one_work+0x7e8/0x155c
> [  108.195431][   T14]  worker_thread+0x958/0xed8
> [  108.197561][   T14]  kthread+0x5fc/0x75c
> [  108.199362][   T14]  ret_from_fork+0x10/0x20
> 
> We can pin a tail page of a folio, but then io_uring will try to unpin
> the the head page of the folio. While it should be fine in terms of
> keeping the page actually alive, but mm folks say it's wrong and
> triggers a debug warning. Use unpin_user_folio() instead of
> unpin_user_page*.
> 
> Cc: stable@vger.kernel.org
> Debugged-by: David Hildenbrand <david@redhat.com>
> Reported-by: syzbot+1d335893772467199ab6@syzkaller.appspotmail.com
> Closes: https://lkml.kernel.org/r/683f1551.050a0220.55ceb.0017.GAE@google.com
> Fixes: a8edbb424b139 ("io_uring/rsrc: enable multi-hugepage buffer coalescing")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>   io_uring/rsrc.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> index c592ceace97d..e83a294c718b 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -112,8 +112,11 @@ static void io_release_ubuf(void *priv)
>   	struct io_mapped_ubuf *imu = priv;
>   	unsigned int i;
>   
> -	for (i = 0; i < imu->nr_bvecs; i++)
> -		unpin_user_page(imu->bvec[i].bv_page);
> +	for (i = 0; i < imu->nr_bvecs; i++) {
> +		struct folio *folio = page_folio(imu->bvec[i].bv_page);
> +
> +		unpin_user_folio(folio, 1);
> +	}
>   }
>   
>   static struct io_mapped_ubuf *io_alloc_imu(struct io_ring_ctx *ctx,
> @@ -810,7 +813,8 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
>   	imu->nr_bvecs = nr_pages;
>   	ret = io_buffer_account_pin(ctx, pages, nr_pages, imu, last_hpage);
>   	if (ret) {
> -		unpin_user_pages(pages, nr_pages);
> +		for (i = 0; i < nr_pages; i++)
> +			unpin_user_folio(page_folio(pages[i]), 1);
>   		goto done;
>   	}
>   

It should fix the issue, but it's a bit suboptimal in the case where we 
didn't coalesc, but there are folio ranges to coalesc:

unpin_user_pages() does a per-folio coalescing.

So in an ideal world, we would cleanly split both paths, and work with 
folios after we coalesced to use folios, and work with pages, when we 
didn't coalesc to use folios.

Then, we can just use unpin_folios() after we coalesced.

In any case, for a fix this is good enough, but probably we can do 
better later.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb