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

["Victor Kamensky (kamensky)" <kamensky@cisco.com>]

Hi Paul,

Please see inline look for 'kamensky2>'

________________________________________
From: linux-mips-owner@vger.kernel.org <linux-mips-owner@vger.kernel.org> on behalf of Paul Burton <paulburton@kernel.org>
Sent: Monday, February 10, 2020 2:52 PM
To: Victor Kamensky (kamensky)
Cc: linux-mips@vger.kernel.org; Ralf Baechle; James Hogan; Vincenzo Frascino; bruce.ashfield@gmail.com; richard.purdie@linuxfoundation.org
Subject: Re: [PATCH 1/2] mips: vdso: fix 'jalr t9' crash in vdso code

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.

kamensky2> Created GOT entry and corresponding 'jalr t9' are for
kamensky2> static function call. I've tried diff where I add __always_inline
kamensky2> for do_hres and few other places in lib/vdso/gettimeofday.c.
kamensky2> When everything inlined I was able to get rid of 'jalr t9'
kamensky3> but resulting patch is ugly and size of vdso grew up to 40%

I'm a little puzzled by how we got into this state of trying to use the
GOT without relocations though.

kamensky2> Yes, I agree it is a bit perplexing.

Does your VDSO image contain relocations? 

kamensky2> No, it does not. It would fail cmd_vdso_check that looks for
kamensky2> the same. Double checked manually - no 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.

kamensky2> I did some digging - looked at glibc sysdeps/mips/dl-machine.h
kamensky2> line 209

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/mips/dl-machine.h;h=06021ea9abcfcc598b2204ce37d643a4809735da;hb=HEAD#l209

kamensky2> new 3rd entry that got created in our case to call static
kamensky2> functions is so called local got entry and it is supposed to be
kamensky2> filled with 'start of DSO' value by run-time linker without any
kamensky2> run-time relocations present.

kamensky2> Here is value of MIPS_LOCAL_GOTNO in dynamic section:

[root@svarog-cisco-com vdso]# mips64-poky-linux-gnun32-readelf -S vdso-n32.so | grep .got
  [11] .got              PROGBITS        000009c0 0009c0 00000c 04 WAp  0   0 16
[root@svarog-cisco-com vdso]# mips64-poky-linux-gnun32-readelf --dynamic vdso-n32.so | grep MIPS_LOCAL_GOTNO
 0x7000000a (MIPS_LOCAL_GOTNO)           3

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.

kamensky2> I've tried to revert it - it starts failing even in cases where it
kamensky2> used to work before: because it detects additional got entry -
kamensky2> pretty much original issue that drove 4467f7ad7dbb commit.
kamesnky2> We could improve the check with taking in account LOCAL_GOTNO.
kamensky2> But I don't see much sense in it: since for global got entries code
kamensky2> dynamic relocations will be present and it would be covered by
kamensky2> cmd_vdso_check ('objdump -R' check from lib/vdso/Makefile).
kamensky2> And when we allow local GOT entries we still would rely on
kamensky2> -mrelax-pic-calls magic of converting those 'jalr t9' into 'bal'
kamensky2> instructions.

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.

kamensky2> I will add few sentences to my commit message explaining why
kamensky2> we don't have dynamic relocations for static functions case.
kamensky2> And as you asked in follow up message I will add those options
kamensky2> unconditionally.
kamensky2> I believe my second patch: build time check for 'jalr t9' in code
kamensky2> should still be useful, since it verifies that conversion to 'bal' driven
kamensky2> by -mrelax-pic-calls does happen.

kamensky2> Please let me know if you have any additional feedback before
kamensky2> I post v2 set of patches.

Thanks,
Victor

Thanks,
    Paul