[PATCH 1/2] test/crypto: free memory in error and skip paths

[PATCH 1/2] test/crypto: free memory in error and skip paths

[Anoob Joseph]

In multi session tests, multiple sessions get created. So the handling
in ut_teardown won't guard against any memory that is not freed by the
test case. Test case should free sessions as well as local memory that
was used to save session pointers both in case of unsupported cases as
well as operation failures.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 app/test/test_cryptodev.c | 55 +++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index fb4fc4e805..86e63a33fc 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -12751,8 +12751,8 @@ test_multi_session(void)
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
 	struct crypto_unittest_params *ut_params = &unittest_params;
 	struct rte_cryptodev_info dev_info;
+	int i, nb_sess, ret = TEST_SUCCESS;
 	void **sessions;
-	uint16_t i;
 
 	/* Verify the capabilities */
 	struct rte_cryptodev_sym_capability_idx cap_idx;
@@ -12782,21 +12782,25 @@ test_multi_session(void)
 		sessions[i] = rte_cryptodev_sym_session_create(
 			ts_params->valid_devs[0], &ut_params->auth_xform,
 				ts_params->session_mpool);
-		if (sessions[i] == NULL && rte_errno == ENOTSUP)
-			return TEST_SKIPPED;
+		if (sessions[i] == NULL && rte_errno == ENOTSUP) {
+			nb_sess = i;
+			ret = TEST_SKIPPED;
+			break;
+		}
 
 		TEST_ASSERT_NOT_NULL(sessions[i],
 				"Session creation failed at session number %u",
 				i);
+
 		/* Attempt to send a request on each session */
-		TEST_ASSERT_SUCCESS( test_AES_CBC_HMAC_SHA512_decrypt_perform(
+		ret = test_AES_CBC_HMAC_SHA512_decrypt_perform(
 			sessions[i],
 			ut_params,
 			ts_params,
 			catch_22_quote_2_512_bytes_AES_CBC_ciphertext,
 			catch_22_quote_2_512_bytes_AES_CBC_HMAC_SHA512_digest,
-			aes_cbc_iv),
-			"Failed to perform decrypt on request number %u.", i);
+			aes_cbc_iv);
+
 		/* free crypto operation structure */
 		rte_crypto_op_free(ut_params->op);
 
@@ -12815,16 +12819,25 @@ test_multi_session(void)
 			rte_pktmbuf_free(ut_params->ibuf);
 			ut_params->ibuf = 0;
 		}
+
+		if (ret == TEST_SKIPPED) {
+			i++;
+			break;
+		}
+
+		TEST_ASSERT_SUCCESS(ret, "Failed to perform decrypt on request number %u.", i);
 	}
 
-	for (i = 0; i < MAX_NB_SESSIONS; i++) {
+	nb_sess = i;
+
+	for (i = 0; i < nb_sess; i++) {
 		rte_cryptodev_sym_session_free(ts_params->valid_devs[0],
 				sessions[i]);
 	}
 
 	rte_free(sessions);
 
-	return TEST_SUCCESS;
+	return ret;
 }
 
 struct multi_session_params {
@@ -12843,8 +12856,9 @@ test_multi_session_random_usage(void)
 {
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
 	struct rte_cryptodev_info dev_info;
+	uint32_t nb_sess, i, j;
+	int ret = TEST_SUCCESS;
 	void **sessions;
-	uint32_t i, j;
 	struct multi_session_params ut_paramz[] = {
 
 		{
@@ -12903,27 +12917,30 @@ test_multi_session_random_usage(void)
 				ts_params->valid_devs[0],
 				&ut_paramz[i].ut_params.auth_xform,
 				ts_params->session_mpool);
-		if (sessions[i] == NULL && rte_errno == ENOTSUP)
-			return TEST_SKIPPED;
+		if (sessions[i] == NULL && rte_errno == ENOTSUP) {
+			nb_sess = i;
+			ret = TEST_SKIPPED;
+			goto session_clear;
+		}
 
 		TEST_ASSERT_NOT_NULL(sessions[i],
 				"Session creation failed at session number %u",
 				i);
 	}
 
+	nb_sess = i;
+
 	srand(time(NULL));
 	for (i = 0; i < 40000; i++) {
 
 		j = rand() % MB_SESSION_NUMBER;
 
-		TEST_ASSERT_SUCCESS(
-			test_AES_CBC_HMAC_SHA512_decrypt_perform(
+		ret = test_AES_CBC_HMAC_SHA512_decrypt_perform(
 					sessions[j],
 					&ut_paramz[j].ut_params,
 					ts_params, ut_paramz[j].cipher,
 					ut_paramz[j].digest,
-					ut_paramz[j].iv),
-			"Failed to perform decrypt on request number %u.", i);
+					ut_paramz[j].iv);
 
 		rte_crypto_op_free(ut_paramz[j].ut_params.op);
 
@@ -12943,9 +12960,15 @@ test_multi_session_random_usage(void)
 			rte_pktmbuf_free(ut_paramz[j].ut_params.ibuf);
 			ut_paramz[j].ut_params.ibuf = 0;
 		}
+
+		if (ret == TEST_SKIPPED)
+			break;
+
+		TEST_ASSERT_SUCCESS(ret, "Failed to perform decrypt on request number %u.", i);
 	}
 
-	for (i = 0; i < MB_SESSION_NUMBER; i++) {
+session_clear:
+	for (i = 0; i < nb_sess; i++) {
 		rte_cryptodev_sym_session_free(ts_params->valid_devs[0],
 				sessions[i]);
 	}
-- 
2.25.1

[PATCH 2/2] test/crypto: handle return code

[Anoob Joseph]

The sub test case, test_snow3g_decryption, may fail for non-critical
reasons such as lack of support. Handle the return value gracefully to
allow TEST_SKIPPED return value to be propagated correctly.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 app/test/test_cryptodev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 86e63a33fc..d212e71ba6 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -6947,6 +6947,7 @@ test_snow3g_decryption_with_digest_test_case_1(void)
 	struct snow3g_hash_test_data snow3g_hash_data;
 	struct rte_cryptodev_info dev_info;
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
+	int ret;
 
 	rte_cryptodev_info_get(ts_params->valid_devs[0], &dev_info);
 	uint64_t feat_flags = dev_info.feature_flags;
@@ -6962,8 +6963,9 @@ test_snow3g_decryption_with_digest_test_case_1(void)
 	 */
 	snow3g_hash_test_vector_setup(&snow3g_test_case_7, &snow3g_hash_data);
 
-	if (test_snow3g_decryption(&snow3g_test_case_7))
-		return TEST_FAILED;
+	ret = test_snow3g_decryption(&snow3g_test_case_7);
+	if (ret != TEST_SUCCESS)
+		return ret;
 
 	return test_snow3g_authentication_verify(&snow3g_hash_data);
 }
-- 
2.25.1

RE: [PATCH 1/2] test/crypto: free memory in error and skip paths

[Akhil Goyal]

> Subject: [PATCH 1/2] test/crypto: free memory in error and skip paths
> 
> In multi session tests, multiple sessions get created. So the handling
> in ut_teardown won't guard against any memory that is not freed by the
> test case. Test case should free sessions as well as local memory that
> was used to save session pointers both in case of unsupported cases as
> well as operation failures.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Acked-by: Akhil Goyal <gakhil@marvell.com>

Thanks for the cleanup

RE: [PATCH 2/2] test/crypto: handle return code

[Akhil Goyal]

> Subject: [PATCH 2/2] test/crypto: handle return code
> 
> The sub test case, test_snow3g_decryption, may fail for non-critical
> reasons such as lack of support. Handle the return value gracefully to
> allow TEST_SKIPPED return value to be propagated correctly.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Acked-by: Akhil Goyal <gakhil@marvell.com>

RE: [PATCH 2/2] test/crypto: handle return code

[Power, Ciara]

Hi Anoob,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday 16 May 2023 11:13
> To: Anoob Joseph <anoobj@marvell.com>; Fan Zhang
> <fanzhang.oss@gmail.com>; Power, Ciara <ciara.power@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>; Ji, Kai
> <kai.ji@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH 2/2] test/crypto: handle return code
> 
> > Subject: [PATCH 2/2] test/crypto: handle return code
> >
> > The sub test case, test_snow3g_decryption, may fail for non-critical
> > reasons such as lack of support. Handle the return value gracefully to
> > allow TEST_SKIPPED return value to be propagated correctly.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> Acked-by: Akhil Goyal <gakhil@marvell.com>

This looks like a duplicate of a patch recently merged: https://patches.dpdk.org/project/dpdk/patch/20230414135526.4271-1-saoirse.odonovan@intel.com/

Thanks,
Ciara

RE: [PATCH 2/2] test/crypto: handle return code

[Anoob Joseph]

Hi Ciara,

> This looks like a duplicate of a patch recently merged:
Indeed. Missed it.

@Akhil, we can abandon this patch.

Thanks,
Anoob

> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Tuesday, May 16, 2023 4:16 PM
> To: Akhil Goyal <gakhil@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; Fan Zhang <fanzhang.oss@gmail.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>; Ji, Kai
> <kai.ji@intel.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH 2/2] test/crypto: handle return code
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob,
> 
> > -----Original Message-----
> > From: Akhil Goyal <gakhil@marvell.com>
> > Sent: Tuesday 16 May 2023 11:13
> > To: Anoob Joseph <anoobj@marvell.com>; Fan Zhang
> > <fanzhang.oss@gmail.com>; Power, Ciara <ciara.power@intel.com>
> > Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Jerin Jacob
> Kollanukkaran
> > <jerinj@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>; Ji, Kai
> > <kai.ji@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH 2/2] test/crypto: handle return code
> >
> > > Subject: [PATCH 2/2] test/crypto: handle return code
> > >
> > > The sub test case, test_snow3g_decryption, may fail for non-critical
> > > reasons such as lack of support. Handle the return value gracefully
> > > to allow TEST_SKIPPED return value to be propagated correctly.
> > >
> > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > Acked-by: Akhil Goyal <gakhil@marvell.com>
> 
> This looks like a duplicate of a patch recently merged:
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patches.dpdk.org_project_dpdk_patch_20230414135526.4271-2D1-
> 2Dsaoirse.odonovan-
> 40intel.com_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxy
> LWs2n6B-
> WYLn1v9SyTMrT5EQqh2TU&m=O_paIbxHa0g1eO0nnvRRDxs52JIZh_Xteso9I
> wXdLnFAYQyoPk4iryD2h4_uQRdB&s=QG45QCchVqKQyfhL8vENmY-
> JXiwsRS6zrbV3XxJoPTA&e=
> 
> Thanks,
> Ciara

[PATCH v2] test/crypto: free memory in error and skip paths

[Anoob Joseph]

In multi session tests, multiple sessions get created. So the handling
in ut_teardown won't guard against any memory that is not freed by the
test case. Test case should free sessions as well as local memory that
was used to save session pointers both in case of unsupported cases as
well as operation failures.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
v2:
* Moved 'ASSERT' to the end to allow cleanup in all cases.
---

 app/test/test_cryptodev.c | 59 ++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index aaa2ce428d..77187e1fd3 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -12753,8 +12753,8 @@ test_multi_session(void)
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
 	struct crypto_unittest_params *ut_params = &unittest_params;
 	struct rte_cryptodev_info dev_info;
+	int i, nb_sess, ret = TEST_SUCCESS;
 	void **sessions;
-	uint16_t i;
 
 	/* Verify the capabilities */
 	struct rte_cryptodev_sym_capability_idx cap_idx;
@@ -12784,21 +12784,25 @@ test_multi_session(void)
 		sessions[i] = rte_cryptodev_sym_session_create(
 			ts_params->valid_devs[0], &ut_params->auth_xform,
 				ts_params->session_mpool);
-		if (sessions[i] == NULL && rte_errno == ENOTSUP)
-			return TEST_SKIPPED;
+		if (sessions[i] == NULL && rte_errno == ENOTSUP) {
+			nb_sess = i;
+			ret = TEST_SKIPPED;
+			break;
+		}
 
 		TEST_ASSERT_NOT_NULL(sessions[i],
 				"Session creation failed at session number %u",
 				i);
+
 		/* Attempt to send a request on each session */
-		TEST_ASSERT_SUCCESS( test_AES_CBC_HMAC_SHA512_decrypt_perform(
+		ret = test_AES_CBC_HMAC_SHA512_decrypt_perform(
 			sessions[i],
 			ut_params,
 			ts_params,
 			catch_22_quote_2_512_bytes_AES_CBC_ciphertext,
 			catch_22_quote_2_512_bytes_AES_CBC_HMAC_SHA512_digest,
-			aes_cbc_iv),
-			"Failed to perform decrypt on request number %u.", i);
+			aes_cbc_iv);
+
 		/* free crypto operation structure */
 		rte_crypto_op_free(ut_params->op);
 
@@ -12817,16 +12821,26 @@ test_multi_session(void)
 			rte_pktmbuf_free(ut_params->ibuf);
 			ut_params->ibuf = 0;
 		}
+
+		if (ret != TEST_SUCCESS) {
+			i++;
+			break;
+		}
 	}
 
-	for (i = 0; i < MAX_NB_SESSIONS; i++) {
+	nb_sess = i;
+
+	for (i = 0; i < nb_sess; i++) {
 		rte_cryptodev_sym_session_free(ts_params->valid_devs[0],
 				sessions[i]);
 	}
 
 	rte_free(sessions);
 
-	return TEST_SUCCESS;
+	if (ret != TEST_SKIPPED)
+		TEST_ASSERT_SUCCESS(ret, "Failed to perform decrypt on request number %u.", i - 1);
+
+	return ret;
 }
 
 struct multi_session_params {
@@ -12845,8 +12859,9 @@ test_multi_session_random_usage(void)
 {
 	struct crypto_testsuite_params *ts_params = &testsuite_params;
 	struct rte_cryptodev_info dev_info;
+	int index = 0, ret = TEST_SUCCESS;
+	uint32_t nb_sess, i, j;
 	void **sessions;
-	uint32_t i, j;
 	struct multi_session_params ut_paramz[] = {
 
 		{
@@ -12905,27 +12920,30 @@ test_multi_session_random_usage(void)
 				ts_params->valid_devs[0],
 				&ut_paramz[i].ut_params.auth_xform,
 				ts_params->session_mpool);
-		if (sessions[i] == NULL && rte_errno == ENOTSUP)
-			return TEST_SKIPPED;
+		if (sessions[i] == NULL && rte_errno == ENOTSUP) {
+			nb_sess = i;
+			ret = TEST_SKIPPED;
+			goto session_clear;
+		}
 
 		TEST_ASSERT_NOT_NULL(sessions[i],
 				"Session creation failed at session number %u",
 				i);
 	}
 
+	nb_sess = i;
+
 	srand(time(NULL));
 	for (i = 0; i < 40000; i++) {
 
 		j = rand() % MB_SESSION_NUMBER;
 
-		TEST_ASSERT_SUCCESS(
-			test_AES_CBC_HMAC_SHA512_decrypt_perform(
+		ret = test_AES_CBC_HMAC_SHA512_decrypt_perform(
 					sessions[j],
 					&ut_paramz[j].ut_params,
 					ts_params, ut_paramz[j].cipher,
 					ut_paramz[j].digest,
-					ut_paramz[j].iv),
-			"Failed to perform decrypt on request number %u.", i);
+					ut_paramz[j].iv);
 
 		rte_crypto_op_free(ut_paramz[j].ut_params.op);
 
@@ -12945,15 +12963,24 @@ test_multi_session_random_usage(void)
 			rte_pktmbuf_free(ut_paramz[j].ut_params.ibuf);
 			ut_paramz[j].ut_params.ibuf = 0;
 		}
+
+		if (ret != TEST_SKIPPED) {
+			index = i;
+			break;
+		}
 	}
 
-	for (i = 0; i < MB_SESSION_NUMBER; i++) {
+session_clear:
+	for (i = 0; i < nb_sess; i++) {
 		rte_cryptodev_sym_session_free(ts_params->valid_devs[0],
 				sessions[i]);
 	}
 
 	rte_free(sessions);
 
+	if (ret != TEST_SKIPPED)
+		TEST_ASSERT_SUCCESS(ret, "Failed to perform decrypt on request number %u.", index);
+
 	return TEST_SUCCESS;
 }
 
-- 
2.25.1

RE: [PATCH v2] test/crypto: free memory in error and skip paths

[Power, Ciara]

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Wednesday 17 May 2023 11:55
> To: Akhil Goyal <gakhil@marvell.com>; Fan Zhang
> <fanzhang.oss@gmail.com>; Power, Ciara <ciara.power@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Jerin Jacob
> <jerinj@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>;
> dev@dpdk.org
> Subject: [PATCH v2] test/crypto: free memory in error and skip paths
> 
> In multi session tests, multiple sessions get created. So the handling in
> ut_teardown won't guard against any memory that is not freed by the test
> case. Test case should free sessions as well as local memory that was used to
> save session pointers both in case of unsupported cases as well as operation
> failures.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
> v2:
> * Moved 'ASSERT' to the end to allow cleanup in all cases.
> ---
<snip>

Acked-by: Ciara Power <ciara.power@intel.com>

RE: [PATCH v2] test/crypto: free memory in error and skip paths

[Akhil Goyal]

> Subject: [PATCH v2] test/crypto: free memory in error and skip paths
> 
> In multi session tests, multiple sessions get created. So the handling
> in ut_teardown won't guard against any memory that is not freed by the
> test case. Test case should free sessions as well as local memory that
> was used to save session pointers both in case of unsupported cases as
> well as operation failures.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
> v2:
> * Moved 'ASSERT' to the end to allow cleanup in all cases.
Applied to dpdk-next-crypto
Thanks.