* [PATCH] io_uring: fix page leak in io_sqe_buffer_register()
@ 2025-06-17 12:39 Penglei Jiang
2025-06-17 12:53 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Penglei Jiang @ 2025-06-17 12:39 UTC (permalink / raw)
To: axboe; +Cc: io-uring, linux-kernel, Penglei Jiang
Add missing unpin_user_pages() in the error path
Fixes: d8c2237d0aa9 ("io_uring: add io_pin_pages() helper")
Signed-off-by: Penglei Jiang <superman.xpt@gmail.com>
---
io_uring/rsrc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index c592ceace97d..f5ac1b530e21 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -804,8 +804,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
}
imu = io_alloc_imu(ctx, nr_pages);
- if (!imu)
+ if (!imu) {
+ unpin_user_pages(pages, nr_pages);
goto done;
+ }
imu->nr_bvecs = nr_pages;
ret = io_buffer_account_pin(ctx, pages, nr_pages, imu, last_hpage);
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] io_uring: fix page leak in io_sqe_buffer_register() 2025-06-17 12:39 [PATCH] io_uring: fix page leak in io_sqe_buffer_register() Penglei Jiang @ 2025-06-17 12:53 ` Jens Axboe 2025-06-17 14:02 ` Penglei Jiang 0 siblings, 1 reply; 4+ messages in thread From: Jens Axboe @ 2025-06-17 12:53 UTC (permalink / raw) To: Penglei Jiang; +Cc: io-uring, linux-kernel On 6/17/25 6:39 AM, Penglei Jiang wrote: > Add missing unpin_user_pages() in the error path > > Fixes: d8c2237d0aa9 ("io_uring: add io_pin_pages() helper") > Signed-off-by: Penglei Jiang <superman.xpt@gmail.com> > --- > io_uring/rsrc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c > index c592ceace97d..f5ac1b530e21 100644 > --- a/io_uring/rsrc.c > +++ b/io_uring/rsrc.c > @@ -804,8 +804,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, > } > > imu = io_alloc_imu(ctx, nr_pages); > - if (!imu) > + if (!imu) { > + unpin_user_pages(pages, nr_pages); > goto done; > + } > > imu->nr_bvecs = nr_pages; > ret = io_buffer_account_pin(ctx, pages, nr_pages, imu, last_hpage); Wouldn't it be better to have the unpin be part of the normal error handling? Not sure why the pin accounting failure doesn't do that already. Totally untested... diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 94a9db030e0e..a68f0cd677a3 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -809,10 +809,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); + if (ret) goto done; - } size = iov->iov_len; /* store original address for later verification */ @@ -840,6 +838,7 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, } done: if (ret) { + unpin_user_pages(pages, nr_pages); if (imu) io_free_imu(ctx, imu); io_cache_free(&ctx->node_cache, node); -- Jens Axboe ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] io_uring: fix page leak in io_sqe_buffer_register() 2025-06-17 12:53 ` Jens Axboe @ 2025-06-17 14:02 ` Penglei Jiang 2025-06-17 14:29 ` Jens Axboe 0 siblings, 1 reply; 4+ messages in thread From: Penglei Jiang @ 2025-06-17 14:02 UTC (permalink / raw) To: axboe; +Cc: io-uring, linux-kernel, superman.xpt On Tue, 17 Jun 2025 06:53:04 -0600, Jens Axboe wrote: > On 6/17/25 6:39 AM, Penglei Jiang wrote: > > Add missing unpin_user_pages() in the error path > > > > Fixes: d8c2237d0aa9 ("io_uring: add io_pin_pages() helper") > > Signed-off-by: Penglei Jiang <superman.xpt@gmail.com> > > --- > > io_uring/rsrc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c > > index c592ceace97d..f5ac1b530e21 100644 > > --- a/io_uring/rsrc.c > > +++ b/io_uring/rsrc.c > > @@ -804,8 +804,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, > > } > > > > imu = io_alloc_imu(ctx, nr_pages); > > - if (!imu) > > + if (!imu) { > > + unpin_user_pages(pages, nr_pages); > > goto done; > > + } > > > > imu->nr_bvecs = nr_pages; > > ret = io_buffer_account_pin(ctx, pages, nr_pages, imu, last_hpage); > > Wouldn't it be better to have the unpin be part of the normal error > handling? Not sure why the pin accounting failure doesn't do that > already. > > Totally untested... > > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c > index 94a9db030e0e..a68f0cd677a3 100644 > --- a/io_uring/rsrc.c > +++ b/io_uring/rsrc.c > @@ -809,10 +809,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); > + if (ret) > goto done; > - } > > size = iov->iov_len; > /* store original address for later verification */ > @@ -840,6 +838,7 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, > } > done: > if (ret) { > + unpin_user_pages(pages, nr_pages); > if (imu) > io_free_imu(ctx, imu); > io_cache_free(&ctx->node_cache, node); Thank you for taking the time to address this issue! However, if io_pin_pages() fails, it will also jump to the done label, but at that point, the value of nr_pages is undefined because nr_pages is only assigned a value inside io_pin_pages() if it succeeds. pages = io_pin_pages((unsigned long) iov->iov_base, iov->iov_len, &nr_pages); if (IS_ERR(pages)) { ret = PTR_ERR(pages); pages = NULL; goto done; } ... done: if (ret) { unpin_user_pages(NULL, undefined-value); ... I'm not sure what the impact of calling unpin_user_pages() in this way would be. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] io_uring: fix page leak in io_sqe_buffer_register() 2025-06-17 14:02 ` Penglei Jiang @ 2025-06-17 14:29 ` Jens Axboe 0 siblings, 0 replies; 4+ messages in thread From: Jens Axboe @ 2025-06-17 14:29 UTC (permalink / raw) To: Penglei Jiang; +Cc: io-uring, linux-kernel On 6/17/25 8:02 AM, Penglei Jiang wrote: > On Tue, 17 Jun 2025 06:53:04 -0600, Jens Axboe wrote: >> On 6/17/25 6:39 AM, Penglei Jiang wrote: >>> Add missing unpin_user_pages() in the error path >>> >>> Fixes: d8c2237d0aa9 ("io_uring: add io_pin_pages() helper") >>> Signed-off-by: Penglei Jiang <superman.xpt@gmail.com> >>> --- >>> io_uring/rsrc.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c >>> index c592ceace97d..f5ac1b530e21 100644 >>> --- a/io_uring/rsrc.c >>> +++ b/io_uring/rsrc.c >>> @@ -804,8 +804,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, >>> } >>> >>> imu = io_alloc_imu(ctx, nr_pages); >>> - if (!imu) >>> + if (!imu) { >>> + unpin_user_pages(pages, nr_pages); >>> goto done; >>> + } >>> >>> imu->nr_bvecs = nr_pages; >>> ret = io_buffer_account_pin(ctx, pages, nr_pages, imu, last_hpage); >> >> Wouldn't it be better to have the unpin be part of the normal error >> handling? Not sure why the pin accounting failure doesn't do that >> already. >> >> Totally untested... >> >> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c >> index 94a9db030e0e..a68f0cd677a3 100644 >> --- a/io_uring/rsrc.c >> +++ b/io_uring/rsrc.c >> @@ -809,10 +809,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); >> + if (ret) >> goto done; >> - } >> >> size = iov->iov_len; >> /* store original address for later verification */ >> @@ -840,6 +838,7 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, >> } >> done: >> if (ret) { >> + unpin_user_pages(pages, nr_pages); >> if (imu) >> io_free_imu(ctx, imu); >> io_cache_free(&ctx->node_cache, node); > > Thank you for taking the time to address this issue! > > However, if io_pin_pages() fails, it will also jump to the done label, > but at that point, the value of nr_pages is undefined because nr_pages > is only assigned a value inside io_pin_pages() if it succeeds. > > pages = io_pin_pages((unsigned long) iov->iov_base, iov->iov_len, > &nr_pages); > if (IS_ERR(pages)) { > ret = PTR_ERR(pages); > pages = NULL; > goto done; > } > > ... > > done: > if (ret) { > unpin_user_pages(NULL, undefined-value); > ... > > I'm not sure what the impact of calling unpin_user_pages() in this way would be. We should just check for 'pages' being valid first. Updated below. If you want to test and send a v2 based on that, I do think that's the better approach as it keeps all the error handling consistent. diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 94a9db030e0e..454cd8855c6c 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -809,10 +809,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); + if (ret) goto done; - } size = iov->iov_len; /* store original address for later verification */ @@ -840,6 +838,8 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, } done: if (ret) { + if (pages) + unpin_user_pages(pages, nr_pages); if (imu) io_free_imu(ctx, imu); io_cache_free(&ctx->node_cache, node); -- Jens Axboe ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-06-17 14:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-17 12:39 [PATCH] io_uring: fix page leak in io_sqe_buffer_register() Penglei Jiang 2025-06-17 12:53 ` Jens Axboe 2025-06-17 14:02 ` Penglei Jiang 2025-06-17 14:29 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox