Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] crypto: crypto4xx - Replace bitwise OR with logical OR in
 crypto4xx_build_pd
To:     Nathan Chancellor <natechancellor@gmail.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>
Cc:     Nick Desaulniers <ndesaulniers@google.com>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
        clang-built-linux@googlegroups.com
References: <20201112200702.1664905-1-natechancellor@gmail.com>
From:   Christian Lamparter <chunkeey@gmail.com>
Message-ID: <1f5e8575-ab7e-59ea-6be7-0340df4d31c5@gmail.com>
Date:   Thu, 12 Nov 2020 22:21:35 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.2
MIME-Version: 1.0
In-Reply-To: <20201112200702.1664905-1-natechancellor@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello,

On 12/11/2020 21:07, Nathan Chancellor wrote:
> Clang warns:
> 
> drivers/crypto/amcc/crypto4xx_core.c:921:60: warning: operator '?:' has
> lower precedence than '|'; '|' will be evaluated first
> [-Wbitwise-conditional-parentheses]
>                   (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
>                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> drivers/crypto/amcc/crypto4xx_core.c:921:60: note: place parentheses
> around the '|' expression to silence this warning
>                   (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
>                                                                           ^
>                                                                          )
> drivers/crypto/amcc/crypto4xx_core.c:921:60: note: place parentheses
> around the '?:' expression to evaluate it first
>                   (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
>                                                                           ^
>                   (
> 1 warning generated.
> 
> It looks like this should have been a logical OR so that
> PD_CTL_HASH_FINAL gets added to the w bitmask if crypto_tfm_alg_type
> is either CRYPTO_ALG_TYPE_AHASH or CRYPTO_ALG_TYPE_AEAD.
Yes. This probably wasn't spotted earlier since the driver doesn't make
use of CRYPTO_ALG_TYPE_AHASH (yet). This is because the hash accelerator
setup cost was never worth it.

> Change the operator so that everything works properly.
I'm curious if this is true. Is there a way to break this somehow on purpose?

I've extracted the code from line 921 and added the defines
(the CRYPTO_ALG_... from the current 5.10-rc3 crypto.h and the PD_CTL_
from crypto4xx_reg_def.h) and replaced the u32 with uint32_t
so it runs in userspace too:

--- crypto4xx_test.c ---
/* test study - is it possible to break the | vs || in crypto4xx's code */

#include <stdio.h>
#include <stdint.h>

#define CRYPTO_ALG_TYPE_AEAD 	0x00000003
#define CRYPTO_ALG_TYPE_AHASH	0x0000000f
#define PD_CTL_HASH_FINAL	(1<<4) /* Stand-in for BIT(4) */
#define PD_CTL_HOST_READY	(1<<0) /* BIT(0) */

uint32_t func_with_bitwise_or(uint32_t alg_type)
{
	return PD_CTL_HOST_READY |
		((alg_type == CRYPTO_ALG_TYPE_AHASH) |
		 (alg_type == CRYPTO_ALG_TYPE_AEAD) ?
			PD_CTL_HASH_FINAL : 0);
}

uint32_t func_with_logical_or(uint32_t alg_type)
{
	return PD_CTL_HOST_READY |
		((alg_type == CRYPTO_ALG_TYPE_AHASH) ||
		 (alg_type == CRYPTO_ALG_TYPE_AEAD) ?
			PD_CTL_HASH_FINAL : 0);
}

int main(int arg, char **args)
{
	uint32_t alg;

	for (alg = 0; alg < 0x10; alg++) { /* this is because CRYPTO_ALG_TYPE_MASK is 0xf */
		if (func_with_bitwise_or(alg) != func_with_logical_or(alg)) {
			printf("for alg_type:%d, the bitwise result=%d doesn't match the logical result=%d\n",
				alg, func_with_bitwise_or(alg), func_with_logical_or(alg));
			return 1;
		}
	}
	printf("logical and bitwise always agreed.\n");

	return 0;
}
--- EOF ---

Both gcc (gcc version 10.2.0 (Debian 10.2.0-17)) or clang (clang version 9.0.1-15)
version always gave the "logical and bitwise always agreed.". which means there wasn't
anything wrong and this patch just makes clang happy? Or can you get it to break?

Also, can you please give this patch a try:
--- extra-bracket.patch

--- a/drivers/crypto/amcc/crypto4xx_core.c
+++ b/drivers/crypto/amcc/crypto4xx_core.c
@@ -932,8 +932,8 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
  	}

  	pd->pd_ctl.w = PD_CTL_HOST_READY |
-		((crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AHASH) |
-		 (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
+		(((crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AHASH) |
+		  (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD)) ?
  			PD_CTL_HASH_FINAL : 0);
  	pd->pd_ctl_len.w = 0x00400000 | (assoclen + datalen);
  	pd_uinfo->state = PD_ENTRY_INUSE | (is_busy ? PD_ENTRY_BUSY : 0);

---
I'm mostly curious if clang will warn about it too.

That said:
Reviewed-by: Christian Lamparter <chunkeey@gmail.com>

Cheers,
Christian

> Fixes: 4b5b79998af6 ("crypto: crypto4xx - fix stalls under heavy load")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1198
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>   drivers/crypto/amcc/crypto4xx_core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c
> index 981de43ea5e2..2e3690f65786 100644
> --- a/drivers/crypto/amcc/crypto4xx_core.c
> +++ b/drivers/crypto/amcc/crypto4xx_core.c
> @@ -917,7 +917,7 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
>   	}
>   
>   	pd->pd_ctl.w = PD_CTL_HOST_READY |
> -		((crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AHASH) |
> +		((crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AHASH) ||
>   		 (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
>   			 : 0);
>   	pd->pd_ctl_len.w = 0x00400000 | (assoclen + datalen);
> 
> base-commit: f8394f232b1eab649ce2df5c5f15b0e528c92091
>