Jens Axboe <axboe@xxxxxxxxx> writes:
> On 2/8/24 1:08 PM, Gabriel Krisman Bertazi wrote:
>> Jens Axboe <axboe@xxxxxxxxx> writes:
>>
>>
>>> - TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, flags 0x%x, %s queue, work %p",
>>> + TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, flags 0x%lx, %s queue, work %p",
>>> __entry->ctx, __entry->req, __entry->user_data,
>>> - __get_str(op_str),
>>> - __entry->flags, __entry->rw ? "hashed" : "normal", __entry->work)
>>> + __get_str(op_str), (long) __entry->flags,
>>
>> Hi Jens,
>>
>> Minor, but on 32-bit kernel the cast is wrong since
>> sizeof(long)==4. Afaik, io_uring still builds on 32-bit archs.
>>
>> If you use (unsigned long long), it will be 64 bit anywhere.
>
> Ah thanks, I'll make that edit.
>
>>> @@ -2171,7 +2171,8 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>> /* req is partially pre-initialised, see io_preinit_req() */
>>> req->opcode = opcode = READ_ONCE(sqe->opcode);
>>> /* same numerical values with corresponding REQ_F_*, safe to copy */
>>> - req->flags = sqe_flags = READ_ONCE(sqe->flags);
>>> + sqe_flags = READ_ONCE(sqe->flags);
>>
>> Did you consider that READ_ONCE won't protect from load tearing the
>> userspace value in 32-bit architectures? It builds silently, though, and
>> I suspect it is mostly fine in the current code, but might become a bug
>> eventually.
>
> sqe->flags is just a byte, so no tearing is possible here. The only
> thing that changed type is req->flags.
You're right, of course. I confused the source of the read with struct
io_kiocb.
--
Gabriel Krisman Bertazi
