From: "Ji, Kai" <kai.ji@intel.com>
To: Anoob Joseph <anoobj@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>,
"Akhil Goyal" <gakhil@marvell.com>
Cc: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
"adamx.dybkowski@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
Date: Tue, 9 Nov 2021 00:52:51 +0000 [thread overview]
Message-ID: <SN6PR11MB34083EA4BE83BE843A31CD3681929@SN6PR11MB3408.namprd11.prod.outlook.com> (raw)
In-Reply-To: <PH0PR18MB4672144D34D7AD9B311968B4DF919@PH0PR18MB4672.namprd18.prod.outlook.com>
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
next prev parent reply other threads:[~2021-11-09 0:52 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-05 15:42 [dpdk-dev] " 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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR11MB34083EA4BE83BE843A31CD3681929@SN6PR11MB3408.namprd11.prod.outlook.com \
--to=kai.ji@intel.com \
--cc=adamx.dybkowski@intel.com \
--cc=anoobj@marvell.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=pablo.de.lara.guarch@intel.com \
--cc=roy.fan.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).