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 B4E02A04B6; Thu, 24 Sep 2020 18:22:42 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 074191DEC6; Thu, 24 Sep 2020 18:22:42 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id CAB6C1C438 for ; Thu, 24 Sep 2020 18:22:39 +0200 (CEST) IronPort-SDR: 0tvUu7e6CG3FswMsgnWgwBILjOwWK+Wrdc3RL57dhUqVnvcQf/KB7Wlazn2Pg4YaomjYmLcEDF o2FuqHBQ28VQ== X-IronPort-AV: E=McAfee;i="6000,8403,9753"; a="179343268" X-IronPort-AV: E=Sophos;i="5.77,298,1596524400"; d="scan'208";a="179343268" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Sep 2020 09:22:38 -0700 IronPort-SDR: RpNiZ+Tm5mA+XwUUSNd+6J5JoUJJNjaY5EdphxigpcI1VAiMzWf+izmd0PrgiWkMLZ+lZwmFcK pUwOS/IqAeTw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,298,1596524400"; d="scan'208";a="336048385" Received: from orsmsx605.amr.corp.intel.com ([10.22.229.18]) by fmsmga004.fm.intel.com with ESMTP; 24 Sep 2020 09:22:38 -0700 Received: from orsmsx607.amr.corp.intel.com (10.22.229.20) by ORSMSX605.amr.corp.intel.com (10.22.229.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 24 Sep 2020 09:22:37 -0700 Received: from orsmsx611.amr.corp.intel.com (10.22.229.24) by ORSMSX607.amr.corp.intel.com (10.22.229.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 24 Sep 2020 09:22:37 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx611.amr.corp.intel.com (10.22.229.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5 via Frontend Transport; Thu, 24 Sep 2020 09:22:37 -0700 Received: from NAM02-SN1-obe.outbound.protection.outlook.com (104.47.36.58) by edgegateway.intel.com (134.134.137.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.1713.5; Thu, 24 Sep 2020 09:22:36 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HWaOd17J/IGv0ycQcijyKpujlwjyIHSFOaBbsyjGJ2HWbpC9kDQGQI++GjNE8d94+Mtt+Mdm+/S0ycBsdgB27jdhiND/94BjViXpmTepGCxmdQdXbjs5eP0r5v4a5x2ornAOEgY9ys23YPERhtEG61xLvOe6vEhHlkFtPT2IxicTIzbO48fmpjiXgUcprVjHCS/fFKmKtY1/7x4fGQDP44r0zkgcpHUw0vqC3rf7m6ALi0huGWKzKAxc3fClGB69+cawmEM1Yf1eNUW1MH1Jy1G9hg1i56byifk0jSaauLQ0bEiqyQfJnh0XyNZdZDmOSYxCDlkuG7K8dQ892PFqwQ== 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=tucoeZ7iPC+bPfwnSOWfBXaDGwchQe1Xezruqu/fAAo=; b=Trh/r7KEk5mPdqHSCzeYHseeijqy5Orq9FY1yEeio9/O228DMNb7rlCbUlaThAtoE+05NJ6lSvEwff7EgaLEf8fAp+FD0B3VsSnpjbB345SMoC2RPhDt8RVSV6gfxirndZdo/rTnzBMQ7jfzcy2qUztD2uvgrOSLWy4zxbNjl347Qu7CFSuLViR1kwrWFqH0YOAfmGrwyhRcmcUIAFxLUb9YUPKEqG3Rc0qs+kBeNAT7GgVyjxdi/Zf1Aubn5DnFE9lsGSws8hHa9zAzteD90tQ7eucLZrGEUuy2OtEhwi1ufxwpJvx74FuWrO9BrCp24Ai9Zu/IEs5wFDnD6Y6ZPg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tucoeZ7iPC+bPfwnSOWfBXaDGwchQe1Xezruqu/fAAo=; b=lVWOijo1zEBhFpReB+iYyUOHldjoptj/fF8B37Nysyim63emwPlbHzoXQGTKEc7ABmKDUXBz+6uNet8Q2s9gi8VjwArxOFtJ5whSplT56mOFLE0g18TcoqVsw+UvZkw6bKj1a6ZnJKUHswYRgOlVGnHly1N+dwF+LFuBg1g6sro= Received: from MN2PR11MB3550.namprd11.prod.outlook.com (2603:10b6:208:ee::21) by MN2PR11MB3871.namprd11.prod.outlook.com (2603:10b6:208:13c::31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3391.14; Thu, 24 Sep 2020 16:22:32 +0000 Received: from MN2PR11MB3550.namprd11.prod.outlook.com ([fe80::850f:c50e:27f4:a517]) by MN2PR11MB3550.namprd11.prod.outlook.com ([fe80::850f:c50e:27f4:a517%7]) with mapi id 15.20.3391.027; Thu, 24 Sep 2020 16:22:32 +0000 From: "Coyle, David" To: "akhil.goyal@nxp.com" , "dev@dpdk.org" , "thomas@monjalon.net" , "mdr@ashroe.eu" , "anoobj@marvell.com" CC: "hemant.agrawal@nxp.com" , "Ananyev, Konstantin" , "Doherty, Declan" , "Nicolau, Radu" Thread-Topic: [PATCH] security: update session create API Thread-Index: AQHWgi+SNKw86ZmcJkWkVyH81iLrTql3+1ig Date: Thu, 24 Sep 2020 16:22:32 +0000 Message-ID: References: <20200903200958.28025-1-akhil.goyal@nxp.com> In-Reply-To: <20200903200958.28025-1-akhil.goyal@nxp.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-reaction: no-action dlp-version: 11.5.1.3 dlp-product: dlpe-windows authentication-results: nxp.com; dkim=none (message not signed) header.d=none;nxp.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [51.37.95.114] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 66e02519-a116-44f0-1435-08d860a60a55 x-ms-traffictypediagnostic: MN2PR11MB3871: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:5797; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: YIS4Z4o21cttlgCQyQ9DEQ2OdXuxs9QtU44mQgtfgmg55+/6CNAlM0zGSpUxbGhSkTADrGLuMxNVGBRoHE64R0y3MS6X0UnkiCWux4PXUOKQnsA1A0b3mcVmZwsvEid4SF8LC3zw7nQqqhvpezi4WujLo2Ragz4AAdDyxZo+aJQPCa3JPu+D7rXA1KX993Xxtx6nV/S2/AnKdqC5pZ5tDvzqz4ubV+0WUBEquhNSruYgJrzl9SxT9Gm9v3AEydx6K9AE+KZ589xSYDwiRLLHak8uPwRpMgQPf7HKHMRzk7klkUVPVJOAjSPMNFPgYOFDKZ+O8sQKWAhVaImp7plOlA== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MN2PR11MB3550.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(39860400002)(396003)(366004)(376002)(136003)(346002)(66946007)(107886003)(26005)(86362001)(2906002)(66556008)(8676002)(33656002)(66446008)(71200400001)(8936002)(186003)(64756008)(4326008)(15650500001)(5660300002)(9686003)(66476007)(6506007)(83380400001)(478600001)(7696005)(110136005)(54906003)(55016002)(316002)(52536014)(76116006); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: 9fRD7BVhWLnyYBmXtRY3uLZoVU5Wy0iG2sv3J1Lj26DaT9ZXAl8ZC7z2QYiSyD5E/ox7yR/VPeyyojpffgEuMjjbGP1ftSPBloKs75fayr5wE8AvXZs3OqYwD1QBJX6LP265kPjFwVL4Q9F7hhj/wkg6CySSNedevzDe8rKNFfBeChaxfGG8Q+jeG2fliYRQDX6yAQmiK16TC2GqPhu29eC0Oa16NFXKcAqkQH4iIVjMO+iegqmcw3P9SmRR5iP0szIlAeFNkGpDBFXii3Se11GxeBV/WH2eEzX9jnpPd3AnUXZSxRZhk+0BtSQOqvzHA1e1IEhVwjS8YTzXjqGrL85gYlQ7XeWits6bZuw5OVdtamRtO7WR4stkAxHEnQ9Lcr7/6t92GBb5NfLudR/T/UGuzoG5G9XtwftkE3zqGZ3TpBhlTahZe6AEdJHkJHVSrKxKP6rjF6KjtvI6+Ywe7fNum3v30mcdrYQtqhRHHYWle5yLgsCrPniyYAr/6KJ0xGdpJZG6VDkocLRukVS2OPOC3jnbnFs4TQk9gmUjDzPn10YVmrP4VO06pjhX9AE91fUU2dEkdNM/JXkUNGC24cQZHPwWYeRQikoyzzjvHN2dkw0mN6HTUW8AnqCn2dChv0QPV7eoU7ukIBhXp8xdjQ== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN2PR11MB3550.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 66e02519-a116-44f0-1435-08d860a60a55 X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Sep 2020 16:22:32.6061 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: jFKBharU1NiQq9m20dlMDKfzusAp6B0XxnGD3HbVmM4FqzcFxD/Pc+cA2B5ObL98T5hyBa/B7p2ZgeRjJUs51w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB3871 X-OriginatorOrg: intel.com 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" Hi Akhil > -----Original Message----- > From: akhil.goyal@nxp.com > Sent: Thursday, September 3, 2020 9:10 PM > 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, >=20 > /* 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); [DC] ts_params->session_mpool is a cryptodev sym session pool. The assumpti= on then in these security tests is that security sessions are smaller than cryptodev sym sessions. This is currentl= y 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 =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 > 790,7 +809,7 @@ test_session_create_inv_mempool(void) > struct rte_security_session *sess; >=20 > sess =3D 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 se= t 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 me= mpools independently. > 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. >=20 > 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 da= ta mempool that is being referred to "The mempool object size should be able to accommodate the driver's private= data of security session." =3D>=20 "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. > 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 >=20 > +* 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_g= et_size callback. There's probably a job here for each PMD owner to add support for= this callback. >=20 > 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 > @@ -2379,12 +2375,8 @@ session_priv_pool_init(struct socket_ctx *ctx, > int32_t socket_id, >=20 > 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()); [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 =3D enabled_cdev_count * - opts->nb_qps * 2; + opts->nb_qps; > git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_securi= ty.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; [DC] Need to add a validity check for priv_mp to rte_security_session_creat= e(). The cryptodev API checks both mp and priv_mp are not NULL, so security shou= ld do the same RTE_PTR_OR_ERR_RET(priv_mp, NULL); >=20 > -- > 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 p= rivate aesni_mb_session struct. This didn't show up previously because the elements of the mempool were lar= ge, because both security session and private session data came from the same mempool with large objects . But now that the secur= ity 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/crypt= o/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 =3D rte_mempool_from_obj(sess_p= riv); - 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?