Re: [PATCH 1/4] io_uring/uring_cmd: get rid of using req->imu

[Jens Axboe <axboe@kernel.dk>]

On 10/21/24 8:43 PM, Ming Lei wrote:
> On Mon, Oct 21, 2024 at 08:03:20PM -0600, Jens Axboe wrote:
>> It's pretty pointless to use io_kiocb as intermediate storage for this,
>> so split the validity check and the actual usage. The resource node is
>> assigned upfront at prep time, to prevent it from going away. The actual
>> import is never called with the ctx->uring_lock held, so grab it for
>> the import.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  io_uring/uring_cmd.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>> index 39c3c816ec78..313e2a389174 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -211,11 +211,15 @@ int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>  		struct io_ring_ctx *ctx = req->ctx;
>>  		u16 index;
>>  
>> -		req->buf_index = READ_ONCE(sqe->buf_index);
>> -		if (unlikely(req->buf_index >= ctx->nr_user_bufs))
>> +		index = READ_ONCE(sqe->buf_index);
>> +		if (unlikely(index >= ctx->nr_user_bufs))
>>  			return -EFAULT;
>> -		index = array_index_nospec(req->buf_index, ctx->nr_user_bufs);
>> -		req->imu = ctx->user_bufs[index];
>> +		req->buf_index = array_index_nospec(index, ctx->nr_user_bufs);
>> +		/*
>> +		 * Pi node upfront, prior to io_uring_cmd_import_fixed()
>> +		 * being called. This prevents destruction of the mapped buffer
>> +		 * we'll need at actual import time.
>> +		 */
>>  		io_req_set_rsrc_node(req, ctx, 0);
>>  	}
>>  	ioucmd->cmd_op = READ_ONCE(sqe->cmd_op);
>> @@ -272,8 +276,16 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
>>  			      struct iov_iter *iter, void *ioucmd)
>>  {
>>  	struct io_kiocb *req = cmd_to_io_kiocb(ioucmd);
>> +	struct io_ring_ctx *ctx = req->ctx;
>> +	struct io_mapped_ubuf *imu;
>> +	int ret;
>>  
>> -	return io_import_fixed(rw, iter, req->imu, ubuf, len);
>> +	mutex_lock(&ctx->uring_lock);
>> +	imu = ctx->user_bufs[req->buf_index];
>> +	ret = io_import_fixed(rw, iter, imu, ubuf, len);
>> +	mutex_unlock(&ctx->uring_lock);
> 
> io_uring_cmd_import_fixed is called in nvme ->issue(), and ->uring_lock
> may be held already.

Gah indeed, in fact it always should be, unless it's forcefully punted
to io-wq. I'll sort that out, thanks. And looks like we have zero tests
for uring_cmd + fixed buffers :-(

-- 
Jens Axboe