Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <alpine.DEB.2.22.394.2006181751270.9276@xps-7390>
 <20200619065007.GA3041@lst.de> <alpine.DEB.2.22.394.2006182351090.9276@xps-7390>
 <20200619074233.GA3723@lst.de> <alpine.DEB.2.22.394.2006200640370.2845@xps-7390>
In-Reply-To: <alpine.DEB.2.22.394.2006200640370.2845@xps-7390>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat, 20 Jun 2020 09:49:47 -0700
Message-ID: <CAHk-=whj7YBvNT3FPHc8oUqwRhjbRkJESnUx6bbpA5ys6W9ujw@mail.gmail.com>
Subject: Re: Commit 25f12ae45fc1 ("maccess: rename probe_kernel_address to
 get_kernel_nofault") causing several OOPSes
To:     "Kenneth R. Crudup" <kenny@panix.com>
Cc:     Christoph Hellwig <hch@lst.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Jun 20, 2020 at 6:46 AM Kenneth R. Crudup <kenny@panix.com> 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