linux-kernel - Re: [PATCH v2 3/3] powerpc: Fix update form addressing in inline assembly

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: <20201020121610.GZ2672@gate.crashing.org>
Date:   Tue, 20 Oct 2020 07:16:10 -0500
From:   Segher Boessenkool <segher@...nel.crashing.org>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        mathieu.desnoyers@...icios.com, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] powerpc: Fix update form addressing in inline assembly

Hi!

On Tue, Oct 20, 2020 at 07:40:09AM +0000, Christophe Leroy wrote:
> In several places, inline assembly uses the "%Un" modifier
> to enable the use of instruction with update form addressing,
> but the associated "<>" constraint is missing.
> 
> As mentioned in previous patch, this fails with gcc 4.9, so
> "<>" can't be used directly.
> 
> Use UPD_CONSTR macro everywhere %Un modifier is used.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>

Oh well, it will be easy enough to remove this wart later, so

Reviewed-by: Segher Boessenkool <segher@...nel.crashing.org>

> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
> @@ -525,7 +525,7 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>  		stw%U0%X0 %2,%0\n\
>  		eieio\n\
>  		stw%U1%X1 %L2,%1"
> -	: "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
> +	: "=m"UPD_CONSTR (*ptep), "=m"UPD_CONSTR (*((unsigned char *)ptep+4))
>  	: "r" (pte) : "memory");

Here it would pre-increment ptep+4.  That can never be something useful
afaics?  The order the two operands are (either or not) pre-modified in
the asm is not specified (GCC does not parse the asm template, by
design), so I fail to see how this could ever work.

> --- a/arch/powerpc/include/asm/nohash/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
> @@ -200,7 +200,7 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>  			stw%U0%X0 %2,%0\n\
>  			eieio\n\
>  			stw%U1%X1 %L2,%1"
> -		: "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
> +		: "=m"UPD_CONSTR (*ptep), "=m"UPD_CONSTR (*((unsigned char *)ptep+4))
>  		: "r" (pte) : "memory");

Same here.

The rest looks fine.


Segher