Re: [PATCH 3/6] io_uring: add support for kernel registered bvecs

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/7/25 15:17, Keith Busch wrote:
> On Fri, Feb 07, 2025 at 02:08:23PM +0000, Pavel Begunkov wrote:
>> On 2/3/25 15:45, Keith Busch wrote:
>>>    		struct io_rsrc_node *node;
>>>    		u64 tag = 0;
>>> +		i = array_index_nospec(up->offset + done, ctx->buf_table.nr);
>>> +		node = io_rsrc_node_lookup(&ctx->buf_table, i);
>>> +		if (node && node->type != IORING_RSRC_BUFFER) {
>>
>> We might need to rethink how it's unregistered. The next patch
>> does it as a ublk commands, but what happens if it gets ejected
>> by someone else?  get_page might protect from kernel corruption
>> and here you try to forbid ejections, but there is io_rsrc_data_free()
>> and the io_uring ctx can die as well and it will have to drop it.
> 
> We prevent clearing an index through the typical user register update
> call. The expected way to clear for a well functioning program is
> through the kernel interfaces.

What I'm saying, it's a sanity check, but it doesn't prevent it
from happening from other paths, and I understand that you're
trying to cover for that.

> Other than that, there's nothing special about kernel buffers here. You
> can kill the ring or tear down registered buffer table, but that same
> scenario exists for user registered buffers. The only thing io_uring

For registered buffers the user can and will have to handle it, but in
case of this proposal the end ublk user wouldn't even know there is
an io_uring and registered buffers, so ultimately the ublk driver will
have to handle edge cases. And for ublk driver to be able to handle it
well even in case of ublk server failures, it'll need to be able to wait
until io_uring releases the buffer.

For example, the ublk server crashes, which closes io_uring => there
is no way to do unregister cmd anymore. IIUC, the ublk driver will
want to complete the block request returning an error, but if it's
done before io_uring releases the buffer, the end ublk user may
attempt to reuse the memory while io_uring is still concurrently
writing to / reading from it, which would be disastrous.

One thing I like about ublk unregister cmd though, is that you can
add some more control like reporting back a short IO, but I doubt we
can do it sanely without sending some sort of a notification back
to ublk. So, maybe it should be both, and in case of forced
unregistration ublk will consider it to be a failure. Another option
is to do it all through normal(ish) io_uring buffer unregisteration
path, but maybe enhanced with additional custom arguments. This way
we have only one path doing that.
  

> needs to ensure is that nothing gets corrupted. User registered buffers
> hold a pin on the user pages while the node is referenced. Kernel
> registered buffers hold a page reference while the node is referenced.
> Nothing special.
> 
>> And then you don't really have clear ownership rules. Does ublk
>> releases the block request and "returns ownership" over pages to
>> its user while io_uring is still dying and potenially have some
>> IO inflight against it?
>>
>> That's why I liked more the option to allow removing buffers from
>> the table as per usual io_uring api / rules instead of a separate
>> unregister ublk cmd.
> 
> ublk is the only entity that knows about the struct request that
> provides the bvec we want to use for zero-copy, so it has to be ublk
> that handles the registration. Moving the unregister outside of that
> breaks the symmetry and requires an indirect call.

cmd execution takes 2 indirect calls, not like there is a
difference here.

> 
>> And inside, when all node refs are dropped,
>> it'd call back to ublk. This way you have a single mechanism of
>> how buffers are dropped from io_uring perspective. Thoughts?
>>
>>> +			err = -EBUSY;
>>> +			break;
>>> +		}
>>> +

...

>> ...
>>>    			unsigned long seg_skip;
>>> diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h
>>> index abd0d5d42c3e1..d1d90d9cd2b43 100644
>>> --- a/io_uring/rsrc.h
>>> +++ b/io_uring/rsrc.h
>>> @@ -13,6 +13,7 @@
>>>    enum {
>>>    	IORING_RSRC_FILE		= 0,
>>>    	IORING_RSRC_BUFFER		= 1,
>>> +	IORING_RSRC_KBUF		= 2,
>>
>> The name "kbuf" is already used, to avoid confusion let's rename it.
>> Ming called it leased buffers before, I think it's a good name.
> 
> These are just fixed buffers, just like user space onces. The only
> difference is where the buffer comes from: kernel or userspace? I don't
> see what the term "lease" has to do with this.

In this particular case, there is a kernel component that expects
it back, that's the leasing part, but thinking about it more, you're
right, the interface can support workflows different from it as well.

I actually like kbuf, but again it's confusing because already used
for an entirely different thing. Maybe it's fine if it doesn't leak
outside of node types.

-- 
Pavel Begunkov