From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id EB9D5A04B1; Sun, 11 Oct 2020 00:06:43 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2F4C71D42A; Sun, 11 Oct 2020 00:06:42 +0200 (CEST) Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-eopbgr130087.outbound.protection.outlook.com [40.107.13.87]) by dpdk.org (Postfix) with ESMTP id CA68D1D413 for ; Sun, 11 Oct 2020 00:06:36 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bxv3E6GmR0q9YTixEIqumyjTFCryKDtdNcBYX2D4AWskKy3PR7I7tUrPfJpiw5uPgaiqQgc1ONl/4GGlhijANdZ153QFnpGbnPCOn7Nz56Vlb+8lmjGGFZE5fyXkhfviyWNcEwiryprg5Be1zVrOZVF+uhpZQFV8L5m/fYMihuzjtHacdTLQa56yO7ob5qvTGjN6gDf6dCr+7KWsWk4/HQlKuA7hAJ5+7i4czifq+iFs2wqkV0M41xmbvsnlFtoK9DtbwkoiHIzuRzNKFzySFkvh4vAKLVBQJoa/T00DDuWvJrd2ASukMMY20lmJsJp58ik3LgkBFMwfvU+01GzpzA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1lerrtBwYf9EkkEkGRcEApdmS/CENExn/QpQMZv2UdU=; b=bXusxKuRGgoMh4btl/mUDnXkIo1SMoBfSrTJtQJM4Y1QfNh4Us73+X/C07Zym9qrhWoi1IwZqJXfjiRZg887WTVQu71gYHiF0CvnjuEaQS7bVYXafTXnc+ukuLOe0EmoGUA5m3TLKIrNAc/y00i4iGqNrLL7uc3G1n2EfnnOKEHizJApF5Nng4gjRw/1GNBcHW+KaEF3GmXx5kfM9Ehu4YBf4dzdRfo+Ymv+Ec+vXmjm9K/VKuSYXbxsxi6zwQcUPH+3Vz+PAvXIBNnF69nuDMEClLcQbxFZDJzpKWmMc1Uj5Pe3tYHZDr3N/lSymU3OGDLhPTUaogLg1x+binahIw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com; dkim=pass header.d=nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1lerrtBwYf9EkkEkGRcEApdmS/CENExn/QpQMZv2UdU=; b=oeY2hAg+iNModTkK6QLZBPnW15TUsPloypjz0/FOVO4BHW/T8YT0RupWd6VrbbxjCFbXotHYsIQi1jfQ7ugcHOBX0XHhat4XvOsb8QvbaV6ol8/8q0OtKClh0rrP4F9qn4+AKetSCp7I4bK4qSR/9GLmPX+zK0KrZZ0UKU92hSA= Received: from VI1PR04MB3168.eurprd04.prod.outlook.com (2603:10a6:802:6::10) by VI1PR0402MB2752.eurprd04.prod.outlook.com (2603:10a6:800:b1::8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3455.24; Sat, 10 Oct 2020 22:06:32 +0000 Received: from VI1PR04MB3168.eurprd04.prod.outlook.com ([fe80::9513:3b55:931f:216e]) by VI1PR04MB3168.eurprd04.prod.outlook.com ([fe80::9513:3b55:931f:216e%4]) with mapi id 15.20.3455.028; Sat, 10 Oct 2020 22:06:32 +0000 From: Akhil Goyal To: "Coyle, David" , "dev@dpdk.org" , "thomas@monjalon.net" , "mdr@ashroe.eu" , "anoobj@marvell.com" CC: Hemant Agrawal , "Ananyev, Konstantin" , "Doherty, Declan" , "Nicolau, Radu" Thread-Topic: [PATCH] security: update session create API Thread-Index: AQHWgi47pud7Lvi5P0KvZAQ+zHhWTKl4GXIAgBmDtDA= Date: Sat, 10 Oct 2020 22:06:31 +0000 Message-ID: References: <20200903200958.28025-1-akhil.goyal@nxp.com> In-Reply-To: Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=nxp.com; x-originating-ip: [122.180.231.103] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: a80da15d-ef84-48a4-9030-08d86d68befb x-ms-traffictypediagnostic: VI1PR0402MB2752: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:6790; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: jZH0RxQANgiLqKrx6BgzYKylQCTxfLNuohfFsqg8/kfvi5RwS+CPM4zrnUqo3vfowt1ial0JcjorpQv4AJNUdIKRMAbCqaCaL1IgWingwAhr0g4avL04mU9Y6JWCUcGPIoxD3zT8mxK2HeSiX1/lYjvtkuhIAgRY38gXMnBqq3L0px5wRult0MbhiQdoDeDqzsbDX9dFpjo42wGk6hUuV4r4Nsae8XdpOS0EOmwAuF8xQJiOn+Sc9mKjJ65Esj27wO8/WwS1u97K7AH/70pP2PcWXWQFRRDf9P0kjiUXBg9z1OPABkAMRwnByllOdZNJl9989j6yKMqFmzU9IE6xTg== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:VI1PR04MB3168.eurprd04.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(39860400002)(346002)(136003)(396003)(376002)(55016002)(110136005)(54906003)(8936002)(7696005)(83380400001)(316002)(9686003)(6506007)(26005)(33656002)(86362001)(5660300002)(186003)(15650500001)(71200400001)(66946007)(66476007)(44832011)(52536014)(4326008)(76116006)(2906002)(8676002)(64756008)(66556008)(478600001)(66446008); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: nQtdr/qj9nwlTMYqSD7zkkdWqbPc2lrdgq+XhfmVCFGsRfzCC3hS2pW+rOMccCobCyhA8gk/jvfWL2BgQ+KCsEtGnHfwCwicsCs257wu1pxTu8ShvBZ7n7HwsuehDhfIDxkh9m1xBwC5+SkE5K1EpNA+SIuq0LXNOUIgtXikLs3094XuQ4fEdSvT+5gaHqxBE4EZ2FIcaM9/jl1f6OL+MOUnkoS4XqSLcVYjzWXiX6F+3a+qO2awUgySZGm859ij3g6MsrfqUIr9uhwsSmgHwYaJ1N8cABVLkoJz/ZEpgpIcMQ/LIFx48C7VL4lCd6xTnMVsmVIW0JmK/oHR4cFPg97Dt45VavBL9Npqqm5CnW+trCey5/+P+2VO16B5QEACscQraLFQkMsTcZkvLI3FzrEMRPT7FPAGgfN9aI2y3rOxLzH7hXrBSrWIzE0Y++VGMujSP3yr3pTziKu3XQEsz/z78yhToy69Ht9rv/mZuq+z4UarPNcpGvCaBqEdAg4eg+KCQp/gqwwSUeM4IfkcOze+JIBQjZbBAluus3BreoNduy0x1A9NXCQ3TT1O56kuEyGwU/X/N8V/G8q384xX9gFdisU5Z0+ZXCJqP3A5FeYeJKgpm6iOTIjfeO0Xt6yV1FVOSQBBiEzvPWnj4Y0aAw== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB3168.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: a80da15d-ef84-48a4-9030-08d86d68befb X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Oct 2020 22:06:31.9819 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: FLOgOFZrv7vWnd0nwT/JeCCJM+jE/P94/bBDFOMLyhSMUwYDBC3yhDxui+JEr+duKwe5BEmPcYaWjrpVOExfvA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB2752 Subject: Re: [dpdk-dev] [PATCH] security: update session create API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Archived-At: List-Archive: Hi David, > Hi Akhil >=20 > > -----Original Message----- > > From: akhil.goyal@nxp.com > > Sent: Thursday, September 3, 2020 9:10 PM >=20 > >=20 > > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c inde= x > > 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 =3D rte_security_session_create(ctx, > > - &sess_conf, ts_params- > > >session_priv_mpool); > > + &sess_conf, ts_params->session_mpool, > > + ts_params->session_priv_mpool); >=20 > [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 curren= tly true, > but may not always be. >=20 > There should possibly be a new mempool created for security sessions. > Or at least an assert somewhere to check a security session is smaller th= an a > cryptodev sym session, so that this doesn't > catch someone out in the future if security session grows in size. >=20 > 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. >=20 > >=20 > > 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; > > >=20 > >=20 > > 790,7 +809,7 @@ test_session_create_inv_mempool(void) > > struct rte_security_session *sess; > > > > sess =3D rte_security_session_create(&ut_params->ctx, &ut_params- > > >conf, > > - NULL); > > + NULL, NULL); >=20 > [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 shoul= d 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 obv= ious. Isn't it? >=20 > >=20 > > 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 offloa= d 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``. >=20 > [DC] This sentence should be updated to specify it's the private session = data > mempool that is being referred to >=20 > "The mempool object size should be able to accommodate the driver's priva= te > data of security session." > =3D> > "The private session data mempool object size should be able to accommoda= te > the driver's private data of security > session." >=20 > Also, a sentence about the required size of the session mempool should al= so be > added. Fixed in v2 >=20 > >=20 > > 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. > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D > > > > +* 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. > > + >=20 > [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 f= or this > callback. >=20 If a PMD is supporting rte_security, then it should comply with the APIs wh= ich 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 >=20 > >=20 > > @@ -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 =3D (get_nb_crypto_sessions() + CDEV_MP_CACHE_SZ * > > - rte_lcore_count()) * 2; > > + rte_lcore_count()); >=20 > [DC] A change to double the number of sessions was made in test-crypto-pe= rf > when adding DOCSIS security protocol to this tester. > It was needed as both session and private session data was pulled from sa= me > mempool. > This change can now be reverted like this... Fixed in v2 >=20 > 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 =3D enabled_cdev_count * > - opts->nb_qps * 2; > + opts->nb_qps; >=20 > >=20 > > git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_secu= rity.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 =3D NULL; >=20 > [DC] Need to add a validity check for priv_mp to rte_security_session_cre= ate(). > The cryptodev API checks both mp and priv_mp are not NULL, so security sh= ould > do the same >=20 > RTE_PTR_OR_ERR_RET(priv_mp, NULL); Fixed in v2 >=20 > > >=20 > >=20 > > -- > > 2.17.1 >=20 > [DC] This API change has highlighted a bug in the security callbacks in t= he AESNi- > MB PMD, specifically in > aesni_mb_pmd_sec_sess_destroy() in > drivers/crypto/aesni_mb/rte_aesni_mb_pmd_ops.c >=20 > Before putting the private session data back to the mempool, this functio= n > 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 l= arge, > 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 >=20 > The fix is as follows: >=20 > 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, >=20 > if (sess_priv) { > struct rte_mempool *sess_mp =3D 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); > } >=20 > Can this be fixed as part of this patchset or separate fix needed? This patch is already applied on the tree now.