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:   Wed, 06 Dec 2017 12:32:50 +0100
From:   Łukasz Stelmach <l.stelmach@...sung.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>
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,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Bartłomiej Żołnierkiewicz 
        <b.zolnierkie@...sung.com>
Subject: Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG

It was <2017-12-05 wto 19:06>, when Krzysztof Kozlowski wrote:
> On Tue, Dec 5, 2017 at 6:53 PM, Krzysztof Kozlowski <krzk@...nel.org> wrote:
>> 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:
>>>>> 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);

[...]

>>> 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.
>
> ... or just convert it to readl_poll_timeout() because it makes code
> more readable, takes care of timeout and you do not have care about
> specific implementation (whether there should or should not be
> cpu_relax).

OK. This appears to perform reasonably.

	do {
		cpu_relax();
	} while (!(exynos_rng_readl(rng, EXYNOS_RNG_STATUS) &
		   EXYNOS_RNG_STATUS_RNG_DONE) && --retry);

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Download attachment "signature.asc" of type "application/pgp-signature" (473 bytes)