Re: [GIT PULL] io_uring updates for 6.5

[Jens Axboe <axboe@kernel.dk>]

On 6/26/23 1:40?PM, Linus Torvalds wrote:
> On Sun, 25 Jun 2023 at 19:39, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Will throw a minor conflict in io_uring/net.c due to the late fixes in
>> mainline, you'll want to keep the kmsg->msg.msg_inq = -1U; assignment
>> there.
> 
> Can you please share some of those drugs you are on?
> 
> Or, better yet, admit you have a problem, and flush those things down
> the toilet.
> 
> That
> 
>         kmsg->msg.msg_inq = -1U;
> 
> pattern is complete insanity.
> 
> It's truly completely insane, because:
> 
>  (a) the whole concept of "-1U" is broken garbage
> 
>  (b) msg_inq is an 'int', not "unsigned int"
> 
> I want to note that assigning "-1" to an unsigned variable is fine,
> and makes perfect sense. "-1" is signed, so if the unsigned variable
> is larger, then the sign extension means that assigning "-1" is the
> same as setting all bits. Look, no need to worry about the size of the
> end result, it always JustWorks(tm).
> 
> Ergo: -1 is fine - regardless of whether the end result is signed or unsigned.
> 
> But doing the same with "-1U" is *dangerous". Because "-1U" is an
> unsigned int, if you assign it to some larger entity, you basically
> get a random end result that depends on the size of 'int' and the size
> of the destination.
> 
> So any time you see something like "-1U", you should go "those are
> some bad bad drugs".
> 
> It doesn't just look odd - it's actively *WRONG*. It's *STUPID*. And
> it's *DANGEROUS*.
> 
> Lookie here: the same completely bogus pattern exists in some testing too:
> 
> io_uring/net.c:
> 
>         if (msg->msg_inq && msg->msg_inq != -1U)
> 
> and it all happens to work, but it happens to work for all the wrong
> reasons. Because  -1U is unsigned, the "msg->msg_inq != -1U"
> comparison is done as "unsigned int", and msg->msg_inq (which contains
> a *signed* -1) becomes 4294967295, and it all matches.
> 
> But while it happens to work, it's entirely illogical and makes no sense at all.
> 
> And if you ever end up in the situation that something is extended to
> 'long', it will break horribly on 64-bit architectures, since now
> "-1U" will literally be 4294967295, while "msg->msg_inq" will become
> -1l, and the two are *not* the same.

Oops yes, I can get that cleaned up. It doesn't really matter in here,
as all we need to know is "did someone assign this value or not", to
avoid relying on msg_inq _always_ returning something when we ask for
it. The actual value of it is way less interesting. Worst case scenario
here is an extra round trip if the available data just happened to
match, which seems basically impossible.

But do agree that it's confusing and bogus, will just change it to -1
in assignment and test that should be it.

> I'm doing this pull, but I want this idiocy fixed.

Thanks! I'll address it in the pre-rc1 pull.

-- 
Jens Axboe