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

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

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.

> 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's somewhat annoying to need to copy this upfront for the fairly rare
> case of needing it.
> 

-- 
Pavel Begunkov