[PATCHSET for-next 0/2] msg_ring fixes

[PATCHSET for-next 0/2] msg_ring fixes

[Jens Axboe]

Hi,

A fix for the issue that Pavel highlighted in the conversion to using
internal task_work for data requests, and while in there, postted an
error in the caching where the wrong freeing function is used.

 io_uring/msg_ring.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

-- 
Jens Axboe

[PATCH 1/2] io_uring/msg_ring: check for dead submitter task

[Jens Axboe]

The change for improving the handling of the target CQE posting
inadvertently dropped the NULL check for the submitter task on the target
ring, reinstate that.

Fixes: 0617bb500bfa ("io_uring/msg_ring: improve handling of target CQE posting")
Reported-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/msg_ring.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index 47a754e83b49..c2171495098b 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -86,16 +86,21 @@ static void io_msg_tw_complete(struct io_kiocb *req, struct io_tw_state *ts)
 	percpu_ref_put(&ctx->refs);
 }
 
-static void io_msg_remote_post(struct io_ring_ctx *ctx, struct io_kiocb *req,
-			       int res, u32 cflags, u64 user_data)
+static int io_msg_remote_post(struct io_ring_ctx *ctx, struct io_kiocb *req,
+			      int res, u32 cflags, u64 user_data)
 {
+	req->task = READ_ONCE(ctx->submitter_task);
+	if (!req->task) {
+		kmem_cache_free(req_cachep, req);
+		return -EOWNERDEAD;
+	}
 	req->cqe.user_data = user_data;
 	io_req_set_res(req, res, cflags);
 	percpu_ref_get(&ctx->refs);
 	req->ctx = ctx;
-	req->task = READ_ONCE(ctx->submitter_task);
 	req->io_task_work.func = io_msg_tw_complete;
 	io_req_task_work_add_remote(req, ctx, IOU_F_TWQ_LAZY_WAKE);
+	return 0;
 }
 
 static struct io_kiocb *io_msg_get_kiocb(struct io_ring_ctx *ctx)
@@ -125,8 +130,8 @@ static int io_msg_data_remote(struct io_kiocb *req)
 	if (msg->flags & IORING_MSG_RING_FLAGS_PASS)
 		flags = msg->cqe_flags;
 
-	io_msg_remote_post(target_ctx, target, msg->len, flags, msg->user_data);
-	return 0;
+	return io_msg_remote_post(target_ctx, target, msg->len, flags,
+					msg->user_data);
 }
 
 static int io_msg_ring_data(struct io_kiocb *req, unsigned int issue_flags)
-- 
2.43.0

[PATCH 2/2] io_uring/msg_ring: use kmem_cache_free() to free request

[Jens Axboe]

The change adding caching around the request allocated and freed for
data messages changed a kmem_cache_free() to a kfree(), which isn't
correct as the request came from slab in the first place. Fix that up
and use the right freeing function if the cache is already at its limit.

Fixes: 50cf5f3842af ("io_uring/msg_ring: add an alloc cache for io_kiocb entries")
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/msg_ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index c2171495098b..29fa9285a33d 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -82,7 +82,7 @@ static void io_msg_tw_complete(struct io_kiocb *req, struct io_tw_state *ts)
 		spin_unlock(&ctx->msg_lock);
 	}
 	if (req)
-		kfree(req);
+		kmem_cache_free(req_cachep, req);
 	percpu_ref_put(&ctx->refs);
 }
 
-- 
2.43.0

Re: [PATCH 2/2] io_uring/msg_ring: use kmem_cache_free() to free request

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> The change adding caching around the request allocated and freed for
> data messages changed a kmem_cache_free() to a kfree(), which isn't
> correct as the request came from slab in the first place. Fix that up
> and use the right freeing function if the cache is already at its limit.
>
> Fixes: 50cf5f3842af ("io_uring/msg_ring: add an alloc cache for io_kiocb entries")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Fwiw, kfree works fine for kmem_cache_alloc objects since 6.4, when SLOB
was removed.  Either way, it doesn't harm.

> ---
>  io_uring/msg_ring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
> index c2171495098b..29fa9285a33d 100644
> --- a/io_uring/msg_ring.c
> +++ b/io_uring/msg_ring.c
> @@ -82,7 +82,7 @@ static void io_msg_tw_complete(struct io_kiocb *req, struct io_tw_state *ts)
>  		spin_unlock(&ctx->msg_lock);
>  	}
>  	if (req)
> -		kfree(req);
> +		kmem_cache_free(req_cachep, req);
>  	percpu_ref_put(&ctx->refs);
>  }

-- 
Gabriel Krisman Bertazi

Re: [PATCH 2/2] io_uring/msg_ring: use kmem_cache_free() to free request

[Jens Axboe]

On 7/2/24 9:17 AM, Gabriel Krisman Bertazi wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> The change adding caching around the request allocated and freed for
>> data messages changed a kmem_cache_free() to a kfree(), which isn't
>> correct as the request came from slab in the first place. Fix that up
>> and use the right freeing function if the cache is already at its limit.
>>
>> Fixes: 50cf5f3842af ("io_uring/msg_ring: add an alloc cache for io_kiocb entries")
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> Fwiw, kfree works fine for kmem_cache_alloc objects since 6.4, when SLOB
> was removed.  Either way, it doesn't harm.

Right, it's more for consistency sake, it's not fixing a real bug.

-- 
Jens Axboe