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

[Jens Axboe <axboe@kernel.dk>]

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
borken anyway and needs fixing. Getting it wrong could obviously
reintroduce potential issues with data getting lost, however.

It's somewhat annoying to need to copy this upfront for the fairly rare
case of needing it.

-- 
Jens Axboe