Re: [PATCH] io_uring/uring_cmd: be smarter about SQE copying

[Jens Axboe <axboe@kernel.dk>]

On 6/2/25 6:10 AM, Pavel Begunkov wrote:
> On 6/2/25 12:55, Jens Axboe wrote:
>> On 6/2/25 4:24 AM, Pavel Begunkov wrote:
>>> On 5/31/25 21:52, Jens Axboe wrote:
>>>> uring_cmd currently copies the SQE unconditionally, which was introduced
>>> ...>       /*
>>>> -     * Unconditionally cache the SQE for now - this is only needed for
>>>> -     * requests that go async, but prep handlers must ensure that any
>>>> -     * sqe data is stable beyond prep. Since uring_cmd is special in
>>>> -     * that it doesn't read in per-op data, play it safe and ensure that
>>>> -     * any SQE data is stable beyond prep. This can later get relaxed.
>>>> +     * Copy SQE now, if we know we're going async. Drain will set
>>>> +     * FORCE_ASYNC, and assume links may cause it to go async. If not,
>>>> +     * copy is deferred until issue time, if the request doesn't issue
>>>> +     * or queue inline.
>>>>         */
>>>> -    memcpy(ac->sqes, sqe, uring_sqe_size(req->ctx));
>>>> -    ioucmd->sqe = ac->sqes;
>>>> +    ioucmd->sqe = sqe;
>>>> +    if (req->flags & (REQ_F_FORCE_ASYNC| REQ_F_LINK | REQ_F_HARDLINK) ||
>>>> +        ctx->submit_state.link.head)
>>>> +        io_uring_sqe_copy(req, ioucmd);
>>>> +
>>>
>>> It'd be great if we can't crash the kernel (or do sth more nefarious with
>>> that), and I'm 95% sure it's possible. The culprit is violation of
>>> layering by poking into io_uring core bits that opcodes should not know
>>> about, the flimsiness of attempts to infer the core io_uring behaviour
>>> from opcode handlers, and leaving a potentially stale ->sqe pointer.
>>
>> Sure, it's not a pretty solution. At all. Might be worth just having a
>> handler for this so that the core can call it, rather than have
>> uring_cmd attempt to cover all the cases here. That's the most offensive
>> part to me, the ->sqe pointer I'm not too worried about, in terms of
>> anything nefarious. If that's a problem, the the uring_cmd prep side is
> 
> If a cmd accesses it after going async while it points to the SQ,
> a lot of interesting things can happen, you just need to swap the
> SQ in between, so that the pointer ends up dangling, and there is a
> convenient feature for that.

Not sure why you aren't being explicit and just saying "resizing", but
yes agree that would be a problematic spot. Above comment was just about
swapping between two separately valid SQEs, not dead SQEs.

>> borken anyway and needs fixing. Getting it wrong could obviously
>> reintroduce potential issues with data getting lost, however.
> 
> It's flimsy as it depends on the file impl, especially with
> -EIOCBQUEUED, but yeah, your choice.

It is... Just consider it an RFC - I think we should do something about
it, productive suggestions more than welcome.

-- 
Jens Axboe