linux-kernel - Re: [PATCH] RISC-V: Break load reservations during switch

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] [day] [month] [year] [list]

Message-ID: <mhng-a0345343-89b4-43e0-94dd-fb4f2cc851e0@palmer-si-x1e>
Date:   Fri, 07 Jun 2019 15:12:42 -0700 (PDT)
From:   Palmer Dabbelt <palmer@...ive.com>
To:     schwab@...ux-m68k.org
CC:     Christoph Hellwig <hch@...radead.org>,
        linux-riscv@...ts.infradead.org,
        Paul Walmsley <paul.walmsley@...ive.com>, marco@...red.org,
        me@...losedp.com, joel@...g.id.au, linux-kernel@...r.kernel.org
Subject:     Re: [PATCH] RISC-V: Break load reservations during switch_to

On Thu, 06 Jun 2019 12:32:01 PDT (-0700), schwab@...ux-m68k.org wrote:
> On Jun 06 2019, Christoph Hellwig <hch@...radead.org> wrote:
>
>> On Wed, Jun 05, 2019 at 04:17:35PM -0700, Palmer Dabbelt wrote:
>>>  	REG_S ra,  TASK_THREAD_RA_RA(a3)
>>> +	/*
>>> +	 * The Linux ABI allows programs to depend on load reservations being
>>> +	 * broken on context switches, but the ISA doesn't require that the
>>> +	 * hardware ever breaks a load reservation.  The only way to break a
>>> +	 * load reservation is with a store conditional, so we emit one here.
>>> +	 * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we
>>> +	 * know this will always fail, but just to be on the safe side this
>>> +	 * writes the same value that was unconditionally written by the
>>> +	 * previous instruction.
>>> +	 */
>>> +#if (TASK_THREAD_RA_RA != 0)
>>
>> I don't think this check works as intended.  TASK_THREAD_RA_RA is a
>> parameterized macro,
>
> Is it?  Just because it is used before an open paren doesn't mean that
> the macro takes a parameter.

Yes, you're right -- the parens there aren't a macro parameter, they're the
assembly syntax for constant-offset loads.  I guess I'd just assumed Christoph
was referring to some magic in how asm-offsets gets generated, as I've never
actually looked inside that stuff.  I went ahead and just tested this

    $ git diff | cat
    diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
    index 578bb5efc085..e3f06495dbf8 100644
    --- a/arch/riscv/kernel/asm-offsets.c
    +++ b/arch/riscv/kernel/asm-offsets.c
    @@ -125,6 +125,7 @@ void asm_offsets(void)
            DEFINE(TASK_THREAD_RA_RA,
                      offsetof(struct task_struct, thread.ra)
                    - offsetof(struct task_struct, thread.ra)
    +               + 1
            );
            DEFINE(TASK_THREAD_SP_RA,
                      offsetof(struct task_struct, thread.sp)

and it causes the expected failure

     $ make.cross ARCH=riscv -j1
    make CROSS_COMPILE=/home/palmer/.local/opt/gcc-8.1.0-nolibc/riscv64-linux/bin/riscv64-linux- ARCH=riscv -j1
      CALL    scripts/checksyscalls.sh
      CALL    scripts/atomic/check-atomics.sh
      CHK     include/generated/compile.h
      AS      arch/riscv/kernel/entry.o
    arch/riscv/kernel/entry.S:344:3: error: #error "The offset between ra and ra is non-zero"
     # error "The offset between ra and ra is non-zero"
       ^~~~~
    make[1]: *** [scripts/Makefile.build:369: arch/riscv/kernel/entry.o] Error 1
    make: *** [Makefile:1071: arch/riscv/kernel] Error 2

so I'm going to leave it alone.  I'll submit a v2 with a better error message
and a cleaner sc.w/sc.d.