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.
--
Jens Axboe
