Re: [PATCH 02/16] io_uring: cqe init hardening

[Jens Axboe <axboe@kernel.dk>]

On 8/24/23 10:28 AM, Pavel Begunkov wrote:
> On 8/19/23 16:03, Jens Axboe wrote:
>> On 8/15/23 11:31 AM, Pavel Begunkov wrote:
>>> io_kiocb::cqe stores the completion info which we'll memcpy to
>>> userspace, and we rely on callbacks and other later steps to populate
>>> it with right values. We have never had problems with that, but it would
>>> still be safer to zero it on allocation.
>>>
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> ---
>>>   io_uring/io_uring.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>> index e189158ebbdd..4d27655be3a6 100644
>>> --- a/io_uring/io_uring.c
>>> +++ b/io_uring/io_uring.c
>>> @@ -1056,7 +1056,7 @@ static void io_preinit_req(struct io_kiocb *req, struct io_ring_ctx *ctx)
>>>       req->link = NULL;
>>>       req->async_data = NULL;
>>>       /* not necessary, but safer to zero */
>>> -    req->cqe.res = 0;
>>> +    memset(&req->cqe, 0, sizeof(req->cqe));
>>>   }
>>>     static void io_flush_cached_locked_reqs(struct io_ring_ctx *ctx,
>>
>> I think this is a good idea, but I wonder if we should open-clear it
>> instead. I've had cases in the past where that's more efficient than
>> calling memset.
> 
> I don't think it ever happens for 16 byte memcpy, and in either
> case it's a cache refill, quite a slow path. I believe memcpy is
> better here.

Yeah I think it's fine as-is - just checked here and either approach
yields the same.

-- 
Jens Axboe