From: "Coyle, David" <david.coyle@intel.com>
To: "akhil.goyal@nxp.com" <akhil.goyal@nxp.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"mdr@ashroe.eu" <mdr@ashroe.eu>,
"anoobj@marvell.com" <anoobj@marvell.com>
Cc: "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
"Doherty, Declan" <declan.doherty@intel.com>,
"Nicolau, Radu" <radu.nicolau@intel.com>
Subject: Re: [dpdk-dev] [PATCH] security: update session create API
Date: Thu, 24 Sep 2020 16:22:32 +0000 [thread overview]
Message-ID: <MN2PR11MB355088D9C924E31E292C9098E3390@MN2PR11MB3550.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200903200958.28025-1-akhil.goyal@nxp.com>
Hi Akhil
> -----Original Message-----
> From: akhil.goyal@nxp.com <akhil.goyal@nxp.com>
> Sent: Thursday, September 3, 2020 9:10 PM
<snip>
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> 70bf6fe2c..6d7da1408 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -7219,7 +7219,8 @@ test_pdcp_proto(int i, int oop,
>
> /* Create security session */
> ut_params->sec_session = rte_security_session_create(ctx,
> - &sess_conf, ts_params-
> >session_priv_mpool);
> + &sess_conf, ts_params->session_mpool,
> + ts_params->session_priv_mpool);
[DC] ts_params->session_mpool is a cryptodev sym session pool. The assumption then in these security tests is that
security sessions are smaller than cryptodev sym sessions. This is currently true, but may not always be.
There should possibly be a new mempool created for security sessions.
Or at least an assert somewhere to check a security session is smaller than a cryptodev sym session, so that this doesn't
catch someone out in the future if security session grows in size.
The same comment applies to the crypto-perf-test and test_ipsec too
<snip>
> diff --git a/app/test/test_security.c b/app/test/test_security.c index
> 77fd5adc6..ed7de348f 100644
> --- a/app/test/test_security.c
> +++ b/app/test/test_security.c
> @@ -237,6 +237,7 @@ static struct mock_session_create_data {
> struct rte_security_session_conf *conf;
> struct rte_security_session *sess;
> struct rte_mempool *mp;
> + struct rte_mempool *priv_mp;
>
<snip>
> 790,7 +809,7 @@ test_session_create_inv_mempool(void)
> struct rte_security_session *sess;
>
> sess = rte_security_session_create(&ut_params->ctx, &ut_params-
> >conf,
> - NULL);
> + NULL, NULL);
[DC] This test test_session_create_inv_mempool() should have the priv_mp set to a valid
value (i.e. ts_params->session_priv_mpool), and a new test function should be added where
mp is valid, but priv_mp is NULL - this way we test for validity of both mempools independently.
<snip>
> a/doc/guides/prog_guide/rte_security.rst
> b/doc/guides/prog_guide/rte_security.rst
> index 127da2e4f..cff0653f5 100644
> --- a/doc/guides/prog_guide/rte_security.rst
> +++ b/doc/guides/prog_guide/rte_security.rst
> @@ -533,8 +533,10 @@ and this allows further acceleration of the offload of
> Crypto workloads.
>
> The Security framework provides APIs to create and free sessions for
> crypto/ethernet devices, where sessions are mempool objects. It is the
> application's responsibility -to create and manage the session mempools. The
> mempool object size should be able to -accommodate the driver's private
> data of security session.
> +to create and manage two session mempools - one for session and other
> +for session private data. The mempool object size should be able to
> +accommodate the driver's private data of security session. The
> +application can get the size of session private data using API
> ``rte_security_session_get_size``.
[DC] This sentence should be updated to specify it's the private session data mempool that is being referred to
"The mempool object size should be able to accommodate the driver's private data of security session."
=>
"The private session data mempool object size should be able to accommodate the driver's private data of security
session."
Also, a sentence about the required size of the session mempool should also be added.
<snip>
> diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> index df227a177..04c1a1b81 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -84,6 +84,12 @@ API Changes
> Also, make sure to start the actual text at the margin.
> =======================================================
>
> +* security: The API ``rte_security_session_create`` is updated to take
> +two
> + mempool objects one for session and other for session private data.
> + So the application need to create two mempools and get the size of
> +session
> + private data using API ``rte_security_session_get_size`` for private
> +session
> + mempool.
> +
[DC] Many of the PMDs which support security don't implement the session_get_size
callback. There's probably a job here for each PMD owner to add support for this callback.
>
> ABI Changes
> -----------
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-
> secgw/ipsec-secgw.c
> index 8ba15d23c..55a5ea9f4 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
<snip>
> @@ -2379,12 +2375,8 @@ session_priv_pool_init(struct socket_ctx *ctx,
> int32_t socket_id,
>
> snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
> "sess_mp_priv_%u", socket_id);
> - /*
> - * Doubled due to rte_security_session_create() uses one mempool
> for
> - * session and for session private data.
> - */
> nb_sess = (get_nb_crypto_sessions() + CDEV_MP_CACHE_SZ *
> - rte_lcore_count()) * 2;
> + rte_lcore_count());
[DC] A change to double the number of sessions was made in test-crypto-perf when adding DOCSIS security protocol to this tester.
It was needed as both session and private session data was pulled from same mempool.
This change can now be reverted like this...
diff --git a/app/test-crypto-perf/main.c b/app/test-crypto-perf/main.c
index 8f8e580e4..6a71aff5f 100644
--- a/app/test-crypto-perf/main.c
+++ b/app/test-crypto-perf/main.c
@@ -248,7 +248,7 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs)
#endif
} else
sessions_needed = enabled_cdev_count *
- opts->nb_qps * 2;
+ opts->nb_qps;
<snip>
> git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> index 515c29e04..293ca747d 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -26,7 +26,8 @@
> struct rte_security_session *
> rte_security_session_create(struct rte_security_ctx *instance,
> struct rte_security_session_conf *conf,
> - struct rte_mempool *mp)
> + struct rte_mempool *mp,
> + struct rte_mempool *priv_mp)
> {
> struct rte_security_session *sess = NULL;
[DC] Need to add a validity check for priv_mp to rte_security_session_create().
The cryptodev API checks both mp and priv_mp are not NULL, so security should do the same
RTE_PTR_OR_ERR_RET(priv_mp, NULL);
>
<snip>
> --
> 2.17.1
[DC] This API change has highlighted a bug in the security callbacks in the AESNi-MB PMD, specifically in
aesni_mb_pmd_sec_sess_destroy() in drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
Before putting the private session data back to the mempool, this function clears the data with a memset.
But the bug is that it cleared the security session struct instead of the private aesni_mb_session struct.
This didn't show up previously because the elements of the mempool were large, because both security session and private session
data came from the same mempool with large objects . But now that the security session mempool object are much smaller, this causes
a seg fault
The fix is as follows:
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
index 2362f0c3c..b11d7f12b 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c
@@ -911,7 +911,7 @@ aesni_mb_pmd_sec_sess_destroy(void *dev __rte_unused,
if (sess_priv) {
struct rte_mempool *sess_mp = rte_mempool_from_obj(sess_priv);
- memset(sess, 0, sizeof(struct aesni_mb_session));
+ memset(sess_priv, 0, sizeof(struct aesni_mb_session));
set_sec_session_private_data(sess, NULL);
rte_mempool_put(sess_mp, sess_priv);
}
Can this be fixed as part of this patchset or separate fix needed?
next prev parent reply other threads:[~2020-09-24 16:22 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20200903201022eucas1p19a5154b9fb7063f72b18dea1114eeed3@eucas1p1.samsung.com>
2020-09-03 20:09 ` akhil.goyal
2020-09-04 16:04 ` Lukasz Wojciechowski
2020-10-10 22:09 ` Akhil Goyal
2020-09-24 16:22 ` Coyle, David [this message]
2020-10-10 22:06 ` Akhil Goyal
2020-10-10 22:11 ` [dpdk-dev] [PATCH v2] " Akhil Goyal
2020-10-12 17:46 ` Akhil Goyal
2020-10-13 2:12 ` Lukasz Wojciechowski
2020-10-14 19:00 ` Akhil Goyal
2020-10-15 1:14 ` Lukasz Wojciechowski
2020-10-14 18:56 ` [dpdk-dev] [PATCH v3] " Akhil Goyal
2020-10-15 1:11 ` Lukasz Wojciechowski
2020-10-17 11:50 ` [dpdk-dev] [PATCH v4] " Akhil Goyal
2020-10-17 13:13 ` Lukasz Wojciechowski
2020-10-18 8:47 ` Akhil Goyal
2020-10-18 9:30 ` Lukasz Wojciechowski
2020-10-18 9:37 ` Lukasz Wojciechowski
2020-10-18 9:42 ` Akhil Goyal
2020-10-18 9:40 ` [dpdk-dev] [PATCH v5] " Akhil Goyal
2020-10-18 11:03 ` Lukasz Wojciechowski
2020-10-19 7:51 ` Thomas Monjalon
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=MN2PR11MB355088D9C924E31E292C9098E3390@MN2PR11MB3550.namprd11.prod.outlook.com \
--to=david.coyle@intel.com \
--cc=akhil.goyal@nxp.com \
--cc=anoobj@marvell.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=konstantin.ananyev@intel.com \
--cc=mdr@ashroe.eu \
--cc=radu.nicolau@intel.com \
--cc=thomas@monjalon.net \
/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).