public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH 5.18 0/2] selected buffers recycling fixes
@ 2022-03-25 13:00 Pavel Begunkov
  2022-03-25 13:00 ` [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf() Pavel Begunkov
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Pavel Begunkov @ 2022-03-25 13:00 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence

Fix two locking problems with new buffer recycling.

Jens, could you help to test it properly and with lockdep enabled?

Pavel Begunkov (2):
  io_uring: fix invalid flags for io_put_kbuf()
  io_uring: fix put_kbuf without proper locking

 fs/io_uring.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf()
  2022-03-25 13:00 [PATCH 5.18 0/2] selected buffers recycling fixes Pavel Begunkov
@ 2022-03-25 13:00 ` Pavel Begunkov
  2022-03-25 13:42   ` Jens Axboe
  2022-03-25 13:00 ` [PATCH 2/2] io_uring: fix put_kbuf without proper locking Pavel Begunkov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2022-03-25 13:00 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence

io_req_complete_failed() doesn't require callers to hold ->uring_lock,
use IO_URING_F_UNLOCKED version of io_put_kbuf(). The only affected
place is the fail path of io_apoll_task_func(). Also add a lockdep
annotation to catch such bugs in the future.

Fixes: 3b2b78a8eb7cc ("io_uring: extend provided buf return to fails")
Signed-off-by: Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 862401d23a5a..c83a650ca5fa 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1388,6 +1388,8 @@ static inline unsigned int io_put_kbuf(struct io_kiocb *req,
 		cflags = __io_put_kbuf(req, &ctx->io_buffers_comp);
 		spin_unlock(&ctx->completion_lock);
 	} else {
+		lockdep_assert_held(&req->ctx->uring_lock);
+
 		cflags = __io_put_kbuf(req, &req->ctx->io_buffers_cache);
 	}
 
@@ -2182,7 +2184,7 @@ static inline void io_req_complete(struct io_kiocb *req, s32 res)
 static void io_req_complete_failed(struct io_kiocb *req, s32 res)
 {
 	req_set_fail(req);
-	io_req_complete_post(req, res, io_put_kbuf(req, 0));
+	io_req_complete_post(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
 }
 
 static void io_req_complete_fail_submit(struct io_kiocb *req)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] io_uring: fix put_kbuf without proper locking
  2022-03-25 13:00 [PATCH 5.18 0/2] selected buffers recycling fixes Pavel Begunkov
  2022-03-25 13:00 ` [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf() Pavel Begunkov
@ 2022-03-25 13:00 ` Pavel Begunkov
  2022-03-25 13:43 ` [PATCH 5.18 0/2] selected buffers recycling fixes Jens Axboe
  2022-03-25 14:39 ` Jens Axboe
  3 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2022-03-25 13:00 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence

io_put_kbuf_comp() should only be called while holding
->completion_lock, however there is no such assumption in io_clean_op()
and thus it can corrupt ->io_buffer_comp. Take the lock there, and
workaround the only user of io_clean_op() calling it with locks. Not
the prettiest solution, but it's easier to refactor it for-next.

Fixes: cc3cec8367cba ("io_uring: speedup provided buffer handling")
Signed-off-by: Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c83a650ca5fa..e5769287b3b8 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1356,6 +1356,8 @@ static unsigned int __io_put_kbuf(struct io_kiocb *req, struct list_head *list)
 
 static inline unsigned int io_put_kbuf_comp(struct io_kiocb *req)
 {
+	lockdep_assert_held(&req->ctx->completion_lock);
+
 	if (likely(!(req->flags & REQ_F_BUFFER_SELECTED)))
 		return 0;
 	return __io_put_kbuf(req, &req->ctx->io_buffers_comp);
@@ -2140,6 +2142,12 @@ static void __io_req_complete_post(struct io_kiocb *req, s32 res,
 			}
 		}
 		io_req_put_rsrc(req, ctx);
+		/*
+		 * Selected buffer deallocation in io_clean_op() assumes that
+		 * we don't hold ->completion_lock. Clean them here to avoid
+		 * deadlocks.
+		 */
+		io_put_kbuf_comp(req);
 		io_dismantle_req(req);
 		io_put_task(req->task, 1);
 		wq_list_add_head(&req->comp_list, &ctx->locked_free_list);
@@ -7115,8 +7123,11 @@ static __cold void io_drain_req(struct io_kiocb *req)
 
 static void io_clean_op(struct io_kiocb *req)
 {
-	if (req->flags & REQ_F_BUFFER_SELECTED)
+	if (req->flags & REQ_F_BUFFER_SELECTED) {
+		spin_lock(&req->ctx->completion_lock);
 		io_put_kbuf_comp(req);
+		spin_unlock(&req->ctx->completion_lock);
+	}
 
 	if (req->flags & REQ_F_NEED_CLEANUP) {
 		switch (req->opcode) {
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf()
  2022-03-25 13:00 ` [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf() Pavel Begunkov
@ 2022-03-25 13:42   ` Jens Axboe
  0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2022-03-25 13:42 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 3/25/22 7:00 AM, Pavel Begunkov wrote:
> io_req_complete_failed() doesn't require callers to hold ->uring_lock,
> use IO_URING_F_UNLOCKED version of io_put_kbuf(). The only affected
> place is the fail path of io_apoll_task_func(). Also add a lockdep
> annotation to catch such bugs in the future.
> 
> Fixes: 3b2b78a8eb7cc ("io_uring: extend provided buf return to fails")
> Signed-off-by: Pavel Begunkov <[email protected]>
> ---
>  fs/io_uring.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 862401d23a5a..c83a650ca5fa 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1388,6 +1388,8 @@ static inline unsigned int io_put_kbuf(struct io_kiocb *req,
>  		cflags = __io_put_kbuf(req, &ctx->io_buffers_comp);
>  		spin_unlock(&ctx->completion_lock);
>  	} else {
> +		lockdep_assert_held(&req->ctx->uring_lock);
> +
>  		cflags = __io_put_kbuf(req, &req->ctx->io_buffers_cache);
>  	}
>  
> @@ -2182,7 +2184,7 @@ static inline void io_req_complete(struct io_kiocb *req, s32 res)
>  static void io_req_complete_failed(struct io_kiocb *req, s32 res)
>  {
>  	req_set_fail(req);
> -	io_req_complete_post(req, res, io_put_kbuf(req, 0));
> +	io_req_complete_post(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
>  }

That took me a bit to grok, but it's because we don't use the flag here
as "ok we need to grab that same lock, rather we use it for "should we
use something else" which makes it safe. We should probably use a bool
for this case instead rather than abuse issue_flags, for later.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 5.18 0/2] selected buffers recycling fixes
  2022-03-25 13:00 [PATCH 5.18 0/2] selected buffers recycling fixes Pavel Begunkov
  2022-03-25 13:00 ` [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf() Pavel Begunkov
  2022-03-25 13:00 ` [PATCH 2/2] io_uring: fix put_kbuf without proper locking Pavel Begunkov
@ 2022-03-25 13:43 ` Jens Axboe
  2022-03-25 14:39 ` Jens Axboe
  3 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2022-03-25 13:43 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On 3/25/22 7:00 AM, Pavel Begunkov wrote:
> Fix two locking problems with new buffer recycling.
> 
> Jens, could you help to test it properly and with lockdep enabled?

Yep, I'll run them through the testing, thanks!

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 5.18 0/2] selected buffers recycling fixes
  2022-03-25 13:00 [PATCH 5.18 0/2] selected buffers recycling fixes Pavel Begunkov
                   ` (2 preceding siblings ...)
  2022-03-25 13:43 ` [PATCH 5.18 0/2] selected buffers recycling fixes Jens Axboe
@ 2022-03-25 14:39 ` Jens Axboe
  3 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2022-03-25 14:39 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

On Fri, 25 Mar 2022 13:00:41 +0000, Pavel Begunkov wrote:
> Fix two locking problems with new buffer recycling.
> 
> Jens, could you help to test it properly and with lockdep enabled?
> 
> Pavel Begunkov (2):
>   io_uring: fix invalid flags for io_put_kbuf()
>   io_uring: fix put_kbuf without proper locking
> 
> [...]

Applied, thanks!

[1/2] io_uring: fix invalid flags for io_put_kbuf()
      commit: ab0ac0959b028779ea43002db81daa12203cb57d
[2/2] io_uring: fix put_kbuf without proper locking
      commit: 8197b053a83335dd1b7eb7581a933924e25c1025

Best regards,
-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-03-25 14:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-25 13:00 [PATCH 5.18 0/2] selected buffers recycling fixes Pavel Begunkov
2022-03-25 13:00 ` [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf() Pavel Begunkov
2022-03-25 13:42   ` Jens Axboe
2022-03-25 13:00 ` [PATCH 2/2] io_uring: fix put_kbuf without proper locking Pavel Begunkov
2022-03-25 13:43 ` [PATCH 5.18 0/2] selected buffers recycling fixes Jens Axboe
2022-03-25 14:39 ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox