Re: [PATCH] powerpc/bug: Cast to unsigned long before passing to inline asm

[Michael Ellerman <mpe@ellerman.id.au>]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Tue, Aug 31, 2021 at 11:27:20PM +1000, Michael Ellerman wrote:
>> Nathan filed an LLVM bug [2], in which Eli Friedman explained that "if
>> you pass a value of a type that's narrower than a register to an inline
>> asm, the high bits are undefined". In this case we are passing a bool
>> to the inline asm, which is a single bit value, and so the compiler
>> thinks it can leave the high bits of r30 unmasked, because only the
>> value of bit 0 matters.
>> 
>> Because the inline asm compares the full width of the register (32 or
>> 64-bit) we need to ensure the value passed to the inline asm has all
>> bits defined. So fix it by casting to long.
>> 
>> We also already cast to long for the similar case in BUG_ENTRY(), which
>> it turns out was added to fix a similar bug in 2005 in commit
>> 32818c2eb6b8 ("[PATCH] ppc64: Fix issue with gcc 4.0 compiled kernels").
>
> That points to <https://gcc.gnu.org/PR23422>, which shows the correct
> explanation.

That's a pity because I don't understand that explanation ^_^

Why does sign extension matter when we're comparing against zero?

> The code as it was did **not** pass a bool.  It perhaps passed an int
> (so many macros in play, it is hard to tell for sure, but it is int or
> long int, perhaps unsigned (which does not change anything here).

I don't understand that. It definitely is passing a bool at the source
level. Are you saying it's getting promoted somehow?

It expands to:

      asm goto(
              "1:   "
              "tdnei"
              "
              " " % 4,
              0 " "\n " ".section __ex_table,\"a\";"
                                              " "
                                              ".balign 4;"
                                              " "
                                              ".long (1b) - . ;"
                                              " "
                                              ".long (%l[__label_warn_on]) - . ;"
                                              " "
                                              ".previous"
                                              " "
                                              ".section __bug_table,\"aw\"\n"
                                              "2:\t.4byte 1b - 2b, %0 - 2b\n"
                                              "\t.short %1, %2\n"
                                              ".org 2b+%3\n"
                                              ".previous\n"
              :
              : "i"("lib/klist.c"), "i"(62),
                "i"((1 << 0) | ((9) << 8)),
                "i"(sizeof(struct bug_entry)),
                "r"(knode_dead(knode))
                ^^^^^^^^^^^^^^^^^^^^^
              :
              : __label_warn_on);

And knode_dead() returns bool:

  static bool knode_dead(struct klist_node *knode)
  {
  	return (unsigned long)knode->n_klist & KNODE_DEAD;
  }

So in my book that means the type there is bool. But I'm not a compiler
guy so I guessing I'm missing something.

> But td wants a 64-bit register, not a 32-bit one (which are the only two
> possibilities for the "r" constraint on PowerPC).  The cast to "long" is
> fine for powerpc64, but it has nothing to do with "narrower than a
> register".

If it's 32-bit vs 64-bit, and the clang explanation is correct, then
we'd expect the low 32-bits of the value passed to the asm to have the
correct value, ie. have been masked with KNODE_DEAD.

> If this is not the correct explanation for LLVM, it needs to solve some
> other bug.

OK. I just need to get this fixed in the kernel, so I'll do a new
version with a changelog that is ~= "shrug not sure what's going on" and
merge that. Then we can argue about what is really going on later :)

cheeers