Re: [PATCH V3] io_uring: uring_cmd: add multishot support

[Jens Axboe <axboe@kernel.dk>]

On 8/20/25 5:08 AM, Ming Lei wrote:
> On Tue, Aug 19, 2025 at 10:00:36AM -0600, Jens Axboe wrote:
>> On 8/19/25 9:00 AM, Ming Lei wrote:
>>> @@ -251,6 +265,11 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>  	}
>>>  
>>>  	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>>> +	if (ioucmd->flags & IORING_URING_CMD_MULTISHOT) {
>>> +		if (ret >= 0)
>>> +			return IOU_ISSUE_SKIP_COMPLETE;
>>> +		io_kbuf_recycle(req, issue_flags);
>>> +	}
>>>  	if (ret == -EAGAIN) {
>>>  		ioucmd->flags |= IORING_URING_CMD_REISSUE;
>>>  		return ret;
>>
>> Final comment on this part... uring_cmd is unique in the sense that it'd
>> be the first potentially pollable file type that supports buffer
>> selection AND can return -EIOCBQUEUED. For non-pollable, the buffer
>> would get committed upfront. For pollable, we'd either finish and put it
>> within this same execution context, or we'd drop it entirely when
>> returning -EAGAIN.
>>
>> So what happens if we get -EIOCBQUEUED with a selected buffer from
>> provided buffer ring, and someome malicious unregisters and frees the
>> buffer ring before that request completes?
> 
> Looks one real trouble for IORING_URING_CMD_MULTISHOT.
> 
> For pollable multishot, ->issue() is run in submitter tw context, and done
> in `sync` style, so ctx->uring_lock protects the buffer list, and
> unregister can't happen. That should be one reason why polled multishot
> can't be run in io-wq context.
> 
> But now -EIOCBQUEUED is returned from ->issue(), we lose ->uring_lock's
> protection for req->buf_list, one idea could be adding referenced buffer
> list for failing unregister in case of any active consumer.
> 
> Do you have suggestions for this problem?

Just commit the buffer upfront, rather than grab it at issue time and
commit when you get the completion callback? Yes that will pin the
buffer for the duration of the IO, but that should not be an issue,
nobody else can use it anyway. Avoiding the pin for pollable files with
potentially infinite IO times (eg pipe that never gets written to, or
socket that never gets data) is a key concept for those kinds of
workloads, but for finite completion times or single use cases like
yours here, that doesn't really matter.

I've got a bit of a side project making the provided buffer selection a
bit more foolproof in the sense that it makes it explicit that the scope
of it is the issue context, but across executions. One current problem
is req->buf_list, which for provided buffer rings really is local scope,
yet it's in the io_kiocb. I'll be moving that somewhere else and out of
io_kiocb. Just a side note, because it's currently easy to get this
wrong even if you know what you are doing, as per your patch.

-- 
Jens Axboe