Re: Commit 25f12ae45fc1 ("maccess: rename probe_kernel_address to get_kernel_nofault") causing several OOPSes

[Linus Torvalds]

On Sat, Jun 20, 2020 at 6:46 AM Kenneth R. Crudup <kenny@xxxxxxxxx> wrote:
>
> So, be totally surprised :) I've just booted with "maccess: rename
> probe_kernel_address to get_kernel_nofault" intact and your probe_roms.c
> patch with no issues.
>
> (Perhaps there's some sort of compiler optimization going on?)

Hmm.

Very strange. I was a tiny bit worried about that part of the patch,
because I also changed the types (from "unsigned char *" to "void *"),
but pointer arithmetic in "unsigned char *" and "void *" is the same,
and Christoph's partial revert patch doesn't even revert that part.

But I really don't see what Christoph's revert would really even
change It switches the order of the arguments back..

It does re-introduce a bug in that macro that I fixed. This macro is
buggy garbage:

+#define probe_kernel_address(addr, retval)             \
+       copy_from_kernel_nofault(&retval, addr, sizeof(retval))

in case 'retval' is a complex expression, becasue of possibly changing
the C order of operations. So it needs to be "&(retval)" in the macro
body.

But that is never the case for 'retval'. For 'addr', yes, but 'addr'
is only used simply (and copy_from_kernel_nofault() isn't a macro).

I'm staring at that opatch and not seeing how it could _possibly_ make
any difference in code generation.

Which is the obvious next step: would you mind compiling that file
with and without the patch and sending me the two object files?

              Linus