[PATCH 5.18 0/2] selected buffers recycling fixes

[PATCH 5.18 0/2] selected buffers recycling fixes

[Pavel Begunkov]

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

[PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf()

[Pavel Begunkov]

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 <asml.silence@gmail.com>
---
 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

Re: [PATCH 1/2] io_uring: fix invalid flags for io_put_kbuf()

[Jens Axboe]

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 <asml.silence@gmail.com>
> ---
>  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

[PATCH 2/2] io_uring: fix put_kbuf without proper locking

[Pavel Begunkov]

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 <asml.silence@gmail.com>
---
 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

Re: [PATCH 5.18 0/2] selected buffers recycling fixes

[Jens Axboe]

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

Re: [PATCH 5.18 0/2] selected buffers recycling fixes

[Jens Axboe]

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