From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <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>
Thread-Topic: [PATCH] security: update session create API
Thread-Index: AQHWgi47pud7Lvi5P0KvZAQ+zHhWTKl4GXIAgBmDtDA=
Date: Sat, 10 Oct 2020 22:06:31 +0000
Message-ID: <VI1PR04MB31684CEDDC74497D3F842F37E6090@VI1PR04MB3168.eurprd04.prod.outlook.com>
References: <20200903200958.28025-1-akhil.goyal@nxp.com>
<MN2PR11MB355088D9C924E31E292C9098E3390@MN2PR11MB3550.namprd11.prod.outlook.com>
In-Reply-To: <MN2PR11MB355088D9C924E31E292C9098E3390@MN2PR11MB3550.namprd11.prod.outlook.com>
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: <VI1PR0402MB2752A34AD719C275AC8CB765E6090@VI1PR0402MB2752.eurprd04.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Hi David,
> Hi Akhil
>=20
> > -----Original Message-----
> > From: akhil.goyal@nxp.com <akhil.goyal@nxp.com>
> > Sent: Thursday, September 3, 2020 9:10 PM
>=20
> <snip>
>=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
> <snip>
>=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
> <snip>
>=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
> <snip>
>=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
> <snip>
>=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
> <snip>
>=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
> <snip>
>=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
> <snip>
>=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.