* [dpdk-dev] [PATCH] crypto/qat: fix uninitilized compiler warning @ 2021-05-14 7:41 Feifei Wang 2021-05-14 7:49 ` [dpdk-dev] [dpdk-stable] " David Marchand ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Feifei Wang @ 2021-05-14 7:41 UTC (permalink / raw) To: John Griffin, Fiona Trahe, Deepak Kumar Jain, Herbert Guan, Jerin Jacob Cc: dev, nd, Feifei Wang, stable, Ruifeng 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> --- 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH] crypto/qat: fix uninitilized compiler warning 2021-05-14 7:41 [dpdk-dev] [PATCH] crypto/qat: fix uninitilized compiler warning Feifei Wang @ 2021-05-14 7:49 ` David Marchand 2021-05-14 8:01 ` [dpdk-dev] 回复: " Feifei Wang 2021-05-17 9:07 ` [dpdk-dev] [PATCH v3] " Feifei Wang ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: David Marchand @ 2021-05-14 7:49 UTC (permalink / raw) To: Feifei Wang Cc: John Griffin, Fiona Trahe, Deepak Kumar Jain, Herbert Guan, Jerin Jacob, dev, nd, dpdk stable, Ruifeng Wang 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] 回复: [dpdk-stable] [PATCH] crypto/qat: fix uninitilized compiler warning 2021-05-14 7:49 ` [dpdk-dev] [dpdk-stable] " David Marchand @ 2021-05-14 8:01 ` Feifei Wang 2021-05-14 8:13 ` [dpdk-dev] " David Marchand 0 siblings, 1 reply; 17+ messages in thread From: Feifei Wang @ 2021-05-14 8:01 UTC (permalink / raw) To: David Marchand Cc: John Griffin, Fiona Trahe, Deepak Kumar Jain, Herbert Guan, Jerin Jacob, dev, nd, dpdk stable, Ruifeng Wang, nd 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH] crypto/qat: fix uninitilized compiler warning 2021-05-14 8:01 ` [dpdk-dev] 回复: " Feifei Wang @ 2021-05-14 8:13 ` David Marchand 2021-05-14 8:30 ` [dpdk-dev] 回复: " Feifei Wang 0 siblings, 1 reply; 17+ messages in thread From: David Marchand @ 2021-05-14 8:13 UTC (permalink / raw) To: Feifei Wang Cc: John Griffin, Fiona Trahe, Deepak Kumar Jain, Herbert Guan, Jerin Jacob, dev, nd, dpdk stable, Ruifeng Wang 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] 回复: [dpdk-stable] [PATCH] crypto/qat: fix uninitilized compiler warning 2021-05-14 8:13 ` [dpdk-dev] " David Marchand @ 2021-05-14 8:30 ` Feifei Wang 0 siblings, 0 replies; 17+ messages in thread From: Feifei Wang @ 2021-05-14 8:30 UTC (permalink / raw) To: David Marchand Cc: John Griffin, Fiona Trahe, Deepak Kumar Jain, Herbert Guan, Jerin Jacob, dev, nd, dpdk stable, Ruifeng Wang, nd > -----邮件原件----- > 发件人: 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-14 7:41 [dpdk-dev] [PATCH] crypto/qat: fix uninitilized compiler warning Feifei Wang 2021-05-14 7:49 ` [dpdk-dev] [dpdk-stable] " David Marchand @ 2021-05-17 9:07 ` Feifei Wang 2021-05-19 7:56 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon ` (3 more replies) 2021-05-20 8:43 ` [dpdk-dev] [PATCH v4] crypto/qat: fix uninitilized gcc " Feifei Wang 2021-05-21 2:00 ` [dpdk-dev] [PATCH v5] crypto/qat: fix uninitialized " Feifei Wang 3 siblings, 4 replies; 17+ messages in thread From: Feifei Wang @ 2021-05-17 9:07 UTC (permalink / raw) To: John Griffin, Fiona Trahe, Deepak Kumar Jain, Jerin Jacob, Herbert Guan Cc: dev, david.marchand, nd, Feifei Wang, stable, Ruifeng 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 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-17 9:07 ` [dpdk-dev] [PATCH v3] " Feifei Wang @ 2021-05-19 7:56 ` Thomas Monjalon 2021-05-19 8:11 ` Ferruh Yigit ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Thomas Monjalon @ 2021-05-19 7:56 UTC (permalink / raw) To: John Griffin, Fiona Trahe, Deepak Kumar Jain, Jerin Jacob, Herbert Guan Cc: stable, dev, david.marchand, nd, Feifei Wang, Ruifeng Wang 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-17 9:07 ` [dpdk-dev] [PATCH v3] " Feifei Wang 2021-05-19 7:56 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon @ 2021-05-19 8:11 ` Ferruh Yigit 2021-05-20 5:44 ` [dpdk-dev] 回复: " Feifei Wang 2021-05-19 13:13 ` [dpdk-dev] " Dybkowski, AdamX 2021-05-19 13:16 ` Dybkowski, AdamX 3 siblings, 1 reply; 17+ messages in thread From: Ferruh Yigit @ 2021-05-19 8:11 UTC (permalink / raw) To: Feifei Wang, John Griffin, Fiona Trahe, Deepak Kumar Jain, Jerin Jacob, Herbert Guan Cc: dev, david.marchand, nd, stable, Ruifeng Wang 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] 回复: [dpdk-stable] [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-19 8:11 ` Ferruh Yigit @ 2021-05-20 5:44 ` Feifei Wang 2021-05-20 8:08 ` Ferruh Yigit 0 siblings, 1 reply; 17+ messages in thread From: Feifei Wang @ 2021-05-20 5:44 UTC (permalink / raw) To: Ferruh Yigit, John Griffin, Fiona Trahe, Deepak Kumar Jain, Jerin Jacob, Herbert Guan Cc: dev, david.marchand, nd, stable, Ruifeng Wang, nd 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] 回复: [dpdk-stable] [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-20 5:44 ` [dpdk-dev] 回复: " Feifei Wang @ 2021-05-20 8:08 ` Ferruh Yigit 0 siblings, 0 replies; 17+ messages in thread From: Ferruh Yigit @ 2021-05-20 8:08 UTC (permalink / raw) To: Feifei Wang, John Griffin, Fiona Trahe, Deepak Kumar Jain, Jerin Jacob, Herbert Guan Cc: dev, david.marchand, nd, stable, Ruifeng Wang 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. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-17 9:07 ` [dpdk-dev] [PATCH v3] " Feifei Wang 2021-05-19 7:56 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon 2021-05-19 8:11 ` Ferruh Yigit @ 2021-05-19 13:13 ` Dybkowski, AdamX 2021-05-19 13:16 ` Dybkowski, AdamX 3 siblings, 0 replies; 17+ messages in thread From: Dybkowski, AdamX @ 2021-05-19 13:13 UTC (permalink / raw) To: Feifei Wang, Griffin, John, Trahe, Fiona, Jain, Deepak K, Jerin Jacob, Herbert Guan Cc: dev, david.marchand, nd, stable, Ruifeng Wang 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-17 9:07 ` [dpdk-dev] [PATCH v3] " Feifei Wang ` (2 preceding siblings ...) 2021-05-19 13:13 ` [dpdk-dev] " Dybkowski, AdamX @ 2021-05-19 13:16 ` Dybkowski, AdamX 2021-05-20 5:47 ` [dpdk-dev] 回复: " Feifei Wang 3 siblings, 1 reply; 17+ messages in thread From: Dybkowski, AdamX @ 2021-05-19 13:16 UTC (permalink / raw) To: Feifei Wang, Griffin, John, Trahe, Fiona, Jain, Deepak K, Jerin Jacob, Herbert Guan Cc: dev, david.marchand, nd, stable, Ruifeng Wang 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] 回复: [PATCH v3] crypto/qat: fix uninitilized compiler warning 2021-05-19 13:16 ` Dybkowski, AdamX @ 2021-05-20 5:47 ` Feifei Wang 0 siblings, 0 replies; 17+ messages in thread From: Feifei Wang @ 2021-05-20 5:47 UTC (permalink / raw) To: Dybkowski, AdamX, Griffin, John, Trahe, Fiona, Jain, Deepak K, Jerin Jacob, Herbert Guan Cc: dev, david.marchand, nd, stable, Ruifeng Wang, nd 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] [PATCH v4] crypto/qat: fix uninitilized gcc compiler warning 2021-05-14 7:41 [dpdk-dev] [PATCH] crypto/qat: fix uninitilized compiler warning Feifei Wang 2021-05-14 7:49 ` [dpdk-dev] [dpdk-stable] " David Marchand 2021-05-17 9:07 ` [dpdk-dev] [PATCH v3] " Feifei Wang @ 2021-05-20 8:43 ` Feifei Wang 2021-05-20 9:06 ` Dybkowski, AdamX 2021-05-21 2:00 ` [dpdk-dev] [PATCH v5] crypto/qat: fix uninitialized " Feifei Wang 3 siblings, 1 reply; 17+ messages in thread From: Feifei Wang @ 2021-05-20 8:43 UTC (permalink / raw) To: John Griffin, Fiona Trahe, Deepak Kumar Jain, Herbert Guan, Jerin Jacob Cc: dev, ferruh.yigit, adamx.dybkowski, nd, Feifei Wang, stable, Ruifeng Wang 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [PATCH v4] crypto/qat: fix uninitilized gcc compiler warning 2021-05-20 8:43 ` [dpdk-dev] [PATCH v4] crypto/qat: fix uninitilized gcc " Feifei Wang @ 2021-05-20 9:06 ` Dybkowski, AdamX 0 siblings, 0 replies; 17+ messages in thread From: Dybkowski, AdamX @ 2021-05-20 9:06 UTC (permalink / raw) To: Feifei Wang, Griffin, John, Trahe, Fiona, Jain, Deepak K, Herbert Guan, Jerin Jacob Cc: dev, Yigit, Ferruh, nd, stable, Ruifeng Wang > -----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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] [PATCH v5] crypto/qat: fix uninitialized gcc compiler warning 2021-05-14 7:41 [dpdk-dev] [PATCH] crypto/qat: fix uninitilized compiler warning Feifei Wang ` (2 preceding siblings ...) 2021-05-20 8:43 ` [dpdk-dev] [PATCH v4] crypto/qat: fix uninitilized gcc " Feifei Wang @ 2021-05-21 2:00 ` Feifei Wang 2021-06-29 20:04 ` [dpdk-dev] [EXT] " Akhil Goyal 3 siblings, 1 reply; 17+ messages in thread From: Feifei Wang @ 2021-05-21 2:00 UTC (permalink / raw) To: John Griffin, Fiona Trahe, Deepak Kumar Jain, Jerin Jacob, Herbert Guan Cc: dev, ferruh.yigit, adamx.dybkowski, nd, Feifei Wang, stable, Ruifeng Wang, Honnappa Nagarahalli 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v5] crypto/qat: fix uninitialized gcc compiler warning 2021-05-21 2:00 ` [dpdk-dev] [PATCH v5] crypto/qat: fix uninitialized " Feifei Wang @ 2021-06-29 20:04 ` Akhil Goyal 0 siblings, 0 replies; 17+ messages in thread From: Akhil Goyal @ 2021-06-29 20:04 UTC (permalink / raw) To: Feifei Wang, John Griffin, Fiona Trahe, Deepak Kumar Jain, Jerin Jacob, Herbert Guan Cc: dev, ferruh.yigit, adamx.dybkowski, nd, stable, Ruifeng Wang, Honnappa Nagarahalli > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-06-29 20:04 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-14 7:41 [dpdk-dev] [PATCH] crypto/qat: fix uninitilized compiler warning Feifei Wang 2021-05-14 7:49 ` [dpdk-dev] [dpdk-stable] " David Marchand 2021-05-14 8:01 ` [dpdk-dev] 回复: " Feifei Wang 2021-05-14 8:13 ` [dpdk-dev] " David Marchand 2021-05-14 8:30 ` [dpdk-dev] 回复: " Feifei Wang 2021-05-17 9:07 ` [dpdk-dev] [PATCH v3] " Feifei Wang 2021-05-19 7:56 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon 2021-05-19 8:11 ` Ferruh Yigit 2021-05-20 5:44 ` [dpdk-dev] 回复: " Feifei Wang 2021-05-20 8:08 ` Ferruh Yigit 2021-05-19 13:13 ` [dpdk-dev] " Dybkowski, AdamX 2021-05-19 13:16 ` Dybkowski, AdamX 2021-05-20 5:47 ` [dpdk-dev] 回复: " Feifei Wang 2021-05-20 8:43 ` [dpdk-dev] [PATCH v4] crypto/qat: fix uninitilized gcc " Feifei Wang 2021-05-20 9:06 ` Dybkowski, AdamX 2021-05-21 2:00 ` [dpdk-dev] [PATCH v5] crypto/qat: fix uninitialized " Feifei Wang 2021-06-29 20:04 ` [dpdk-dev] [EXT] " Akhil Goyal
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git