Re: [PATCH 1/2] mips: vdso: fix 'jalr t9' crash in vdso code

[Paul Burton <paulburton@kernel.org>]

Hi Victor,

On Mon, Feb 10, 2020 at 09:12:59PM +0000, Victor Kamensky (kamensky) wrote:
> On Mon, Feb 10, 2020 at 11:14:24AM -0800, Paul Burton wrote:
> > Which kernel version did you find this issue with? Could you check
> > whether you have commit bbcc5672b006 ("MIPS: Avoid VDSO ABI breakage due
> > to global register variable")? That was merged for v5.5-rc5.
> 
> Ah, I just spotted your detailed cover letter - my bad :)
> 
> Please try moving from v5.4 to v5.4.18 or higher, or just cherry-picking
> the commit I mentioned.
> 
> Double commit that you mentioned is present in 5.4.15 (since
> v5.4.9) that I use. It is 5b004a238460113276319536534928c58d95e599
> 
> [kamensky@kamensky-p53s linux]$ git tag --contains 5b004a238460113276319536534928c58d95e599
> v5.4.10
> v5.4.11
> v5.4.12
> v5.4.13
> v5.4.14
> v5.4.15
> v5.4.16
> v5.4.17
> v5.4.18
> v5.4.9

Right you are; I don't know how I picked up v5.4.18 as the version it
made it into.

OK, so you're using v5.4.15.

> You can check your tree: disassemble arch/mips/vdso/vgettimeofday-n32.o
> you would see 'jalr t9' calls, then disassemble arsm/mips/vdso/vdos-n32.so
> and check that those 'jalr t9' places got converted into 'bal' instructions,
> even though t9 set up instructions are there. This conversion happens by
> ld during relocation processing if code was compiled with options I propose
> to add explicitly. Walk through gcc and binutils places that do that are in my
> cover letter write up.
> 
> You can reproduce the issue by doing opposite of what I propose:
> disable -mrelax-pic-calls and -mexplicit-relocs defaults in your
> toolchain by either using negate variant of option, ie
> -mno-relax-pic-calls or -mno-explicit-relocs or both. Something
> like in diff below. I've tried it on mips-poky-linux-gcc toolchain
> that got correct defaults and worked for us before, after adding
> no variants it shows the same 'jalr t9' crash as with other yocto
> mips toolchains.

OK, I see what's happening here but I think focusing in on the "jalr
$t9" instructions as the explanation is misleading. Those aren't a
problem in & of themselves, but the fact that the GOT is being used is
problematic because we don't apply relocations for the VDSO. That,
combined with the fact that the VDSO is position-independent, means the
GOT can't possibly contain the right address for the function being
called & indeed contains zero.

I'm a little puzzled by how we got into this state of trying to use the
GOT without relocations though. Does your VDSO image contain
relocations? Could you try running "objdump -rR vdso-n32.so.dbg.raw"? We
have a check in the genvdso tool for reloc sections that should cause
the build to fail if they're present, and I'd expect the toolchain to be
emitting relocs if it's using the GOT. GOT with no relocs makes no
sense.

I also expect that reverting commit 4467f7ad7dbb ("MIPS: genvdso: Remove
GOT checks") would flag the problem & cause your build to fail. Perhaps
we should bring that check back & either special case lld or require its
users to have a very recent version of it.

It would be good to submit a v2 of this patch with the commit message
updated to explain that some toolchains attempt to use the GOT without
these flags being specified, and even better if you can figure out why
that happens.

Thanks,
    Paul