Re: [PATCH 1/6] io_uring: expand main struct io_kiocb flags to 64-bits

[Jens Axboe <axboe@kernel.dk>]

On 2/8/24 1:08 PM, Gabriel Krisman Bertazi wrote:
> Jens Axboe <axboe@kernel.dk> 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