Re: [GIT PULL] io_uring updates for 6.5

[Linus Torvalds <torvalds@linux-foundation.org>]

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.

I don't know who came up with this crazy pattern, but it must die.
"-1U" is garbage. Yes, it means "all bits set in an 'unsigned int'",
so it does have real semantics, but dammit, those semantics are very
seldom the ones you want.

They most *definitely* aren't the ones you want if you then mix things
with a signed type.

And yes, greping for this I found some truly disgusting things elsewhere too:

mm/zsmalloc.c:
        set_freeobj(zspage, (unsigned int)(-1UL));

net/core/rtnetlink.c:
        filters->mask[i] = -1U;

both of which are impressively confused. There's sadly a number of
other cases too.

That zsmalloc.c case is impressively crazy. First you use the wrong
type, then you use the wrong operation, and then you use a cast to fix
it all up. Absolutely none of which makes any sense, and it should
just have used "-1".

The rtnetlink case is also impressive, since mask[] isn't even an
array of 'unsigned int', but a 'u32'. They may be the same thing in
the end, but it's a sign of true confusion to think that "-1u" is
somehow fine.

Again, if you want to assign "all bits set" to a random unsigned type,
just use "-1". Sign extension is _literally_ your friend, the whole
world is 2's complement, and it's the only reason "-1" works in the
first place.

And if what you want is a particular unsigned type with all bits set
(say, unsigned long), I suggest you use "~0ul" to indicate that.

Because if you don't want to rely on sign extension, just use the
actual bit operation, for chrissake!

Ok, that was a rant about code that happens to work, but that is crap
regardless.

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

Writing illogical code, and then relying (probably without realizing
it) on some of the stranger parts of the implicit integer type
conversions in C to make that code work, that is just not good.

           Linus