In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, compiler will report variable uninitilized warning: ../drivers/crypto/qat/qat_sym_session.c: In function ‘partial_hash_compute’: ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: ‘<U35a0>’ may be used uninitialized in this function [-Wmaybe-uninitialized] 241 | #define rte_bswap32(x) __builtin_bswap32(x) ... This is because "digest" will be initialized by "rte_memcpy" function rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. However, compiler cannot know it is initialized by the function. To fix this, use "calloc" to initialize "digest". Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") Cc: stable@dpdk.org Signed-off-by: Feifei Wang <feifei.wang2@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> --- drivers/crypto/qat/qat_sym_session.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c index 231b1640da..dab23a3290 100644 --- a/drivers/crypto/qat/qat_sym_session.c +++ b/drivers/crypto/qat/qat_sym_session.c @@ -1190,8 +1190,8 @@ static int partial_hash_compute(enum icp_qat_hw_auth_algo hash_alg, uint8_t *data_out) { int digest_size; - uint8_t digest[qat_hash_get_digest_size( - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; + uint8_t *digest = (uint8_t *)calloc(qat_hash_get_digest_size( + ICP_QAT_HW_AUTH_ALGO_DELIMITER), sizeof(uint8_t)); uint32_t *hash_state_out_be32; uint64_t *hash_state_out_be64; int i; -- 2.25.1
On Fri, May 14, 2021 at 9:41 AM Feifei Wang <feifei.wang2@arm.com> wrote: > > In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, compiler > will report variable uninitilized warning: > > ../drivers/crypto/qat/qat_sym_session.c: In function ‘partial_hash_compute’: > ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: > ‘<U35a0>’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > 241 | #define rte_bswap32(x) __builtin_bswap32(x) > ... > > This is because "digest" will be initialized by "rte_memcpy" function > rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. However, > compiler cannot know it is initialized by the function. > > To fix this, use "calloc" to initialize "digest". > > Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") > Cc: stable@dpdk.org > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > --- > drivers/crypto/qat/qat_sym_session.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c > index 231b1640da..dab23a3290 100644 > --- a/drivers/crypto/qat/qat_sym_session.c > +++ b/drivers/crypto/qat/qat_sym_session.c > @@ -1190,8 +1190,8 @@ static int partial_hash_compute(enum icp_qat_hw_auth_algo hash_alg, > uint8_t *data_out) > { > int digest_size; > - uint8_t digest[qat_hash_get_digest_size( > - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; > + uint8_t *digest = (uint8_t *)calloc(qat_hash_get_digest_size( > + ICP_QAT_HW_AUTH_ALGO_DELIMITER), sizeof(uint8_t)); calloc can fail. cast is unneded. A simple uint8_t digest[...] = { 0 } would do the job. > uint32_t *hash_state_out_be32; > uint64_t *hash_state_out_be64; > int i; > -- > 2.25.1 > -- David Marchand
Hi, David > -----邮件原件----- > 发件人: David Marchand <david.marchand@redhat.com> > 发送时间: 2021年5月14日 15:50 > 收件人: Feifei Wang <Feifei.Wang2@arm.com> > 抄送: John Griffin <john.griffin@intel.com>; Fiona Trahe > <fiona.trahe@intel.com>; Deepak Kumar Jain <deepak.k.jain@intel.com>; > Herbert Guan <Herbert.Guan@arm.com>; Jerin Jacob > <jerin.jacob@caviumnetworks.com>; dev <dev@dpdk.org>; nd > <nd@arm.com>; dpdk stable <stable@dpdk.org>; Ruifeng Wang > <Ruifeng.Wang@arm.com> > 主题: Re: [dpdk-stable] [PATCH] crypto/qat: fix uninitilized compiler warning > > On Fri, May 14, 2021 at 9:41 AM Feifei Wang <feifei.wang2@arm.com> wrote: > > > > In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, > compiler > > will report variable uninitilized warning: > > > > ../drivers/crypto/qat/qat_sym_session.c: In function > ‘partial_hash_compute’: > > ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: > > ‘<U35a0>’ may be used uninitialized in this function > > [-Wmaybe-uninitialized] > > 241 | #define rte_bswap32(x) __builtin_bswap32(x) > > ... > > > > This is because "digest" will be initialized by "rte_memcpy" function > > rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. > > However, compiler cannot know it is initialized by the function. > > > > To fix this, use "calloc" to initialize "digest". > > > > Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") > > Cc: stable@dpdk.org > > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > > --- > > drivers/crypto/qat/qat_sym_session.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/crypto/qat/qat_sym_session.c > > b/drivers/crypto/qat/qat_sym_session.c > > index 231b1640da..dab23a3290 100644 > > --- a/drivers/crypto/qat/qat_sym_session.c > > +++ b/drivers/crypto/qat/qat_sym_session.c > > @@ -1190,8 +1190,8 @@ static int partial_hash_compute(enum > icp_qat_hw_auth_algo hash_alg, > > uint8_t *data_out) { > > int digest_size; > > - uint8_t digest[qat_hash_get_digest_size( > > - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; > > + uint8_t *digest = (uint8_t *)calloc(qat_hash_get_digest_size( > > + ICP_QAT_HW_AUTH_ALGO_DELIMITER), > > + sizeof(uint8_t)); > > calloc can fail. > cast is unneded. > > A simple uint8_t digest[...] = { 0 } would do the job. Thanks for your comment. Actually, we have tried to use this simple method: uint8_t digest[qat_hash_get_digest_size(ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0}; And it will report compile error: ../drivers/crypto/qat/qat_sym_session.c:1194:4: error: variable-sized object may not be initialized 1194 | ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0}; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../drivers/crypto/qat/qat_sym_session.c:1194:40: warning: excess elements in array initializer 1194 | ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0}; | ^ ../drivers/crypto/qat/qat_sym_session.c:1194:40: note: (near initialization for ‘digest’) ninja: build stopped: subcommand failed. I think this is because compiler cannot know the size of digest due to function 'qat_hash_get_digest_size' during compiling time. Best Regards Feifei > > > > uint32_t *hash_state_out_be32; > > uint64_t *hash_state_out_be64; > > int i; > > -- > > 2.25.1 > > > > > > -- > David Marchand
On Fri, May 14, 2021 at 10:01 AM Feifei Wang <Feifei.Wang2@arm.com> wrote:
> > > @@ -1190,8 +1190,8 @@ static int partial_hash_compute(enum
> > icp_qat_hw_auth_algo hash_alg,
> > > uint8_t *data_out) {
> > > int digest_size;
> > > - uint8_t digest[qat_hash_get_digest_size(
> > > - ICP_QAT_HW_AUTH_ALGO_DELIMITER)];
> > > + uint8_t *digest = (uint8_t *)calloc(qat_hash_get_digest_size(
> > > + ICP_QAT_HW_AUTH_ALGO_DELIMITER),
> > > + sizeof(uint8_t));
> >
> > calloc can fail.
> > cast is unneded.
> >
> > A simple uint8_t digest[...] = { 0 } would do the job.
>
> Thanks for your comment. Actually, we have tried to use this simple method:
> uint8_t digest[qat_hash_get_digest_size(ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0};
>
> And it will report compile error:
> ../drivers/crypto/qat/qat_sym_session.c:1194:4: error: variable-sized object may not be initialized
> 1194 | ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0};
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/crypto/qat/qat_sym_session.c:1194:40: warning: excess elements in array initializer
> 1194 | ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0};
> | ^
> ../drivers/crypto/qat/qat_sym_session.c:1194:40: note: (near initialization for ‘digest’)
> ninja: build stopped: subcommand failed.
>
> I think this is because compiler cannot know the size of digest due to function
> 'qat_hash_get_digest_size' during compiling time.
Ok, I had not noticed the function call.
Please check dynamic allocation succeeds.
+ free this temp buffer before leaving this helper function.
--
David marchand
> -----邮件原件----- > 发件人: David Marchand <david.marchand@redhat.com> > 发送时间: 2021年5月14日 16:14 > 收件人: Feifei Wang <Feifei.Wang2@arm.com> > 抄送: John Griffin <john.griffin@intel.com>; Fiona Trahe > <fiona.trahe@intel.com>; Deepak Kumar Jain <deepak.k.jain@intel.com>; > Herbert Guan <Herbert.Guan@arm.com>; Jerin Jacob > <jerin.jacob@caviumnetworks.com>; dev <dev@dpdk.org>; nd > <nd@arm.com>; dpdk stable <stable@dpdk.org>; Ruifeng Wang > <Ruifeng.Wang@arm.com> > 主题: Re: [dpdk-stable] [PATCH] crypto/qat: fix uninitilized compiler warning > > On Fri, May 14, 2021 at 10:01 AM Feifei Wang <Feifei.Wang2@arm.com> > wrote: > > > > @@ -1190,8 +1190,8 @@ static int partial_hash_compute(enum > > > icp_qat_hw_auth_algo hash_alg, > > > > uint8_t *data_out) { > > > > int digest_size; > > > > - uint8_t digest[qat_hash_get_digest_size( > > > > - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; > > > > + uint8_t *digest = (uint8_t *)calloc(qat_hash_get_digest_size( > > > > + ICP_QAT_HW_AUTH_ALGO_DELIMITER), > > > > + sizeof(uint8_t)); > > > > > > calloc can fail. > > > cast is unneded. > > > > > > A simple uint8_t digest[...] = { 0 } would do the job. > > > > Thanks for your comment. Actually, we have tried to use this simple > method: > > uint8_t > > digest[qat_hash_get_digest_size(ICP_QAT_HW_AUTH_ALGO_DELIMITER)] > = > > {0}; > > > > And it will report compile error: > > ../drivers/crypto/qat/qat_sym_session.c:1194:4: error: variable-sized > object may not be initialized > > 1194 | ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0}; > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../drivers/crypto/qat/qat_sym_session.c:1194:40: warning: excess > elements in array initializer > > 1194 | ICP_QAT_HW_AUTH_ALGO_DELIMITER)] = {0}; > > | ^ > > ../drivers/crypto/qat/qat_sym_session.c:1194:40: note: (near > > initialization for ‘digest’) > > ninja: build stopped: subcommand failed. > > > > I think this is because compiler cannot know the size of digest due to > > function 'qat_hash_get_digest_size' during compiling time. > > Ok, I had not noticed the function call. > > Please check dynamic allocation succeeds. > + free this temp buffer before leaving this helper function. Thanks for this comments, I will add check and free operations for this dynamic allocation in the next version Best Regards Feifei > > > -- > David marchand
In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, compiler will report variable uninitilized warning: ../drivers/crypto/qat/qat_sym_session.c: In function ‘partial_hash_compute’: ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: ‘<U35a0>’ may be used uninitialized in this function [-Wmaybe-uninitialized] 241 | #define rte_bswap32(x) __builtin_bswap32(x) ... This is because "digest" will be initialized by "rte_memcpy" function rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. However, compiler cannot know it is initialized by the function. To fix this, use "calloc" to initialize "digest". Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") Cc: stable@dpdk.org Signed-off-by: Feifei Wang <feifei.wang2@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> --- v2: add check and free for memory dynamic allocation (David Marchand) v3: fix compiler error drivers/crypto/qat/qat_sym_session.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c index 231b1640da..105a10957a 100644 --- a/drivers/crypto/qat/qat_sym_session.c +++ b/drivers/crypto/qat/qat_sym_session.c @@ -1190,8 +1190,7 @@ static int partial_hash_compute(enum icp_qat_hw_auth_algo hash_alg, uint8_t *data_out) { int digest_size; - uint8_t digest[qat_hash_get_digest_size( - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; + uint8_t *digest; uint32_t *hash_state_out_be32; uint64_t *hash_state_out_be64; int i; @@ -1200,55 +1199,65 @@ static int partial_hash_compute(enum icp_qat_hw_auth_algo hash_alg, if (digest_size <= 0) return -EFAULT; + digest = calloc(qat_hash_get_digest_size( + ICP_QAT_HW_AUTH_ALGO_DELIMITER), sizeof(uint8_t)); + if (!digest) + return -ENOMEM; + hash_state_out_be32 = (uint32_t *)data_out; hash_state_out_be64 = (uint64_t *)data_out; switch (hash_alg) { case ICP_QAT_HW_AUTH_ALGO_SHA1: if (partial_hash_sha1(data_in, digest)) - return -EFAULT; + goto fail; for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++) *hash_state_out_be32 = rte_bswap32(*(((uint32_t *)digest)+i)); break; case ICP_QAT_HW_AUTH_ALGO_SHA224: if (partial_hash_sha224(data_in, digest)) - return -EFAULT; + goto fail; for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++) *hash_state_out_be32 = rte_bswap32(*(((uint32_t *)digest)+i)); break; case ICP_QAT_HW_AUTH_ALGO_SHA256: if (partial_hash_sha256(data_in, digest)) - return -EFAULT; + goto fail; for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++) *hash_state_out_be32 = rte_bswap32(*(((uint32_t *)digest)+i)); break; case ICP_QAT_HW_AUTH_ALGO_SHA384: if (partial_hash_sha384(data_in, digest)) - return -EFAULT; + goto fail; for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++) *hash_state_out_be64 = rte_bswap64(*(((uint64_t *)digest)+i)); break; case ICP_QAT_HW_AUTH_ALGO_SHA512: if (partial_hash_sha512(data_in, digest)) - return -EFAULT; + goto fail; for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++) *hash_state_out_be64 = rte_bswap64(*(((uint64_t *)digest)+i)); break; case ICP_QAT_HW_AUTH_ALGO_MD5: if (partial_hash_md5(data_in, data_out)) - return -EFAULT; + goto fail; break; default: QAT_LOG(ERR, "invalid hash alg %u", hash_alg); - return -EFAULT; + goto fail; } + free(digest); return 0; + +fail: + free(digest); + return -EFAULT; } #define HMAC_IPAD_VALUE 0x36 #define HMAC_OPAD_VALUE 0x5c -- 2.25.1
17/05/2021 11:07, Feifei Wang:
> In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, compiler
> will report variable uninitilized warning:
>
> ../drivers/crypto/qat/qat_sym_session.c:
> In function ‘partial_hash_compute’:
> ../lib/eal/include/generic/rte_byteorder.h:241:24: warning:
> ‘<U35a0>’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 241 | #define rte_bswap32(x) __builtin_bswap32(x)
> ...
>
> This is because "digest" will be initialized by "rte_memcpy" function
> rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. However,
> compiler cannot know it is initialized by the function.
>
> To fix this, use "calloc" to initialize "digest".
>
> Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy")
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v2: add check and free for memory dynamic allocation (David Marchand)
> v3: fix compiler error
I am not confortable applying this patch without PMD maintainers ack.
On 5/17/2021 10:07 AM, Feifei Wang wrote: > In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, compiler > will report variable uninitilized warning: > > ../drivers/crypto/qat/qat_sym_session.c: > In function ‘partial_hash_compute’: > ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: > ‘<U35a0>’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > 241 | #define rte_bswap32(x) __builtin_bswap32(x) > ... > > This is because "digest" will be initialized by "rte_memcpy" function > rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. However, How 'digest' is initialized by 'rte_memcpy'? > compiler cannot know it is initialized by the function. > > To fix this, use "calloc" to initialize "digest". > > Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") > Cc: stable@dpdk.org > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > --- > v2: add check and free for memory dynamic allocation (David Marchand) > v3: fix compiler error > > drivers/crypto/qat/qat_sym_session.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c > index 231b1640da..105a10957a 100644 > --- a/drivers/crypto/qat/qat_sym_session.c > +++ b/drivers/crypto/qat/qat_sym_session.c > @@ -1190,8 +1190,7 @@ static int partial_hash_compute(enum icp_qat_hw_auth_algo hash_alg, > uint8_t *data_out) > { > int digest_size; > - uint8_t digest[qat_hash_get_digest_size( > - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; > + uint8_t *digest; Will a memset 'digest' work too? Although not sure which one is better.
Hi.
I checked how it works on QAT hardware, and it does no harm. All unit tests still pass.
Verified on two QAT-enabled machines.
The question is: should this patch be simplified to just add a call to memset (to zero the digest variable while still keeping it on the stack) at the function begin? And that will be a faster solution in run-time (using the variable on the stack instead of calloc that uses the heap).
Adam
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Feifei Wang
> Sent: Monday, 17 May, 2021 11:07
> To: Griffin, John <john.griffin@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>; Jerin
> Jacob <jerin.jacob@caviumnetworks.com>; Herbert Guan
> <herbert.guan@arm.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; nd@arm.com; Feifei
> Wang <feifei.wang2@arm.com>; stable@dpdk.org; Ruifeng Wang
> <ruifeng.wang@arm.com>
> Subject: [dpdk-dev] [PATCH v3] crypto/qat: fix uninitilized compiler warning
>
> In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true,
> compiler will report variable uninitilized warning:
>
> ../drivers/crypto/qat/qat_sym_session.c:
> In function ‘partial_hash_compute’:
> ../lib/eal/include/generic/rte_byteorder.h:241:24: warning:
> ‘<U35a0>’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 241 | #define rte_bswap32(x) __builtin_bswap32(x)
> ...
>
> This is because "digest" will be initialized by "rte_memcpy" function rather
> than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. However,
> compiler cannot know it is initialized by the function.
>
> To fix this, use "calloc" to initialize "digest".
>
> Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy")
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v2: add check and free for memory dynamic allocation (David Marchand)
> v3: fix compiler error
>
> drivers/crypto/qat/qat_sym_session.c | 27 ++++++++++++++++++---------
> 1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/crypto/qat/qat_sym_session.c
> b/drivers/crypto/qat/qat_sym_session.c
> index 231b1640da..105a10957a 100644
> --- a/drivers/crypto/qat/qat_sym_session.c
> +++ b/drivers/crypto/qat/qat_sym_session.c
> @@ -1190,8 +1190,7 @@ static int partial_hash_compute(enum
> icp_qat_hw_auth_algo hash_alg,
> uint8_t *data_out)
> {
> int digest_size;
> - uint8_t digest[qat_hash_get_digest_size(
> - ICP_QAT_HW_AUTH_ALGO_DELIMITER)];
> + uint8_t *digest;
> uint32_t *hash_state_out_be32;
> uint64_t *hash_state_out_be64;
> int i;
> @@ -1200,55 +1199,65 @@ static int partial_hash_compute(enum
> icp_qat_hw_auth_algo hash_alg,
> if (digest_size <= 0)
> return -EFAULT;
>
> + digest = calloc(qat_hash_get_digest_size(
> + ICP_QAT_HW_AUTH_ALGO_DELIMITER),
> sizeof(uint8_t));
> + if (!digest)
> + return -ENOMEM;
> +
> hash_state_out_be32 = (uint32_t *)data_out;
> hash_state_out_be64 = (uint64_t *)data_out;
>
> switch (hash_alg) {
> case ICP_QAT_HW_AUTH_ALGO_SHA1:
> if (partial_hash_sha1(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++)
> *hash_state_out_be32 =
> rte_bswap32(*(((uint32_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA224:
> if (partial_hash_sha224(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++)
> *hash_state_out_be32 =
> rte_bswap32(*(((uint32_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA256:
> if (partial_hash_sha256(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++)
> *hash_state_out_be32 =
> rte_bswap32(*(((uint32_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA384:
> if (partial_hash_sha384(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++)
> *hash_state_out_be64 =
> rte_bswap64(*(((uint64_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA512:
> if (partial_hash_sha512(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++)
> *hash_state_out_be64 =
> rte_bswap64(*(((uint64_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_MD5:
> if (partial_hash_md5(data_in, data_out))
> - return -EFAULT;
> + goto fail;
> break;
> default:
> QAT_LOG(ERR, "invalid hash alg %u", hash_alg);
> - return -EFAULT;
> + goto fail;
> }
>
> + free(digest);
> return 0;
> +
> +fail:
> + free(digest);
> + return -EFAULT;
> }
> #define HMAC_IPAD_VALUE 0x36
> #define HMAC_OPAD_VALUE 0x5c
> --
> 2.25.1
Hi.
I checked how it works on QAT hardware, and it does no harm. All unit tests still pass.
Verified on two QAT-enabled machines.
The question is: should this patch be simplified to just add a call to memset (to zero the digest variable while still keeping it on the stack) at the function begin? And that will be a faster solution in run-time (using the variable on the stack instead of calloc that uses the heap).
Adam
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Feifei Wang
> Sent: Monday, 17 May, 2021 11:07
> To: Griffin, John <john.griffin@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>; Jerin
> Jacob <jerin.jacob@caviumnetworks.com>; Herbert Guan
> <herbert.guan@arm.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; nd@arm.com; Feifei
> Wang <feifei.wang2@arm.com>; stable@dpdk.org; Ruifeng Wang
> <ruifeng.wang@arm.com>
> Subject: [dpdk-dev] [PATCH v3] crypto/qat: fix uninitilized compiler warning
>
> In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true,
> compiler will report variable uninitilized warning:
>
> ../drivers/crypto/qat/qat_sym_session.c:
> In function ‘partial_hash_compute’:
> ../lib/eal/include/generic/rte_byteorder.h:241:24: warning:
> ‘<U35a0>’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 241 | #define rte_bswap32(x) __builtin_bswap32(x)
> ...
>
> This is because "digest" will be initialized by "rte_memcpy" function rather
> than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. However,
> compiler cannot know it is initialized by the function.
>
> To fix this, use "calloc" to initialize "digest".
>
> Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy")
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v2: add check and free for memory dynamic allocation (David Marchand)
> v3: fix compiler error
>
> drivers/crypto/qat/qat_sym_session.c | 27 ++++++++++++++++++---------
> 1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/crypto/qat/qat_sym_session.c
> b/drivers/crypto/qat/qat_sym_session.c
> index 231b1640da..105a10957a 100644
> --- a/drivers/crypto/qat/qat_sym_session.c
> +++ b/drivers/crypto/qat/qat_sym_session.c
> @@ -1190,8 +1190,7 @@ static int partial_hash_compute(enum
> icp_qat_hw_auth_algo hash_alg,
> uint8_t *data_out)
> {
> int digest_size;
> - uint8_t digest[qat_hash_get_digest_size(
> - ICP_QAT_HW_AUTH_ALGO_DELIMITER)];
> + uint8_t *digest;
> uint32_t *hash_state_out_be32;
> uint64_t *hash_state_out_be64;
> int i;
> @@ -1200,55 +1199,65 @@ static int partial_hash_compute(enum
> icp_qat_hw_auth_algo hash_alg,
> if (digest_size <= 0)
> return -EFAULT;
>
> + digest = calloc(qat_hash_get_digest_size(
> + ICP_QAT_HW_AUTH_ALGO_DELIMITER),
> sizeof(uint8_t));
> + if (!digest)
> + return -ENOMEM;
> +
> hash_state_out_be32 = (uint32_t *)data_out;
> hash_state_out_be64 = (uint64_t *)data_out;
>
> switch (hash_alg) {
> case ICP_QAT_HW_AUTH_ALGO_SHA1:
> if (partial_hash_sha1(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++)
> *hash_state_out_be32 =
> rte_bswap32(*(((uint32_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA224:
> if (partial_hash_sha224(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++)
> *hash_state_out_be32 =
> rte_bswap32(*(((uint32_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA256:
> if (partial_hash_sha256(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++)
> *hash_state_out_be32 =
> rte_bswap32(*(((uint32_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA384:
> if (partial_hash_sha384(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++)
> *hash_state_out_be64 =
> rte_bswap64(*(((uint64_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_SHA512:
> if (partial_hash_sha512(data_in, digest))
> - return -EFAULT;
> + goto fail;
> for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++)
> *hash_state_out_be64 =
> rte_bswap64(*(((uint64_t *)digest)+i));
> break;
> case ICP_QAT_HW_AUTH_ALGO_MD5:
> if (partial_hash_md5(data_in, data_out))
> - return -EFAULT;
> + goto fail;
> break;
> default:
> QAT_LOG(ERR, "invalid hash alg %u", hash_alg);
> - return -EFAULT;
> + goto fail;
> }
>
> + free(digest);
> return 0;
> +
> +fail:
> + free(digest);
> + return -EFAULT;
> }
> #define HMAC_IPAD_VALUE 0x36
> #define HMAC_OPAD_VALUE 0x5c
> --
> 2.25.1
Hi, Ferruh Thanks for your comments. Please see below. > -----邮件原件----- > 发件人: Ferruh Yigit <ferruh.yigit@intel.com> > 发送时间: 2021年5月19日 16:12 > 收件人: Feifei Wang <Feifei.Wang2@arm.com>; John Griffin > <john.griffin@intel.com>; Fiona Trahe <fiona.trahe@intel.com>; Deepak > Kumar Jain <deepak.k.jain@intel.com>; Jerin Jacob > <jerin.jacob@caviumnetworks.com>; Herbert Guan > <Herbert.Guan@arm.com> > 抄送: dev@dpdk.org; david.marchand@redhat.com; nd <nd@arm.com>; > stable@dpdk.org; Ruifeng Wang <Ruifeng.Wang@arm.com> > 主题: Re: [dpdk-stable] [PATCH v3] crypto/qat: fix uninitilized compiler > warning > > On 5/17/2021 10:07 AM, Feifei Wang wrote: > > In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, > compiler > > will report variable uninitilized warning: > > > > ../drivers/crypto/qat/qat_sym_session.c: > > In function ‘partial_hash_compute’: > > ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: > > ‘<U35a0>’ may be used uninitialized in this function > > [-Wmaybe-uninitialized] > > 241 | #define rte_bswap32(x) __builtin_bswap32(x) > > ... > > > > This is because "digest" will be initialized by "rte_memcpy" function > > rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. > > However, > > How 'digest' is initialized by 'rte_memcpy'? Firstly, 'digest' is initialized by rte_memcpy in partial_hash_sha_x function : 'partial_hash_compute' -> 'partial_hash_sha_x' -> 'rte_memcpy'. If "RTE_ARCH_ARM64_MEMCPY = false", rte_memcpy will be defined as 'memcpy' to initialize 'digest' in lib\eal\arm\include\rte_memcpy_64.h: 364, and compiler can identify this. However, if "RTE_ARCH_ARM64_MEMCPY = true", rte_memcpy will be a inline function, and finally it will initialize 'digest' with two steps by invoking rte_mov16: rte_memcpy -> rte_memcpy_ge16_lt_128 -> step 1: rte_mov16(dst,src ) step 2: rte_mov16(dst - 16 + n, src - 16 + n) And the compiler cannot identify this multi-step initialization, then it will report warning. > > > compiler cannot know it is initialized by the function. > > > > To fix this, use "calloc" to initialize "digest". > > > > Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") > > Cc: stable@dpdk.org > > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > > --- > > v2: add check and free for memory dynamic allocation (David Marchand) > > v3: fix compiler error > > > > drivers/crypto/qat/qat_sym_session.c | 27 ++++++++++++++++++--------- > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/crypto/qat/qat_sym_session.c > > b/drivers/crypto/qat/qat_sym_session.c > > index 231b1640da..105a10957a 100644 > > --- a/drivers/crypto/qat/qat_sym_session.c > > +++ b/drivers/crypto/qat/qat_sym_session.c > > @@ -1190,8 +1190,7 @@ static int partial_hash_compute(enum > icp_qat_hw_auth_algo hash_alg, > > uint8_t *data_out) > > { > > int digest_size; > > - uint8_t digest[qat_hash_get_digest_size( > > - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; > > + uint8_t *digest; > > Will a memset 'digest' work too? Although not sure which one is better. Thanks for your meaningful comments, I try to use memset and it is ok to solve this warning. I will update this in the next version.
Hi, > -----邮件原件----- > 发件人: Dybkowski, AdamX <adamx.dybkowski@intel.com> > 发送时间: 2021年5月19日 21:17 > 收件人: Feifei Wang <Feifei.Wang2@arm.com>; Griffin, John > <john.griffin@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>; Jain, > Deepak K <deepak.k.jain@intel.com>; Jerin Jacob > <jerin.jacob@caviumnetworks.com>; Herbert Guan > <Herbert.Guan@arm.com> > 抄送: dev@dpdk.org; david.marchand@redhat.com; nd <nd@arm.com>; > stable@dpdk.org; Ruifeng Wang <Ruifeng.Wang@arm.com> > 主题: RE: [dpdk-dev] [PATCH v3] crypto/qat: fix uninitilized compiler warning > > Hi. > > I checked how it works on QAT hardware, and it does no harm. All unit tests > still pass. > Verified on two QAT-enabled machines. Thanks very much for your testing. > > The question is: should this patch be simplified to just add a call to memset > (to zero the digest variable while still keeping it on the stack) at the function > begin? And that will be a faster solution in run-time (using the variable on the > stack instead of calloc that uses the heap). > A good comment for this. I try to apply it and it also can solve the warning. And actually maybe memset is more faster. I will update this in the next version. Best Regards Feifei > Adam > > > -----Original Message----- > > From: dev <dev-bounces@dpdk.org> On Behalf Of Feifei Wang > > Sent: Monday, 17 May, 2021 11:07 > > To: Griffin, John <john.griffin@intel.com>; Trahe, Fiona > > <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>; > > Jerin Jacob <jerin.jacob@caviumnetworks.com>; Herbert Guan > > <herbert.guan@arm.com> > > Cc: dev@dpdk.org; david.marchand@redhat.com; nd@arm.com; Feifei > Wang > > <feifei.wang2@arm.com>; stable@dpdk.org; Ruifeng Wang > > <ruifeng.wang@arm.com> > > Subject: [dpdk-dev] [PATCH v3] crypto/qat: fix uninitilized compiler > > warning > > > > In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, > compiler > > will report variable uninitilized warning: > > > > ../drivers/crypto/qat/qat_sym_session.c: > > In function ‘partial_hash_compute’: > > ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: > > ‘<U35a0>’ may be used uninitialized in this function > > [-Wmaybe-uninitialized] > > 241 | #define rte_bswap32(x) __builtin_bswap32(x) > > ... > > > > This is because "digest" will be initialized by "rte_memcpy" function > > rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. > > However, compiler cannot know it is initialized by the function. > > > > To fix this, use "calloc" to initialize "digest". > > > > Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") > > Cc: stable@dpdk.org > > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > > --- > > v2: add check and free for memory dynamic allocation (David Marchand) > > v3: fix compiler error > > > > drivers/crypto/qat/qat_sym_session.c | 27 ++++++++++++++++++--------- > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/crypto/qat/qat_sym_session.c > > b/drivers/crypto/qat/qat_sym_session.c > > index 231b1640da..105a10957a 100644 > > --- a/drivers/crypto/qat/qat_sym_session.c > > +++ b/drivers/crypto/qat/qat_sym_session.c > > @@ -1190,8 +1190,7 @@ static int partial_hash_compute(enum > > icp_qat_hw_auth_algo hash_alg, > > uint8_t *data_out) > > { > > int digest_size; > > - uint8_t digest[qat_hash_get_digest_size( > > - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; > > + uint8_t *digest; > > uint32_t *hash_state_out_be32; > > uint64_t *hash_state_out_be64; > > int i; > > @@ -1200,55 +1199,65 @@ static int partial_hash_compute(enum > > icp_qat_hw_auth_algo hash_alg, > > if (digest_size <= 0) > > return -EFAULT; > > > > + digest = calloc(qat_hash_get_digest_size( > > + ICP_QAT_HW_AUTH_ALGO_DELIMITER), > > sizeof(uint8_t)); > > + if (!digest) > > + return -ENOMEM; > > + > > hash_state_out_be32 = (uint32_t *)data_out; > > hash_state_out_be64 = (uint64_t *)data_out; > > > > switch (hash_alg) { > > case ICP_QAT_HW_AUTH_ALGO_SHA1: > > if (partial_hash_sha1(data_in, digest)) > > - return -EFAULT; > > + goto fail; > > for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++) > > *hash_state_out_be32 = > > rte_bswap32(*(((uint32_t *)digest)+i)); > > break; > > case ICP_QAT_HW_AUTH_ALGO_SHA224: > > if (partial_hash_sha224(data_in, digest)) > > - return -EFAULT; > > + goto fail; > > for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++) > > *hash_state_out_be32 = > > rte_bswap32(*(((uint32_t *)digest)+i)); > > break; > > case ICP_QAT_HW_AUTH_ALGO_SHA256: > > if (partial_hash_sha256(data_in, digest)) > > - return -EFAULT; > > + goto fail; > > for (i = 0; i < digest_size >> 2; i++, hash_state_out_be32++) > > *hash_state_out_be32 = > > rte_bswap32(*(((uint32_t *)digest)+i)); > > break; > > case ICP_QAT_HW_AUTH_ALGO_SHA384: > > if (partial_hash_sha384(data_in, digest)) > > - return -EFAULT; > > + goto fail; > > for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++) > > *hash_state_out_be64 = > > rte_bswap64(*(((uint64_t *)digest)+i)); > > break; > > case ICP_QAT_HW_AUTH_ALGO_SHA512: > > if (partial_hash_sha512(data_in, digest)) > > - return -EFAULT; > > + goto fail; > > for (i = 0; i < digest_size >> 3; i++, hash_state_out_be64++) > > *hash_state_out_be64 = > > rte_bswap64(*(((uint64_t *)digest)+i)); > > break; > > case ICP_QAT_HW_AUTH_ALGO_MD5: > > if (partial_hash_md5(data_in, data_out)) > > - return -EFAULT; > > + goto fail; > > break; > > default: > > QAT_LOG(ERR, "invalid hash alg %u", hash_alg); > > - return -EFAULT; > > + goto fail; > > } > > > > + free(digest); > > return 0; > > + > > +fail: > > + free(digest); > > + return -EFAULT; > > } > > #define HMAC_IPAD_VALUE 0x36 > > #define HMAC_OPAD_VALUE 0x5c > > -- > > 2.25.1
On 5/20/2021 6:44 AM, Feifei Wang wrote: > Hi, Ferruh > > Thanks for your comments. > Please see below. > >> -----邮件原件----- >> 发件人: Ferruh Yigit <ferruh.yigit@intel.com> >> 发送时间: 2021年5月19日 16:12 >> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; John Griffin >> <john.griffin@intel.com>; Fiona Trahe <fiona.trahe@intel.com>; Deepak >> Kumar Jain <deepak.k.jain@intel.com>; Jerin Jacob >> <jerin.jacob@caviumnetworks.com>; Herbert Guan >> <Herbert.Guan@arm.com> >> 抄送: dev@dpdk.org; david.marchand@redhat.com; nd <nd@arm.com>; >> stable@dpdk.org; Ruifeng Wang <Ruifeng.Wang@arm.com> >> 主题: Re: [dpdk-stable] [PATCH v3] crypto/qat: fix uninitilized compiler >> warning >> >> On 5/17/2021 10:07 AM, Feifei Wang wrote: >>> In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, >> compiler >>> will report variable uninitilized warning: >>> >>> ../drivers/crypto/qat/qat_sym_session.c: >>> In function ‘partial_hash_compute’: >>> ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: >>> ‘<U35a0>’ may be used uninitialized in this function >>> [-Wmaybe-uninitialized] >>> 241 | #define rte_bswap32(x) __builtin_bswap32(x) >>> ... >>> >>> This is because "digest" will be initialized by "rte_memcpy" function >>> rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. >>> However, >> >> How 'digest' is initialized by 'rte_memcpy'? > > Firstly, 'digest' is initialized by rte_memcpy in partial_hash_sha_x function : > 'partial_hash_compute' -> 'partial_hash_sha_x' -> 'rte_memcpy'. > > If "RTE_ARCH_ARM64_MEMCPY = false", rte_memcpy will be defined as > 'memcpy' to initialize 'digest' in lib\eal\arm\include\rte_memcpy_64.h: 364, > and compiler can identify this. > > However, if "RTE_ARCH_ARM64_MEMCPY = true", rte_memcpy will be a inline > function, and finally it will initialize 'digest' with two steps by invoking rte_mov16: > rte_memcpy -> rte_memcpy_ge16_lt_128 -> > step 1: rte_mov16(dst,src ) > step 2: rte_mov16(dst - 16 + n, src - 16 + n) > And the compiler cannot identify this multi-step initialization, then it will report warning. > OK, I got what you mean, thanks for clarification. >> >>> compiler cannot know it is initialized by the function. >>> >>> To fix this, use "calloc" to initialize "digest". >>> >>> Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Feifei Wang <feifei.wang2@arm.com> >>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> >>> --- >>> v2: add check and free for memory dynamic allocation (David Marchand) >>> v3: fix compiler error >>> >>> drivers/crypto/qat/qat_sym_session.c | 27 ++++++++++++++++++--------- >>> 1 file changed, 18 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/crypto/qat/qat_sym_session.c >>> b/drivers/crypto/qat/qat_sym_session.c >>> index 231b1640da..105a10957a 100644 >>> --- a/drivers/crypto/qat/qat_sym_session.c >>> +++ b/drivers/crypto/qat/qat_sym_session.c >>> @@ -1190,8 +1190,7 @@ static int partial_hash_compute(enum >> icp_qat_hw_auth_algo hash_alg, >>> uint8_t *data_out) >>> { >>> int digest_size; >>> - uint8_t digest[qat_hash_get_digest_size( >>> - ICP_QAT_HW_AUTH_ALGO_DELIMITER)]; >>> + uint8_t *digest; >> >> Will a memset 'digest' work too? Although not sure which one is better. > Thanks for your meaningful comments, I try to use memset and it is ok to solve this warning. > I will update this in the next version. >
In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, gcc will report variable uninitilized warning: ../drivers/crypto/qat/qat_sym_session.c: In function ‘partial_hash_compute’: ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: ‘<U35a0>’ may be used uninitialized in this function [-Wmaybe-uninitialized] 241 | #define rte_bswap32(x) __builtin_bswap32(x) ... This is because "digest" will be initialized by "rte_memcpy" function rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. Furthermore, 'rte_memcpy' will initialize 'digest' with two steps by invoking rte_mov_x functions. For example: partial_hash_sha1 -> rte_memcpy -> rte_memcpy_ge16_lt_128 -> step 1: rte_mov16(dst,src ) step 2: rte_mov16(dst - 16 + n, src - 16 + n) However, gcc compiler cannot identify this multi-step initialization, then it will report warning. To fix this, use "memset" to initialize "digest". Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") Cc: stable@dpdk.org Signed-off-by: Feifei Wang <feifei.wang2@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> --- v2: add check and free for memory dynamic allocation (David Marchand) v3: fix compiler error v4: use 'memset' to initialize digest (Ferruh, Adam) drivers/crypto/qat/qat_sym_session.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c index 231b1640da..e22dd3600c 100644 --- a/drivers/crypto/qat/qat_sym_session.c +++ b/drivers/crypto/qat/qat_sym_session.c @@ -1196,6 +1196,9 @@ static int partial_hash_compute(enum icp_qat_hw_auth_algo hash_alg, uint64_t *hash_state_out_be64; int i; + /* Initialize to avoid gcc warning */ + memset(digest, 0, sizeof(digest)); + digest_size = qat_hash_get_digest_size(hash_alg); if (digest_size <= 0) return -EFAULT; -- 2.25.1
> -----Original Message-----
> From: Feifei Wang <feifei.wang2@arm.com>
> Sent: Thursday, 20 May, 2021 10:44
> To: Griffin, John <john.griffin@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
> Herbert Guan <herbert.guan@arm.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Dybkowski,
> AdamX <adamx.dybkowski@intel.com>; nd@arm.com; Feifei Wang
> <feifei.wang2@arm.com>; stable@dpdk.org; Ruifeng Wang
> <ruifeng.wang@arm.com>
> Subject: [PATCH v4] crypto/qat: fix uninitilized gcc compiler warning
>
> In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, gcc
> will report variable uninitilized warning:
>
> ../drivers/crypto/qat/qat_sym_session.c: In function
> ‘partial_hash_compute’:
> ../lib/eal/include/generic/rte_byteorder.h:241:24: warning:
> ‘<U35a0>’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 241 | #define rte_bswap32(x) __builtin_bswap32(x)
> ...
>
> This is because "digest" will be initialized by "rte_memcpy" function rather
> than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true.
> Furthermore, 'rte_memcpy' will initialize 'digest' with two steps by invoking
> rte_mov_x functions. For example:
>
> partial_hash_sha1 -> rte_memcpy -> rte_memcpy_ge16_lt_128 -> step 1:
> rte_mov16(dst,src ) step 2: rte_mov16(dst - 16 + n, src - 16 + n)
>
> However, gcc compiler cannot identify this multi-step initialization, then it
> will report warning.
>
> To fix this, use "memset" to initialize "digest".
>
> Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy")
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
Acked-by: Adam Dybkowski <adamx.dybkowski@intel.com>
In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, gcc will report variable uninitialized warning: ../drivers/crypto/qat/qat_sym_session.c: In function ‘partial_hash_compute’: ../lib/eal/include/generic/rte_byteorder.h:241:24: warning: ‘<U35a0>’ may be used uninitialized in this function [-Wmaybe-uninitialized] 241 | #define rte_bswap32(x) __builtin_bswap32(x) ... This is because "digest" will be initialized by "rte_memcpy" function rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true. Furthermore, 'rte_memcpy' will initialize 'digest' with two steps by invoking rte_mov_x functions. For example: partial_hash_sha1 -> rte_memcpy -> rte_memcpy_ge16_lt_128 -> step 1: rte_mov16(dst,src ) step 2: rte_mov16(dst - 16 + n, src - 16 + n) However, gcc compiler cannot identify this multi-step initialization, then it will report warning. To fix this, use "memset" to initialize "digest". Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy") Cc: stable@dpdk.org Signed-off-by: Feifei Wang <feifei.wang2@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> Acked-by: Adam Dybkowski <adamx.dybkowski@intel.com> --- v2: add check and free for memory dynamic allocation (David Marchand) v3: fix compiler error v4: use 'memset' to initialize digest (Ferruh, Adam) v5: fixed spelling errors in commit message (Honnappa) drivers/crypto/qat/qat_sym_session.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c index 231b1640da..e22dd3600c 100644 --- a/drivers/crypto/qat/qat_sym_session.c +++ b/drivers/crypto/qat/qat_sym_session.c @@ -1196,6 +1196,9 @@ static int partial_hash_compute(enum icp_qat_hw_auth_algo hash_alg, uint64_t *hash_state_out_be64; int i; + /* Initialize to avoid gcc warning */ + memset(digest, 0, sizeof(digest)); + digest_size = qat_hash_get_digest_size(hash_alg); if (digest_size <= 0) return -EFAULT; -- 2.25.1
> In Arm platform, when "RTE_ARCH_ARM64_MEMCPY" is set as true, gcc will
> report variable uninitialized warning:
>
> ../drivers/crypto/qat/qat_sym_session.c: In function
> ‘partial_hash_compute’:
> ../lib/eal/include/generic/rte_byteorder.h:241:24: warning:
> ‘<U35a0>’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 241 | #define rte_bswap32(x) __builtin_bswap32(x)
> ...
>
> This is because "digest" will be initialized by "rte_memcpy" function
> rather than "memcpy" if "RTE_ARCH_ARM64_MEMCPY" is set as true.
> Furthermore, 'rte_memcpy' will initialize 'digest' with two steps
> by invoking rte_mov_x functions. For example:
>
> partial_hash_sha1 -> rte_memcpy -> rte_memcpy_ge16_lt_128 ->
> step 1: rte_mov16(dst,src )
> step 2: rte_mov16(dst - 16 + n, src - 16 + n)
>
> However, gcc compiler cannot identify this multi-step initialization,
> then it will report warning.
>
> To fix this, use "memset" to initialize "digest".
>
> Fixes: cd7fc8a84b48 ("eal/arm64: optimize memcpy")
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Acked-by: Adam Dybkowski <adamx.dybkowski@intel.com>
> ---
> v2: add check and free for memory dynamic allocation (David Marchand)
> v3: fix compiler error
> v4: use 'memset' to initialize digest (Ferruh, Adam)
> v5: fixed spelling errors in commit message (Honnappa)
>
Applied to dpdk-next-crypto
Thanks.