From: Akhil Goyal <akhil.goyal@nxp.com> To: "Coyle, David" <david.coyle@intel.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 <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: Sat, 10 Oct 2020 22:06:31 +0000 Message-ID: <VI1PR04MB31684CEDDC74497D3F842F37E6090@VI1PR04MB3168.eurprd04.prod.outlook.com> (raw) In-Reply-To: <MN2PR11MB355088D9C924E31E292C9098E3390@MN2PR11MB3550.namprd11.prod.outlook.com> Hi David, > 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 Fixed for test and crypto-perf. Test_ipsec is not exactly using a security session. Fixing that is out of scope of this patch. > > <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. I would say that would be an overkill with not much gain. Both mempool should be created before session is created. That is quite obvious. Isn't it? > > <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. Fixed in v2 > > <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. > If a PMD is supporting rte_security, then it should comply with the APIs which are required. > > > > 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... Fixed in v2 > > 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); Fixed in v2 > > > > > <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? This patch is already applied on the tree now.
next prev parent reply other threads:[~2020-10-10 22:06 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 2020-10-10 22:06 ` Akhil Goyal [this message] 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=VI1PR04MB31684CEDDC74497D3F842F37E6090@VI1PR04MB3168.eurprd04.prod.outlook.com \ --to=akhil.goyal@nxp.com \ --cc=anoobj@marvell.com \ --cc=david.coyle@intel.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
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