linux-kernel - Re: [RFC PATCH v2 04/10] lib: vdso: get pointer to vdso data from the arch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D2614EC4-5B80-4846-994D-22730ACD44A1@amacapital.net>
Date:   Tue, 24 Dec 2019 20:15:11 +0800
From:   Andy Lutomirski <luto@...capital.net>
To:     christophe leroy <christophe.leroy@....fr>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Arnd Bergmann <arnd@...db.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "open list:MIPS" <linux-mips@...r.kernel.org>,
        X86 ML <x86@...nel.org>
Subject: Re: [RFC PATCH v2 04/10] lib: vdso: get pointer to vdso data from the arch



> On Dec 24, 2019, at 7:53 PM, christophe leroy <christophe.leroy@....fr> wrote:
> 
> 
> 
>> Le 24/12/2019 à 03:27, Andy Lutomirski a écrit :
>>> On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy
>>> <christophe.leroy@....fr> wrote:
>>> 
>>> On powerpc, __arch_get_vdso_data() clobbers the link register,
>>> requiring the caller to set a stack frame in order to save it.
>>> 
>>> As the parent function already has to set a stack frame and save
>>> the link register to call the C vdso function, retriving the
>>> vdso data pointer there is lighter.
>> I'm confused.  Can't you inline __arch_get_vdso_data()?  Or is the
>> issue that you can't retrieve the program counter on power without
>> clobbering the link register?
> 
> Yes it can be inlined (I did it in V1 https://patchwork.ozlabs.org/patch/1180571/), but you can't do it without clobbering the link register, because the only way to get the program counter is to do to as if you were calling another function but you call to the address which just follows where you are, so that it sets LR which the simulated return address which corresponds to the address following the branch.
> 
> static __always_inline
> const struct vdso_data *__arch_get_vdso_data(void)
> {
>    void *ptr;
> 
>    asm volatile(
>        "    bcl    20, 31, .+4;\n"
>        "    mflr    %0;\n"
>        "    addi    %0, %0, __kernel_datapage_offset - (.-4);\n"
>        : "=b"(ptr) : : "lr");
> 
>    return ptr + *(unsigned long *)ptr;
> }
> 
>> I would imagine that this patch generates worse code on any
>> architecture with PC-relative addressing modes (which includes at
>> least x86_64, and I would guess includes most modern architectures).
> 
> Why ? Powerpc is also using PC-relative addressing for all calls but indirect calls.

I mean PC-relative access for data.  The data page is at a constant, known offset from the vDSO text.

I haven’t checked how much x86_64 benefits from this, but at least the non-array fields ought to be accessible with a PC-relative access.

It should be possible to refactor a little bit so that the compiler can still see what’s going on.  Maybe your patch actually does this. I’d want to look at the assembly.  This also might not matter much on x86_64 in particular, since x86_64 can convert a PC-relative address to an absolute address with a single instruction with no clobbers.

Does power have PC-relative data access?  If so, I wonder if the code can be arranged so that even the array accesses don’t require computing an absolute address at any point.