linux-kernel - Re: [PATCH 2/3] crypto: exynos

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]

Date:   Tue, 5 Dec 2017 18:53:31 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Łukasz Stelmach <l.stelmach@...sung.com>
Cc:     Stephan Mueller <smueller@...onox.de>, robh+dt@...nel.org,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Kukjin Kim <kgene@...nel.org>, linux-crypto@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
        m.szyprowski@...sung.com, b.zolnierkie@...sung.com
Subject: Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG

On Tue, Dec 05, 2017 at 05:43:10PM +0100, Łukasz Stelmach wrote:
> It was <2017-12-05 wto 14:54>, when Stephan Mueller wrote:
> > Am Dienstag, 5. Dezember 2017, 13:35:57 CET schrieb Łukasz Stelmach:
> >
> > Hi Łukasz,
> >
> >> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
> >> to retrieve generated numbers from the registers of PRNG.
> >> 
> >> Remove unnecessary invocation of cpu_relax().
> >> 
> >> Signed-off-by: Łukasz Stelmach <l.stelmach@...sung.com>
> >> ---
> >>  drivers/crypto/exynos-rng.c | 36 +++++-------------------------------
> >>  1 file changed, 5 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> >> index 894ef93ef5ec..002e9d2a83cc 100644
> >> --- a/drivers/crypto/exynos-rng.c
> >> +++ b/drivers/crypto/exynos-rng.c
> 
> [...]
> 
> >> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
> >> *rng, {
> >>  	int retry = EXYNOS_RNG_WAIT_RETRIES;
> >> 
> >> +	*read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
> >> +
> >>  	if (rng->type == EXYNOS_PRNG_TYPE4) {
> >>  		exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
> >>  				  EXYNOS_RNG_CONTROL);
> >> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
> >> *rng, }
> >> 
> >>  	while (!(exynos_rng_readl(rng,
> >> -			EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && --retry)
> >> -		cpu_relax();
> >> +			EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) &&
> >> +	       --retry);
> SM>
> SM> Is this related to the patch?
> 
> KK> It looks like unrelated change so split it into separate commit with
> KK> explanation why you are changing the common busy-loop pattern.
> KK> exynos_rng_readl() uses relaxed versions of readl() so I would expect
> KK> here cpu_relax().
> 
> Yes. As far as I can tell this gives the major part of the performance
> improvement brought by this patch.

In that case definitely split and explain... what and why you want to
achieve here.

> 
> The busy loop is not very busy. Every time I checked the loop (w/o
> cpu_relax()) was executed twice (retry was 98) and the operation was
> reliable. I don't see why do we need a memory barrier here. On the other
> hand, I am not sure the whole exynos_rng_get_random() shouldn't be ran
> under a mutex or a spinlock (I don't see anything like this in the upper
> layers of the crypto framework).
> 
> The *_relaxed() I/O operations do not enforce memory

The cpu_relax() is a common pattern for busy-loop. If you want to break
this pattern - please explain why only this part of kernel should not
follow it (and rest of kernel should).

The other part - this code is already using relaxed versions which might
get you into difficult to debug issues. You mentioned that loop works
reliable after removing the cpu_relax... yeah, it might for 99.999% but
that's not the argument. I remember few emails from Arnd Bergmann
mentioning explicitly to avoid using relaxed versions "just because",
unless it is necessary or really understood.

The code first writes to control register, then checks for status so you
should have these operations strictly ordered. Therefore I think
cpu_relax() should not be removed.

Best regards,
Krzysztof