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. Change the
operator so that everything works properly.
Fixes: 4b5b79998af6 ("crypto: crypto4xx - fix stalls under heavy load")
Link: https://github.com/ClangBuiltLinux/linux/issues/1198
Signed-off-by: Nathan Chancellor <[email protected]>
---
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) ?
PD_CTL_HASH_FINAL : 0);
pd->pd_ctl_len.w = 0x00400000 | (assoclen + datalen);
base-commit: f8394f232b1eab649ce2df5c5f15b0e528c92091
--
2.29.2
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 <[email protected]>
Cheers,
Christian
> Fixes: 4b5b79998af6 ("crypto: crypto4xx - fix stalls under heavy load")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1198
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> 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
>
On Thu, Nov 12, 2020 at 10:21:35PM +0100, Christian Lamparter wrote:
> 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 do not really have a way to validate that statement, I just figured
that the operator being wrong meant that something could go wrong that
was not intended.
> 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);
> }
Looking at this more, I think the only reason that the code works as is
is because PD_CTL_HOST_READY is 1 AND CRYPTO_ALG_TYPE_AHASH is not used.
(alg_type == CRYPTO_ALG_TYPE_AEAD) ? PD_CTL_HASH_FINAL : 0 is evaluated
first, which results in either PD_CTL_HASH_FINAL or 0.
Then (alg_type == CRYPTO_ALG_TYPE_AHASH) is evaluated, which is
evaluated to either 0 or 1.
Then we mask everything together:
PD_CTL_HOST_READY | (0 || 1) | (PD_CTL_HOST_READY || 0)
If PD_CTL_HOST_READY was anything other than BIT(0), we would have an
extra 0x1 in the mask. That realistically might not matter, I did not
have a full look over the code to see what this might mean. If
CRYPTO_ALG_TYPE_AHASH was used, it could be used over
CRYPTO_ALG_TYPE_AEAD and PD_CTL_HASH_FINAL would never get added to the
mask, which certainly sounds like a bug.
> 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.
It does not with that diff. I guess it is entirely up to you which one
we go with.
> That said:
> Reviewed-by: Christian Lamparter <[email protected]>
Thank you for all the analysis and taking a look over the patch, I
appreciate it!
Cheers,
Nathan
From: Nathan Chancellor
> Sent: 12 November 2020 21:49
>
> On Thu, Nov 12, 2020 at 10:21:35PM +0100, Christian Lamparter wrote:
> > 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 do not really have a way to validate that statement, I just figured
> that the operator being wrong meant that something could go wrong that
> was not intended.
>
> > 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);
> > }
>
> Looking at this more, I think the only reason that the code works as is
> is because PD_CTL_HOST_READY is 1 AND CRYPTO_ALG_TYPE_AHASH is not used.
>
> (alg_type == CRYPTO_ALG_TYPE_AEAD) ? PD_CTL_HASH_FINAL : 0 is evaluated
> first, which results in either PD_CTL_HASH_FINAL or 0.
>
> Then (alg_type == CRYPTO_ALG_TYPE_AHASH) is evaluated, which is
> evaluated to either 0 or 1.
>
> Then we mask everything together:
>
> PD_CTL_HOST_READY | (0 || 1) | (PD_CTL_HOST_READY || 0)
The result is the same for both | and || as they are both higher
priority than ?: (which is only higher priority than ,).
The () around the == aren't needed (except to stop the compiler
bleating). The bitwise | is lower priority than == because it
existed before || and K&R didn't change the priority when they
added || (I think they've said later they wished they had.)
The () around the entire ?: clause are needed.
So the code is the same as:
rval = PD_CTL_HOST_READY;
if (alg_type == CRYPTO_ALG_TYPE_AHASH | alg_type == CRYPTO_ALG_TYPE_AEAD)
rval |= PD_CTL_HASH_FINAL;
return rval;
Using | may well generate faster code (no branches).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, Nov 12, 2020 at 01:07:02PM -0700, 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. Change the
> operator so that everything works properly.
>
> Fixes: 4b5b79998af6 ("crypto: crypto4xx - fix stalls under heavy load")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1198
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> drivers/crypto/amcc/crypto4xx_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt