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

[Ming Lei <ming.lei@redhat.com>]

On Wed, Aug 20, 2025 at 07:11:52AM -0600, Jens Axboe wrote:
> 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.

OK, I will send V4 with documenting "commit the buffer upfront" usage.

> 
> 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.

Thanks for the clarification!


Thanks,
Ming