DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data length
@ 2021-11-05 15:42 Kai Ji
  2021-11-05 16:31 ` Zhang, Roy Fan
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Kai Ji @ 2021-11-05 15:42 UTC (permalink / raw)
  To: dev; +Cc: Kai Ji, pablo.de.lara.guarch, adamx.dybkowski

This patch fixes incorrect data lengths computation in cryptodev
unit test. Previously some data lengths were incorrectly set, which
was insensitive for crypto op unit tets but is critical for raw data
path API unit tests. The patch addressed the issue by setting the
correct data lengths for some tests.

Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
Cc: pablo.de.lara.guarch@intel.com

Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-CMAC")
Cc: adamx.dybkowski@intel.com

Signed-off-by: Kai Ji <kai.ji@intel.com>
---
 app/test/test_cryptodev.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 52457596e2..b926412742 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -4102,9 +4102,9 @@ test_kasumi_decryption(const struct kasumi_test_data *tdata)
 
 	/* Create KASUMI operation */
 	retval = create_wireless_algo_cipher_operation(tdata->cipher_iv.data,
-					tdata->cipher_iv.len,
-					tdata->ciphertext.len,
-					tdata->validCipherOffsetInBits.len);
+			tdata->cipher_iv.len,
+			RTE_ALIGN_CEIL(tdata->validCipherLenInBits.len, 8),
+			tdata->validCipherOffsetInBits.len);
 	if (retval < 0)
 		return retval;
 
@@ -7335,6 +7335,7 @@ test_mixed_auth_cipher(const struct mixed_cipher_auth_test_data *tdata,
 	unsigned int plaintext_len;
 	unsigned int ciphertext_pad_len;
 	unsigned int ciphertext_len;
+	unsigned int data_len;
 
 	struct rte_cryptodev_info dev_info;
 	struct rte_crypto_op *op;
@@ -7395,21 +7396,22 @@ test_mixed_auth_cipher(const struct mixed_cipher_auth_test_data *tdata,
 	plaintext_len = ceil_byte_length(tdata->plaintext.len_bits);
 	ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 16);
 	plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 16);
+	data_len = RTE_MAX(ciphertext_pad_len, plaintext_pad_len);
 
 	if (verify) {
 		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
-				ciphertext_pad_len);
+				data_len);
 		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
 		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
+			rte_pktmbuf_append(ut_params->obuf, data_len);
 		debug_hexdump(stdout, "ciphertext:", ciphertext,
 				ciphertext_len);
 	} else {
 		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
-				plaintext_pad_len);
+				data_len);
 		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
 		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, plaintext_pad_len);
+			rte_pktmbuf_append(ut_params->obuf, data_len);
 		debug_hexdump(stdout, "plaintext:", plaintext, plaintext_len);
 	}
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data length
  2021-11-05 15:42 [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data length Kai Ji
@ 2021-11-05 16:31 ` Zhang, Roy Fan
  2021-11-08  4:05 ` [dpdk-dev] [EXT] " Anoob Joseph
  2021-11-09 10:27 ` [dpdk-dev] [dpdk-dev v2] " Kai Ji
  2 siblings, 0 replies; 10+ messages in thread
From: Zhang, Roy Fan @ 2021-11-05 16:31 UTC (permalink / raw)
  To: Ji, Kai, dev; +Cc: Ji, Kai, De Lara Guarch, Pablo, adamx.dybkowski

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kai Ji
> Sent: Friday, November 5, 2021 3:42 PM
> To: dev@dpdk.org
> Cc: Ji, Kai <kai.ji@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; adamx.dybkowski@intel.com
> Subject: [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data
> length
> 
> This patch fixes incorrect data lengths computation in cryptodev
> unit test. Previously some data lengths were incorrectly set, which
> was insensitive for crypto op unit tets but is critical for raw data
> path API unit tests. The patch addressed the issue by setting the
> correct data lengths for some tests.
> 
> Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
> Cc: pablo.de.lara.guarch@intel.com
> 
> Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-
> CMAC")
> Cc: adamx.dybkowski@intel.com
> 
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix incomplete data length
  2021-11-05 15:42 [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data length Kai Ji
  2021-11-05 16:31 ` Zhang, Roy Fan
@ 2021-11-08  4:05 ` Anoob Joseph
  2021-11-08  4:32   ` Anoob Joseph
  2021-11-09 10:27 ` [dpdk-dev] [dpdk-dev v2] " Kai Ji
  2 siblings, 1 reply; 10+ messages in thread
From: Anoob Joseph @ 2021-11-08  4:05 UTC (permalink / raw)
  To: Kai Ji, dev, Akhil Goyal
  Cc: pablo.de.lara.guarch, adamx.dybkowski, roy.fan.zhang

Hi Kai,

Patch looks good. Wondering if we need same fix in functions such as " test_zuc_auth_cipher()".

We were also hitting this issue when we enabled few additional features in Marvell PMDs. Upon investigation, we realized that this issue would come up for certain packet size combinations if the padded lengths are not same. We observed the issue only with test_mixed_auth_cipher(), which is getting addressed with this patch. Just wondering if you have checked whether other places also would need a fix.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kai Ji
> Sent: Friday, November 5, 2021 9:12 PM
> To: dev@dpdk.org
> Cc: Kai Ji <kai.ji@intel.com>; pablo.de.lara.guarch@intel.com;
> adamx.dybkowski@intel.com
> Subject: [EXT] [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data
> length
> 
> External Email
> 
> ----------------------------------------------------------------------
> This patch fixes incorrect data lengths computation in cryptodev unit test.
> Previously some data lengths were incorrectly set, which was insensitive for
> crypto op unit tets but is critical for raw data path API unit tests. The patch
> addressed the issue by setting the correct data lengths for some tests.
> 
> Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
> Cc: pablo.de.lara.guarch@intel.com
> 
> Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-
> CMAC")
> Cc: adamx.dybkowski@intel.com
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---
>  app/test/test_cryptodev.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> 52457596e2..b926412742 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -4102,9 +4102,9 @@ test_kasumi_decryption(const struct
> kasumi_test_data *tdata)
> 
>  	/* Create KASUMI operation */
>  	retval = create_wireless_algo_cipher_operation(tdata-
> >cipher_iv.data,
> -					tdata->cipher_iv.len,
> -					tdata->ciphertext.len,
> -					tdata->validCipherOffsetInBits.len);
> +			tdata->cipher_iv.len,
> +			RTE_ALIGN_CEIL(tdata->validCipherLenInBits.len, 8),
> +			tdata->validCipherOffsetInBits.len);
>  	if (retval < 0)
>  		return retval;
> 
> @@ -7335,6 +7335,7 @@ test_mixed_auth_cipher(const struct
> mixed_cipher_auth_test_data *tdata,
>  	unsigned int plaintext_len;
>  	unsigned int ciphertext_pad_len;
>  	unsigned int ciphertext_len;
> +	unsigned int data_len;
> 
>  	struct rte_cryptodev_info dev_info;
>  	struct rte_crypto_op *op;
> @@ -7395,21 +7396,22 @@ test_mixed_auth_cipher(const struct
> mixed_cipher_auth_test_data *tdata,
>  	plaintext_len = ceil_byte_length(tdata->plaintext.len_bits);
>  	ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 16);
>  	plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 16);
> +	data_len = RTE_MAX(ciphertext_pad_len, plaintext_pad_len);
> 
>  	if (verify) {
>  		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params-
> >ibuf,
> -				ciphertext_pad_len);
> +				data_len);
>  		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
>  		if (op_mode == OUT_OF_PLACE)
> -			rte_pktmbuf_append(ut_params->obuf,
> ciphertext_pad_len);
> +			rte_pktmbuf_append(ut_params->obuf, data_len);
>  		debug_hexdump(stdout, "ciphertext:", ciphertext,
>  				ciphertext_len);
>  	} else {
>  		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params-
> >ibuf,
> -				plaintext_pad_len);
> +				data_len);
>  		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
>  		if (op_mode == OUT_OF_PLACE)
> -			rte_pktmbuf_append(ut_params->obuf,
> plaintext_pad_len);
> +			rte_pktmbuf_append(ut_params->obuf, data_len);
>  		debug_hexdump(stdout, "plaintext:", plaintext,
> plaintext_len);
>  	}
> 
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix incomplete data length
  2021-11-08  4:05 ` [dpdk-dev] [EXT] " Anoob Joseph
@ 2021-11-08  4:32   ` Anoob Joseph
  2021-11-09  0:52     ` Ji, Kai
  0 siblings, 1 reply; 10+ messages in thread
From: Anoob Joseph @ 2021-11-08  4:32 UTC (permalink / raw)
  To: Anoob Joseph, Kai Ji, dev, Akhil Goyal
  Cc: pablo.de.lara.guarch, adamx.dybkowski, roy.fan.zhang

Hi Kai,

Also, couple of nits. Please check inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> Sent: Monday, November 8, 2021 9:36 AM
> To: Kai Ji <kai.ji@intel.com>; dev@dpdk.org; Akhil Goyal
> <gakhil@marvell.com>
> Cc: pablo.de.lara.guarch@intel.com; adamx.dybkowski@intel.com;
> roy.fan.zhang@intel.com
> Subject: Re: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix incomplete
> data length
> 
> Hi Kai,
> 
> Patch looks good. Wondering if we need same fix in functions such as "
> test_zuc_auth_cipher()".
> 
> We were also hitting this issue when we enabled few additional features in
> Marvell PMDs. Upon investigation, we realized that this issue would come up
> for certain packet size combinations if the padded lengths are not same. We
> observed the issue only with test_mixed_auth_cipher(), which is getting
> addressed with this patch. Just wondering if you have checked whether
> other places also would need a fix.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Kai Ji
> > Sent: Friday, November 5, 2021 9:12 PM
> > To: dev@dpdk.org
> > Cc: Kai Ji <kai.ji@intel.com>; pablo.de.lara.guarch@intel.com;
> > adamx.dybkowski@intel.com
> > Subject: [EXT] [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete
> > data length
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > This patch fixes incorrect data lengths computation in cryptodev unit test.
> > Previously some data lengths were incorrectly set, which was
> > insensitive for crypto op unit tets but is critical for raw data path
> > API unit tests. The patch addressed the issue by setting the correct data
> lengths for some tests.
> >
> > Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless
> > algorithms")
> > Cc: pablo.de.lara.guarch@intel.com
> >
> > Fixes: e847fc512817 ("test/crypto: add encrypted digest case for
> > AES-CTR-
> > CMAC")
> > Cc: adamx.dybkowski@intel.com
> >
> > Signed-off-by: Kai Ji <kai.ji@intel.com>
> > ---
> >  app/test/test_cryptodev.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index
> > 52457596e2..b926412742 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -4102,9 +4102,9 @@ test_kasumi_decryption(const struct
> > kasumi_test_data *tdata)
> >
> >  	/* Create KASUMI operation */
> >  	retval = create_wireless_algo_cipher_operation(tdata-
> > >cipher_iv.data,
> > -					tdata->cipher_iv.len,
> > -					tdata->ciphertext.len,
> > -					tdata->validCipherOffsetInBits.len);
> > +			tdata->cipher_iv.len,
> > +			RTE_ALIGN_CEIL(tdata->validCipherLenInBits.len, 8),
> > +			tdata->validCipherOffsetInBits.len);
> >  	if (retval < 0)
> >  		return retval;
> >
> > @@ -7335,6 +7335,7 @@ test_mixed_auth_cipher(const struct
> > mixed_cipher_auth_test_data *tdata,
> >  	unsigned int plaintext_len;
> >  	unsigned int ciphertext_pad_len;
> >  	unsigned int ciphertext_len;
> > +	unsigned int data_len;
> >
> >  	struct rte_cryptodev_info dev_info;
> >  	struct rte_crypto_op *op;
> > @@ -7395,21 +7396,22 @@ test_mixed_auth_cipher(const struct
> > mixed_cipher_auth_test_data *tdata,
> >  	plaintext_len = ceil_byte_length(tdata->plaintext.len_bits);
> >  	ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 16);
> >  	plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 16);
> > +	data_len = RTE_MAX(ciphertext_pad_len, plaintext_pad_len);

[Anoob] Isn't ciphertext_pad_len guaranteed to be the larger one of the two? Do we need another variable and the RTE_MAX?
 
> >
> >  	if (verify) {
> >  		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params-
> > >ibuf,
> > -				ciphertext_pad_len);
> > +				data_len);
> >  		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
> >  		if (op_mode == OUT_OF_PLACE)
> > -			rte_pktmbuf_append(ut_params->obuf,
> > ciphertext_pad_len);
> > +			rte_pktmbuf_append(ut_params->obuf, data_len);
> >  		debug_hexdump(stdout, "ciphertext:", ciphertext,
> >  				ciphertext_len);
> >  	} else {
> >  		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params-
> > >ibuf,
> > -				plaintext_pad_len);
> > +				data_len);
> >  		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
> >  		if (op_mode == OUT_OF_PLACE)
> > -			rte_pktmbuf_append(ut_params->obuf,
> > plaintext_pad_len);
> > +			rte_pktmbuf_append(ut_params->obuf, data_len);

[Anoob] Now that more things are common across the branches, can we move out some bits outside the if condition? Like, the above line is definitely same and be kept outside condition. The append call prior to this can also be kept common if we can rename the local variable.
 
> >  		debug_hexdump(stdout, "plaintext:", plaintext,
> plaintext_len);
> >  	}
> >
> > --
> > 2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix incomplete data length
  2021-11-08  4:32   ` Anoob Joseph
@ 2021-11-09  0:52     ` Ji, Kai
  0 siblings, 0 replies; 10+ messages in thread
From: Ji, Kai @ 2021-11-09  0:52 UTC (permalink / raw)
  To: Anoob Joseph, dev, Akhil Goyal
  Cc: De Lara Guarch, Pablo, adamx.dybkowski, Zhang, Roy Fan

Hi Anoob, 

Please see my commit inline below. 

Thanks Kai 

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Monday, November 8, 2021 4:32 AM
> To: Anoob Joseph <anoobj@marvell.com>; Ji, Kai <kai.ji@intel.com>;
> dev@dpdk.org; Akhil Goyal <gakhil@marvell.com>
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> adamx.dybkowski@intel.com; Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix incomplete
> data length
> 
> Hi Kai,
> 
> Also, couple of nits. Please check inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> > Sent: Monday, November 8, 2021 9:36 AM
> > To: Kai Ji <kai.ji@intel.com>; dev@dpdk.org; Akhil Goyal
> > <gakhil@marvell.com>
> > Cc: pablo.de.lara.guarch@intel.com; adamx.dybkowski@intel.com;
> > roy.fan.zhang@intel.com
> > Subject: Re: [dpdk-dev] [EXT] [dpdk-dev v1] test/cryptodev: fix
> > incomplete data length
> >
> > Hi Kai,
> >
> > Patch looks good. Wondering if we need same fix in functions such as "
> > test_zuc_auth_cipher()".
> >
> > We were also hitting this issue when we enabled few additional
> > features in Marvell PMDs. Upon investigation, we realized that this
> > issue would come up for certain packet size combinations if the padded
> > lengths are not same. We observed the issue only with
> > test_mixed_auth_cipher(), which is getting addressed with this patch.
> > Just wondering if you have checked whether other places also would need
> a fix.
> >

[Kai] Yes, this fix should apply to test_zuc_auth_cipher().  However,  I don't see any zuc test failed from my side as there  is no zuc test vector to cover the OOP partial digest case. I think I will add this fix into the zuc test anyway.   

> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Kai Ji
> > > Sent: Friday, November 5, 2021 9:12 PM
> > > To: dev@dpdk.org
> > > Cc: Kai Ji <kai.ji@intel.com>; pablo.de.lara.guarch@intel.com;
> > > adamx.dybkowski@intel.com
> > > Subject: [EXT] [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix
> > > incomplete data length
> > >
> > > External Email
> > >
> > > --------------------------------------------------------------------
> > > -- This patch fixes incorrect data lengths computation in cryptodev
> > > unit test.
> > > Previously some data lengths were incorrectly set, which was
> > > insensitive for crypto op unit tets but is critical for raw data
> > > path API unit tests. The patch addressed the issue by setting the
> > > correct data
> > lengths for some tests.
> > >
> > > Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless
> > > algorithms")
> > > Cc: pablo.de.lara.guarch@intel.com
> > >
> > > Fixes: e847fc512817 ("test/crypto: add encrypted digest case for
> > > AES-CTR-
> > > CMAC")
> > > Cc: adamx.dybkowski@intel.com
> > >
> > > Signed-off-by: Kai Ji <kai.ji@intel.com>
> > > ---
<snip>
> > > @@ -7335,6 +7335,7 @@ test_mixed_auth_cipher(const struct
> > > mixed_cipher_auth_test_data *tdata,
> > >  	unsigned int plaintext_len;
> > >  	unsigned int ciphertext_pad_len;
> > >  	unsigned int ciphertext_len;
> > > +	unsigned int data_len;
> > >
> > >  	struct rte_cryptodev_info dev_info;
> > >  	struct rte_crypto_op *op;
> > > @@ -7395,21 +7396,22 @@ test_mixed_auth_cipher(const struct
> > > mixed_cipher_auth_test_data *tdata,
> > >  	plaintext_len = ceil_byte_length(tdata->plaintext.len_bits);
> > >  	ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 16);
> > >  	plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 16);
> > > +	data_len = RTE_MAX(ciphertext_pad_len, plaintext_pad_len);
> 
> [Anoob] Isn't ciphertext_pad_len guaranteed to be the larger one of the two?
> Do we need another variable and the RTE_MAX?

[Kai] the ciphertext_pad_len should be always bigger than plaintext_pad_len,  code changed in v2. 

> 
> > >
> > >  	if (verify) {
> > >  		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params-
> > > >ibuf,
> > > -				ciphertext_pad_len);
> > > +				data_len);
> > >  		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
> > >  		if (op_mode == OUT_OF_PLACE)
> > > -			rte_pktmbuf_append(ut_params->obuf,
> > > ciphertext_pad_len);
> > > +			rte_pktmbuf_append(ut_params->obuf, data_len);
> > >  		debug_hexdump(stdout, "ciphertext:", ciphertext,
> > >  				ciphertext_len);
> > >  	} else {
> > >  		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params-
> > > >ibuf,
> > > -				plaintext_pad_len);
> > > +				data_len);
> > >  		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
> > >  		if (op_mode == OUT_OF_PLACE)
> > > -			rte_pktmbuf_append(ut_params->obuf,
> > > plaintext_pad_len);
> > > +			rte_pktmbuf_append(ut_params->obuf, data_len);
> 
> [Anoob] Now that more things are common across the branches, can we
> move out some bits outside the if condition? Like, the above line is definitely
> same and be kept outside condition. The append call prior to this can also be
> kept common if we can rename the local variable.

[Kai] code changed in v2 

> 
> > >  		debug_hexdump(stdout, "plaintext:", plaintext,
> > plaintext_len);
> > >  	}
> > >
> > > --
> > > 2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [dpdk-dev v2] test/cryptodev: fix incomplete data length
  2021-11-05 15:42 [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data length Kai Ji
  2021-11-05 16:31 ` Zhang, Roy Fan
  2021-11-08  4:05 ` [dpdk-dev] [EXT] " Anoob Joseph
@ 2021-11-09 10:27 ` Kai Ji
  2021-11-09 10:32   ` [dpdk-dev] [EXT] " Anoob Joseph
  2021-11-09 10:42   ` [dpdk-dev] [dpdk-dev v3] " Kai Ji
  2 siblings, 2 replies; 10+ messages in thread
From: Kai Ji @ 2021-11-09 10:27 UTC (permalink / raw)
  To: dev; +Cc: Kai Ji, pablo.de.lara.guarch, adamx.dybkowski, damianx.nowak

This patch fixes incorrect data lengths computation in cryptodev
unit test. Previously some data lengths were incorrectly set, which
was insensitive for crypto op unit tets but is critical for raw data
path API unit tests. The patch addressed the issue by setting the
correct data lengths for some tests.

Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
Cc: pablo.de.lara.guarch@intel.com

Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-CMAC")
Cc: adamx.dybkowski@intel.com

Fixes: b1c1df46878d ("test/crypto: add ZUC test cases for auth-cipher")
Cc: damianx.nowak@intel.com

Signed-off-by: Kai Ji <kai.ji@intel.com>
---
 app/test/test_cryptodev.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 52457596e2..2dd1bbb64c 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -4102,9 +4102,9 @@ test_kasumi_decryption(const struct kasumi_test_data *tdata)
 
 	/* Create KASUMI operation */
 	retval = create_wireless_algo_cipher_operation(tdata->cipher_iv.data,
-					tdata->cipher_iv.len,
-					tdata->ciphertext.len,
-					tdata->validCipherOffsetInBits.len);
+			tdata->cipher_iv.len,
+			RTE_ALIGN_CEIL(tdata->validCipherLenInBits.len, 8),
+			tdata->validCipherOffsetInBits.len);
 	if (retval < 0)
 		return retval;
 
@@ -6332,20 +6332,20 @@ test_zuc_auth_cipher(const struct wireless_test_data *tdata,
 		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
 					ciphertext_pad_len);
 		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
 		debug_hexdump(stdout, "ciphertext:", ciphertext,
 			ciphertext_len);
 	} else {
+		/* make sure enough space to cover partial digest verify case */
 		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
-					plaintext_pad_len);
+					ciphertext_pad_len);
 		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, plaintext_pad_len);
 		debug_hexdump(stdout, "plaintext:", plaintext,
 			plaintext_len);
 	}
 
+	if (op_mode == OUT_OF_PLACE)
+		rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
+
 	/* Create ZUC operation */
 	retval = create_wireless_algo_auth_cipher_operation(
 		tdata->digest.data, tdata->digest.len,
@@ -7395,24 +7395,26 @@ test_mixed_auth_cipher(const struct mixed_cipher_auth_test_data *tdata,
 	plaintext_len = ceil_byte_length(tdata->plaintext.len_bits);
 	ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 16);
 	plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 16);
+	if (ciphertext_pad_len > plaintext_pad_len)
+		printf("NOOB: \n");
 
 	if (verify) {
 		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
 				ciphertext_pad_len);
 		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
 		debug_hexdump(stdout, "ciphertext:", ciphertext,
 				ciphertext_len);
 	} else {
+		/* make sure enough space to cover partial digest verify case */
 		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
-				plaintext_pad_len);
+				ciphertext_pad_len);
 		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, plaintext_pad_len);
 		debug_hexdump(stdout, "plaintext:", plaintext, plaintext_len);
 	}
 
+	if (op_mode == OUT_OF_PLACE)
+		rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
+
 	/* Create the operation */
 	retval = create_wireless_algo_auth_cipher_operation(
 			tdata->digest_enc.data, tdata->digest_enc.len,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [dpdk-dev v2] test/cryptodev: fix incomplete data length
  2021-11-09 10:27 ` [dpdk-dev] [dpdk-dev v2] " Kai Ji
@ 2021-11-09 10:32   ` Anoob Joseph
  2021-11-09 10:42   ` [dpdk-dev] [dpdk-dev v3] " Kai Ji
  1 sibling, 0 replies; 10+ messages in thread
From: Anoob Joseph @ 2021-11-09 10:32 UTC (permalink / raw)
  To: Kai Ji, dev; +Cc: pablo.de.lara.guarch, adamx.dybkowski, damianx.nowak

Hi Kai,

Minor nit inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kai Ji
> Sent: Tuesday, November 9, 2021 3:57 PM
> To: dev@dpdk.org
> Cc: Kai Ji <kai.ji@intel.com>; pablo.de.lara.guarch@intel.com;
> adamx.dybkowski@intel.com; damianx.nowak@intel.com
> Subject: [EXT] [dpdk-dev] [dpdk-dev v2] test/cryptodev: fix incomplete data
> length
> 
> External Email
> 
> ----------------------------------------------------------------------
> This patch fixes incorrect data lengths computation in cryptodev unit test.
> Previously some data lengths were incorrectly set, which was insensitive for
> crypto op unit tets but is critical for raw data path API unit tests. The patch
> addressed the issue by setting the correct data lengths for some tests.
> 
> Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
> Cc: pablo.de.lara.guarch@intel.com
> 
> Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-
> CMAC")
> Cc: adamx.dybkowski@intel.com
> 
> Fixes: b1c1df46878d ("test/crypto: add ZUC test cases for auth-cipher")
> Cc: damianx.nowak@intel.com
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---
>  app/test/test_cryptodev.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> 52457596e2..2dd1bbb64c 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -4102,9 +4102,9 @@ test_kasumi_decryption(const struct
> kasumi_test_data *tdata)
> 
>  	/* Create KASUMI operation */
>  	retval = create_wireless_algo_cipher_operation(tdata-
> >cipher_iv.data,
> -					tdata->cipher_iv.len,
> -					tdata->ciphertext.len,
> -					tdata->validCipherOffsetInBits.len);
> +			tdata->cipher_iv.len,
> +			RTE_ALIGN_CEIL(tdata->validCipherLenInBits.len, 8),
> +			tdata->validCipherOffsetInBits.len);
>  	if (retval < 0)
>  		return retval;
> 
> @@ -6332,20 +6332,20 @@ test_zuc_auth_cipher(const struct
> wireless_test_data *tdata,
>  		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params-
> >ibuf,
>  					ciphertext_pad_len);
>  		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
> -		if (op_mode == OUT_OF_PLACE)
> -			rte_pktmbuf_append(ut_params->obuf,
> ciphertext_pad_len);
>  		debug_hexdump(stdout, "ciphertext:", ciphertext,
>  			ciphertext_len);
>  	} else {
> +		/* make sure enough space to cover partial digest verify case
> */
>  		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params-
> >ibuf,
> -					plaintext_pad_len);
> +					ciphertext_pad_len);
>  		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
> -		if (op_mode == OUT_OF_PLACE)
> -			rte_pktmbuf_append(ut_params->obuf,
> plaintext_pad_len);
>  		debug_hexdump(stdout, "plaintext:", plaintext,
>  			plaintext_len);
>  	}
> 
> +	if (op_mode == OUT_OF_PLACE)
> +		rte_pktmbuf_append(ut_params->obuf,
> ciphertext_pad_len);
> +
>  	/* Create ZUC operation */
>  	retval = create_wireless_algo_auth_cipher_operation(
>  		tdata->digest.data, tdata->digest.len, @@ -7395,24 +7395,26
> @@ test_mixed_auth_cipher(const struct mixed_cipher_auth_test_data
> *tdata,
>  	plaintext_len = ceil_byte_length(tdata->plaintext.len_bits);
>  	ciphertext_pad_len = RTE_ALIGN_CEIL(ciphertext_len, 16);
>  	plaintext_pad_len = RTE_ALIGN_CEIL(plaintext_len, 16);
> +	if (ciphertext_pad_len > plaintext_pad_len)
> +		printf("NOOB: \n");

[Anoob] Is the above intentional?
 
> 
>  	if (verify) {
>  		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params-
> >ibuf,
>  				ciphertext_pad_len);
>  		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
> -		if (op_mode == OUT_OF_PLACE)
> -			rte_pktmbuf_append(ut_params->obuf,
> ciphertext_pad_len);
>  		debug_hexdump(stdout, "ciphertext:", ciphertext,
>  				ciphertext_len);
>  	} else {
> +		/* make sure enough space to cover partial digest verify case
> */
>  		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params-
> >ibuf,
> -				plaintext_pad_len);
> +				ciphertext_pad_len);
>  		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
> -		if (op_mode == OUT_OF_PLACE)
> -			rte_pktmbuf_append(ut_params->obuf,
> plaintext_pad_len);
>  		debug_hexdump(stdout, "plaintext:", plaintext,
> plaintext_len);
>  	}
> 
> +	if (op_mode == OUT_OF_PLACE)
> +		rte_pktmbuf_append(ut_params->obuf,
> ciphertext_pad_len);
> +
>  	/* Create the operation */
>  	retval = create_wireless_algo_auth_cipher_operation(
>  			tdata->digest_enc.data, tdata->digest_enc.len,
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [dpdk-dev v3] test/cryptodev: fix incomplete data length
  2021-11-09 10:27 ` [dpdk-dev] [dpdk-dev v2] " Kai Ji
  2021-11-09 10:32   ` [dpdk-dev] [EXT] " Anoob Joseph
@ 2021-11-09 10:42   ` Kai Ji
  2021-11-09 16:25     ` [dpdk-dev] [EXT] " Anoob Joseph
  2021-11-11 11:17     ` [EXT] [dpdk-dev] " Akhil Goyal
  1 sibling, 2 replies; 10+ messages in thread
From: Kai Ji @ 2021-11-09 10:42 UTC (permalink / raw)
  To: dev; +Cc: Kai Ji, pablo.de.lara.guarch, adamx.dybkowski, damianx.nowak

This patch fixes incorrect data lengths computation in cryptodev
unit test. Previously some data lengths were incorrectly set, which
was insensitive for crypto op unit tets but is critical for raw data
path API unit tests. The patch addressed the issue by setting the
correct data lengths for some tests.

Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
Cc: pablo.de.lara.guarch@intel.com

Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-CMAC")
Cc: adamx.dybkowski@intel.com

Fixes: b1c1df46878d ("test/crypto: add ZUC test cases for auth-cipher")
Cc: damianx.nowak@intel.com

Signed-off-by: Kai Ji <kai.ji@intel.com>
---

v2:
- review comments addressed

v3:
- remove unneeded debug code

 app/test/test_cryptodev.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 52457596e2..52e975b74c 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -4102,9 +4102,9 @@ test_kasumi_decryption(const struct kasumi_test_data *tdata)

 	/* Create KASUMI operation */
 	retval = create_wireless_algo_cipher_operation(tdata->cipher_iv.data,
-					tdata->cipher_iv.len,
-					tdata->ciphertext.len,
-					tdata->validCipherOffsetInBits.len);
+			tdata->cipher_iv.len,
+			RTE_ALIGN_CEIL(tdata->validCipherLenInBits.len, 8),
+			tdata->validCipherOffsetInBits.len);
 	if (retval < 0)
 		return retval;

@@ -6332,20 +6332,20 @@ test_zuc_auth_cipher(const struct wireless_test_data *tdata,
 		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
 					ciphertext_pad_len);
 		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
 		debug_hexdump(stdout, "ciphertext:", ciphertext,
 			ciphertext_len);
 	} else {
+		/* make sure enough space to cover partial digest verify case */
 		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
-					plaintext_pad_len);
+					ciphertext_pad_len);
 		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, plaintext_pad_len);
 		debug_hexdump(stdout, "plaintext:", plaintext,
 			plaintext_len);
 	}

+	if (op_mode == OUT_OF_PLACE)
+		rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
+
 	/* Create ZUC operation */
 	retval = create_wireless_algo_auth_cipher_operation(
 		tdata->digest.data, tdata->digest.len,
@@ -7400,19 +7400,19 @@ test_mixed_auth_cipher(const struct mixed_cipher_auth_test_data *tdata,
 		ciphertext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
 				ciphertext_pad_len);
 		memcpy(ciphertext, tdata->ciphertext.data, ciphertext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
 		debug_hexdump(stdout, "ciphertext:", ciphertext,
 				ciphertext_len);
 	} else {
+		/* make sure enough space to cover partial digest verify case */
 		plaintext = (uint8_t *)rte_pktmbuf_append(ut_params->ibuf,
-				plaintext_pad_len);
+				ciphertext_pad_len);
 		memcpy(plaintext, tdata->plaintext.data, plaintext_len);
-		if (op_mode == OUT_OF_PLACE)
-			rte_pktmbuf_append(ut_params->obuf, plaintext_pad_len);
 		debug_hexdump(stdout, "plaintext:", plaintext, plaintext_len);
 	}

+	if (op_mode == OUT_OF_PLACE)
+		rte_pktmbuf_append(ut_params->obuf, ciphertext_pad_len);
+
 	/* Create the operation */
 	retval = create_wireless_algo_auth_cipher_operation(
 			tdata->digest_enc.data, tdata->digest_enc.len,
--
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [dpdk-dev v3] test/cryptodev: fix incomplete data length
  2021-11-09 10:42   ` [dpdk-dev] [dpdk-dev v3] " Kai Ji
@ 2021-11-09 16:25     ` Anoob Joseph
  2021-11-11 11:17     ` [EXT] [dpdk-dev] " Akhil Goyal
  1 sibling, 0 replies; 10+ messages in thread
From: Anoob Joseph @ 2021-11-09 16:25 UTC (permalink / raw)
  To: Kai Ji, dev; +Cc: pablo.de.lara.guarch, adamx.dybkowski, damianx.nowak

Hi Kai,

Thanks for addressing the comments.

Acked-by: Anoob Joseph <anoobj@marvell.com>

> 
> ----------------------------------------------------------------------
> This patch fixes incorrect data lengths computation in cryptodev unit test.
> Previously some data lengths were incorrectly set, which was insensitive for
> crypto op unit tets but is critical for raw data path API unit tests. The patch
> addressed the issue by setting the correct data lengths for some tests.
> 
> Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
> Cc: pablo.de.lara.guarch@intel.com
> 
> Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-
> CMAC")
> Cc: adamx.dybkowski@intel.com
> 
> Fixes: b1c1df46878d ("test/crypto: add ZUC test cases for auth-cipher")
> Cc: damianx.nowak@intel.com
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---
> 
> v2:
> - review comments addressed
> 
> v3:
> - remove unneeded debug code
> 
>  app/test/test_cryptodev.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [EXT] [dpdk-dev] [dpdk-dev v3] test/cryptodev: fix incomplete data length
  2021-11-09 10:42   ` [dpdk-dev] [dpdk-dev v3] " Kai Ji
  2021-11-09 16:25     ` [dpdk-dev] [EXT] " Anoob Joseph
@ 2021-11-11 11:17     ` Akhil Goyal
  1 sibling, 0 replies; 10+ messages in thread
From: Akhil Goyal @ 2021-11-11 11:17 UTC (permalink / raw)
  To: Kai Ji, dev
  Cc: pablo.de.lara.guarch, adamx.dybkowski, damianx.nowak, dpdk stable

> This patch fixes incorrect data lengths computation in cryptodev
> unit test. Previously some data lengths were incorrectly set, which
> was insensitive for crypto op unit tets but is critical for raw data
> path API unit tests. The patch addressed the issue by setting the
> correct data lengths for some tests.
> 
> Fixes: 681f540da52b ("cryptodev: do not use AAD in wireless algorithms")
> Cc: pablo.de.lara.guarch@intel.com
> 
> Fixes: e847fc512817 ("test/crypto: add encrypted digest case for AES-CTR-
> CMAC")
> Cc: adamx.dybkowski@intel.com
> 
> Fixes: b1c1df46878d ("test/crypto: add ZUC test cases for auth-cipher")
> Cc: damianx.nowak@intel.com
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---
Applied to dpdk-next-crypto

Cc: stable@dpdk.org


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-11-11 11:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05 15:42 [dpdk-dev] [dpdk-dev v1] test/cryptodev: fix incomplete data length Kai Ji
2021-11-05 16:31 ` Zhang, Roy Fan
2021-11-08  4:05 ` [dpdk-dev] [EXT] " Anoob Joseph
2021-11-08  4:32   ` Anoob Joseph
2021-11-09  0:52     ` Ji, Kai
2021-11-09 10:27 ` [dpdk-dev] [dpdk-dev v2] " Kai Ji
2021-11-09 10:32   ` [dpdk-dev] [EXT] " Anoob Joseph
2021-11-09 10:42   ` [dpdk-dev] [dpdk-dev v3] " Kai Ji
2021-11-09 16:25     ` [dpdk-dev] [EXT] " Anoob Joseph
2021-11-11 11:17     ` [EXT] [dpdk-dev] " Akhil Goyal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).