security: update session create API

  From: Akhil Goyal <akhil.goyal@nxp.com>

The API ``rte_security_session_create`` takes only single
mempool for session and session private data. So the
application need to create mempool for twice the number of
sessions needed and will also lead to wastage of memory as
session private data need more memory compared to session.
Hence the API is modified to take two mempool pointers
- one for session and one for private data.
This is very similar to crypto based session create APIs.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---
 app/test-crypto-perf/cperf_ops.c       |  4 +--
 app/test/test_cryptodev.c              |  8 +++--
 app/test/test_ipsec.c                  |  3 +-
 app/test/test_security.c               | 42 ++++++++++++++++++++------
 doc/guides/prog_guide/rte_security.rst |  6 ++--
 doc/guides/rel_notes/deprecation.rst   |  7 -----
 doc/guides/rel_notes/release_20_11.rst |  6 ++++
 examples/ipsec-secgw/ipsec-secgw.c     | 12 ++------
 examples/ipsec-secgw/ipsec.c           |  9 ++++--
 lib/librte_security/rte_security.c     |  6 ++--
 lib/librte_security/rte_security.h     |  4 ++-
 11 files changed, 68 insertions(+), 39 deletions(-)
  

W dniu 03.09.2020 o 22:09, akhil.goyal@nxp.com pisze:
> From: Akhil Goyal <akhil.goyal@nxp.com>
>
> The API ``rte_security_session_create`` takes only single
> mempool for session and session private data. So the
> application need to create mempool for twice the number of
> sessions needed and will also lead to wastage of memory as
> session private data need more memory compared to session.
> Hence the API is modified to take two mempool pointers
> - one for session and one for private data.
> This is very similar to crypto based session create APIs.
>
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
>   app/test-crypto-perf/cperf_ops.c       |  4 +--
>   app/test/test_cryptodev.c              |  8 +++--
>   app/test/test_ipsec.c                  |  3 +-
>   app/test/test_security.c               | 42 ++++++++++++++++++++------
>   doc/guides/prog_guide/rte_security.rst |  6 ++--
>   doc/guides/rel_notes/deprecation.rst   |  7 -----
>   doc/guides/rel_notes/release_20_11.rst |  6 ++++
>   examples/ipsec-secgw/ipsec-secgw.c     | 12 ++------
>   examples/ipsec-secgw/ipsec.c           |  9 ++++--
>   lib/librte_security/rte_security.c     |  6 ++--
>   lib/librte_security/rte_security.h     |  4 ++-
>   11 files changed, 68 insertions(+), 39 deletions(-)
>
> diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
> index 3da835a9c..3a64a2c34 100644
> --- a/app/test-crypto-perf/cperf_ops.c
> +++ b/app/test-crypto-perf/cperf_ops.c
> @@ -621,7 +621,7 @@ cperf_create_session(struct rte_mempool *sess_mp,
>   
>   		/* Create security session */
>   		return (void *)rte_security_session_create(ctx,
> -					&sess_conf, sess_mp);
> +					&sess_conf, sess_mp, priv_mp);
>   	}
>   	if (options->op_type == CPERF_DOCSIS) {
>   		enum rte_security_docsis_direction direction;
> @@ -664,7 +664,7 @@ cperf_create_session(struct rte_mempool *sess_mp,
>   
>   		/* Create security session */
>   		return (void *)rte_security_session_create(ctx,
> -					&sess_conf, priv_mp);
> +					&sess_conf, sess_mp, priv_mp);
>   	}
>   #endif
>   	sess = rte_cryptodev_sym_session_create(sess_mp);
> 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);
>   
>   	if (!ut_params->sec_session) {
>   		printf("TestCase %s()-%d line %d failed %s: ",
> @@ -7479,7 +7480,8 @@ test_pdcp_proto_SGL(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);
>   
>   	if (!ut_params->sec_session) {
>   		printf("TestCase %s()-%d line %d failed %s: ",
> @@ -7836,6 +7838,7 @@ test_docsis_proto_uplink(int i, struct docsis_test_data *d_td)
>   
>   	/* Create security session */
>   	ut_params->sec_session = rte_security_session_create(ctx, &sess_conf,
> +					ts_params->session_mpool,
>   					ts_params->session_priv_mpool);
>   
>   	if (!ut_params->sec_session) {
> @@ -8011,6 +8014,7 @@ test_docsis_proto_downlink(int i, struct docsis_test_data *d_td)
>   
>   	/* Create security session */
>   	ut_params->sec_session = rte_security_session_create(ctx, &sess_conf,
> +					ts_params->session_mpool,
>   					ts_params->session_priv_mpool);
>   
>   	if (!ut_params->sec_session) {
> diff --git a/app/test/test_ipsec.c b/app/test/test_ipsec.c
> index 79d00d7e0..9ad07a179 100644
> --- a/app/test/test_ipsec.c
> +++ b/app/test/test_ipsec.c
> @@ -632,7 +632,8 @@ create_dummy_sec_session(struct ipsec_unitest_params *ut,
>   	static struct rte_security_session_conf conf;
>   
>   	ut->ss[j].security.ses = rte_security_session_create(&dummy_sec_ctx,
> -					&conf, qp->mp_session_private);
> +					&conf, qp->mp_session,
> +					qp->mp_session_private);
>   
>   	if (ut->ss[j].security.ses == NULL)
>   		return -ENOMEM;
> 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;
>   
>   	int ret;
>   
session_create op is now called with private mbuf, so you need also to 
update assert in mock session_create:

@@ -248,13 +249,13 @@ static int
  mock_session_create(void *device,
                 struct rte_security_session_conf *conf,
                 struct rte_security_session *sess,
-               struct rte_mempool *mp)
+               struct rte_mempool *priv_mp)
  {
         mock_session_create_exp.called++;

         MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, 
device);
         MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, conf);
-       MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, mp);
+       MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp, 
priv_mp);

         mock_session_create_exp.sess = sess;



> @@ -502,6 +503,7 @@ struct rte_security_ops mock_ops = {
>    */
>   static struct security_testsuite_params {
>   	struct rte_mempool *session_mpool;
> +	struct rte_mempool *session_priv_mpool;
>   } testsuite_params = { NULL };
>   
>   /**
> @@ -525,6 +527,7 @@ static struct security_unittest_params {
>   };
>   
>   #define SECURITY_TEST_MEMPOOL_NAME "SecurityTestsMempoolName"
> +#define SECURITY_TEST_PRIV_MEMPOOL_NAME "SecurityTestsPrivMempoolName"
Please make the mempool name shorter, otherwise it causes tests to fail:

EAL: Test assert testsuite_setup line 558 failed: Cannot create priv 
mempool File name too long

>   #define SECURITY_TEST_MEMPOOL_SIZE 15
>   #define SECURITY_TEST_SESSION_OBJECT_SIZE sizeof(struct rte_security_session)
>   
> @@ -545,6 +548,17 @@ testsuite_setup(void)
>   			SOCKET_ID_ANY, 0);
>   	TEST_ASSERT_NOT_NULL(ts_params->session_mpool,
>   			"Cannot create mempool %s\n", rte_strerror(rte_errno));
> +
> +	ts_params->session_priv_mpool = rte_mempool_create(
> +			SECURITY_TEST_PRIV_MEMPOOL_NAME,
> +			SECURITY_TEST_MEMPOOL_SIZE,
> +			rte_security_session_get_size(&unittest_params.ctx),
> +			0, 0, NULL, NULL, NULL, NULL,
> +			SOCKET_ID_ANY, 0);
> +	TEST_ASSERT_NOT_NULL(ts_params->session_priv_mpool,
> +			"Cannot create priv mempool %s\n",
> +			rte_strerror(rte_errno));
> +
If creation of private data mpool fails, primary mempool need to be 
freed before function returns failure code.
>   	return TEST_SUCCESS;
>   }
>   
> @@ -659,7 +673,8 @@ ut_setup_with_session(void)
>   	mock_session_create_exp.ret = 0;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_NOT_NULL(rte_security_session_create,
>   			sess);
>   	TEST_ASSERT_EQUAL(sess, mock_session_create_exp.sess,
> @@ -701,7 +716,8 @@ test_session_create_inv_context(void)
>   	struct rte_security_session *sess;
>   
>   	sess = rte_security_session_create(NULL, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -725,7 +741,8 @@ test_session_create_inv_context_ops(void)
>   	ut_params->ctx.ops = NULL;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -749,7 +766,8 @@ test_session_create_inv_context_ops_fun(void)
>   	ut_params->ctx.ops = &empty_ops;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -770,7 +788,8 @@ test_session_create_inv_configuration(void)
>   	struct rte_security_session *sess;
>   
>   	sess = rte_security_session_create(&ut_params->ctx, NULL,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -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);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -824,7 +843,8 @@ test_session_create_mempool_empty(void)
>   	TEST_ASSERT_MEMPOOL_USAGE(SECURITY_TEST_MEMPOOL_SIZE);
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 0);
> @@ -853,10 +873,12 @@ test_session_create_ops_failure(void)
>   	mock_session_create_exp.device = NULL;
>   	mock_session_create_exp.conf = &ut_params->conf;
>   	mock_session_create_exp.mp = ts_params->session_mpool;
> +	mock_session_create_exp.priv_mp = ts_params->session_priv_mpool;
>   	mock_session_create_exp.ret = -1;	/* Return failure status. */
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_session_create,
>   			sess, NULL, "%p");
>   	TEST_ASSERT_MOCK_CALLS(mock_session_create_exp, 1);
> @@ -879,10 +901,12 @@ test_session_create_success(void)
>   	mock_session_create_exp.device = NULL;
>   	mock_session_create_exp.conf = &ut_params->conf;
>   	mock_session_create_exp.mp = ts_params->session_mpool;
> +	mock_session_create_exp.priv_mp = ts_params->session_priv_mpool;
>   	mock_session_create_exp.ret = 0;	/* Return success status. */
>   
>   	sess = rte_security_session_create(&ut_params->ctx, &ut_params->conf,
> -			ts_params->session_mpool);
> +			ts_params->session_mpool,
> +			ts_params->session_priv_mpool);
>   	TEST_ASSERT_MOCK_FUNCTION_CALL_NOT_NULL(rte_security_session_create,
>   			sess);
>   	TEST_ASSERT_EQUAL(sess, mock_session_create_exp.sess,
> diff --git 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``.
>   
>   Once the session mempools have been created, ``rte_security_session_create()``
>   is used to allocate and initialize a session for the required crypto/ethernet device.
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 345c38d5b..84be88a13 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -263,13 +263,6 @@ Deprecation Notices
>     This feature faces reliability issues and is often conflicting with
>     new features being implemented.
>   
> -* security: The API ``rte_security_session_create`` takes only single mempool
> -  for session and session private data. So the application need to create
> -  mempool for twice the number of sessions needed and will also lead to
> -  wastage of memory as session private data need more memory compared to session.
> -  Hence the API will be modified to take two mempool pointers - one for session
> -  and one for private data.
> -
>   * cryptodev: ``RTE_CRYPTO_AEAD_LIST_END`` from ``enum rte_crypto_aead_algorithm``,
>     ``RTE_CRYPTO_CIPHER_LIST_END`` from ``enum rte_crypto_cipher_algorithm`` and
>     ``RTE_CRYPTO_AUTH_LIST_END`` from ``enum rte_crypto_auth_algorithm``
> 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.
> +
>   
>   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
> @@ -2351,12 +2351,8 @@ session_pool_init(struct socket_ctx *ctx, int32_t socket_id, size_t sess_sz)
>   
>   	snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
>   			"sess_mp_%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());
>   	sess_mp = rte_cryptodev_sym_session_pool_create(
>   			mp_name, nb_sess, sess_sz, CDEV_MP_CACHE_SZ, 0,
>   			socket_id);
> @@ -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());
>   	sess_mp = rte_mempool_create(mp_name,
>   			nb_sess,
>   			sess_sz,
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> index 01faa7ac7..6baeeb342 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -117,7 +117,8 @@ create_lookaside_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa,
>   			set_ipsec_conf(sa, &(sess_conf.ipsec));
>   
>   			ips->security.ses = rte_security_session_create(ctx,
> -					&sess_conf, ipsec_ctx->session_priv_pool);
> +					&sess_conf, ipsec_ctx->session_pool,
> +					ipsec_ctx->session_priv_pool);
>   			if (ips->security.ses == NULL) {
>   				RTE_LOG(ERR, IPSEC,
>   				"SEC Session init failed: err: %d\n", ret);
> @@ -198,7 +199,8 @@ create_inline_session(struct socket_ctx *skt_ctx, struct ipsec_sa *sa,
>   		}
>   
>   		ips->security.ses = rte_security_session_create(sec_ctx,
> -				&sess_conf, skt_ctx->session_pool);
> +				&sess_conf, skt_ctx->session_pool,
> +				skt_ctx->session_priv_pool);
>   		if (ips->security.ses == NULL) {
>   			RTE_LOG(ERR, IPSEC,
>   				"SEC Session init failed: err: %d\n", ret);
> @@ -378,7 +380,8 @@ create_inline_session(struct socket_ctx *skt_ctx, struct ipsec_sa *sa,
>   		sess_conf.userdata = (void *) sa;
>   
>   		ips->security.ses = rte_security_session_create(sec_ctx,
> -					&sess_conf, skt_ctx->session_pool);
> +					&sess_conf, skt_ctx->session_pool,
> +					skt_ctx->session_priv_pool);
>   		if (ips->security.ses == NULL) {
>   			RTE_LOG(ERR, IPSEC,
>   				"SEC Session init failed: err: %d\n", ret);
> diff --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;
>   
> @@ -37,7 +38,8 @@ rte_security_session_create(struct rte_security_ctx *instance,
>   	if (rte_mempool_get(mp, (void **)&sess))
>   		return NULL;
>   
> -	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
> +	if (instance->ops->session_create(instance->device, conf,
> +				sess, priv_mp)) {
>   		rte_mempool_put(mp, (void *)sess);
>   		return NULL;
>   	}
> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
> index 16839e539..1710cdd6a 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -386,6 +386,7 @@ struct rte_security_session {
>    * @param   instance	security instance
>    * @param   conf	session configuration parameters
>    * @param   mp		mempool to allocate session objects from
> + * @param   priv_mp	mempool to allocate session private data objects from
>    * @return
>    *  - On success, pointer to session
>    *  - On failure, NULL
> @@ -393,7 +394,8 @@ struct rte_security_session {
>   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);
>   
>   /**
>    * Update security session as specified by the session configuration
  

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?
  

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.
  

Hi Lukasz,

Thanks for the review.

> > 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;
> >
> >   	int ret;
> >
> session_create op is now called with private mbuf, so you need also to
> update assert in mock session_create:

OK will be fixed in v2

> 
> @@ -248,13 +249,13 @@ static int
>   mock_session_create(void *device,
>                  struct rte_security_session_conf *conf,
>                  struct rte_security_session *sess,
> -               struct rte_mempool *mp)
> +               struct rte_mempool *priv_mp)
>   {
>          mock_session_create_exp.called++;
> 
>          MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp,
> device);
>          MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp,
> conf);
> -       MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp,
> mp);
> +       MOCK_TEST_ASSERT_POINTER_PARAMETER(mock_session_create_exp,
> priv_mp);
> 
>          mock_session_create_exp.sess = sess;
> 
> 
> 
> > @@ -502,6 +503,7 @@ struct rte_security_ops mock_ops = {
> >    */
> >   static struct security_testsuite_params {
> >   	struct rte_mempool *session_mpool;
> > +	struct rte_mempool *session_priv_mpool;
> >   } testsuite_params = { NULL };
> >
> >   /**
> > @@ -525,6 +527,7 @@ static struct security_unittest_params {
> >   };
> >
> >   #define SECURITY_TEST_MEMPOOL_NAME "SecurityTestsMempoolName"
> > +#define SECURITY_TEST_PRIV_MEMPOOL_NAME
> "SecurityTestsPrivMempoolName"
> Please make the mempool name shorter, otherwise it causes tests to fail:
> 
> EAL: Test assert testsuite_setup line 558 failed: Cannot create priv
> mempool File name too long
> 

Fixed in v2

> >   #define SECURITY_TEST_MEMPOOL_SIZE 15
> >   #define SECURITY_TEST_SESSION_OBJECT_SIZE sizeof(struct
> rte_security_session)
> >
> > @@ -545,6 +548,17 @@ testsuite_setup(void)
> >   			SOCKET_ID_ANY, 0);
> >   	TEST_ASSERT_NOT_NULL(ts_params->session_mpool,
> >   			"Cannot create mempool %s\n",
> rte_strerror(rte_errno));
> > +
> > +	ts_params->session_priv_mpool = rte_mempool_create(
> > +			SECURITY_TEST_PRIV_MEMPOOL_NAME,
> > +			SECURITY_TEST_MEMPOOL_SIZE,
> > +			rte_security_session_get_size(&unittest_params.ctx),
> > +			0, 0, NULL, NULL, NULL, NULL,
> > +			SOCKET_ID_ANY, 0);
> > +	TEST_ASSERT_NOT_NULL(ts_params->session_priv_mpool,
> > +			"Cannot create priv mempool %s\n",
> > +			rte_strerror(rte_errno));
> > +
> If creation of private data mpool fails, primary mempool need to be
> freed before function returns failure code.
This is an issue in whole of the file.
However, have fixed it in v2 for this particular case in v2.