[dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference

DPDK patches and discussions
 help / color / mirror / Atom feed

* [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference
@ 2017-07-31  2:30 Pablo de Lara
  2017-07-31  9:18 ` [dpdk-dev] [PATCH v2] " Pablo de Lara
  2017-07-31 12:32 ` [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference Sergio Gonzalez Monroy
  0 siblings, 2 replies; 15+ messages in thread
From: Pablo de Lara @ 2017-07-31  2:30 UTC (permalink / raw)
  To: declan.doherty; +Cc: dev, Pablo de Lara

When registering a crypto driver, if memory allocation
fails, application should exit and do not allow
a NULL pointer dereference.

Coverity issue: 158645

Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 lib/librte_cryptodev/rte_cryptodev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 327d7e8..4492b0d 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -1404,6 +1404,12 @@ rte_cryptodev_allocate_driver(const struct rte_driver *drv)
 	struct cryptodev_driver *driver;
 
 	driver = malloc(sizeof(*driver));
+
+	if (driver == NULL)
+		rte_exit(EXIT_FAILURE,
+			"Could not allocate memory for crypto driver %u\n",
+			nb_drivers);
+
 	driver->driver = drv;
 	driver->id = nb_drivers;
 
-- 
2.9.4

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-07-31  2:30 [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference Pablo de Lara
@ 2017-07-31  9:18 ` Pablo de Lara
  2017-07-31 19:33   ` Thomas Monjalon
  2017-08-16  2:41   ` [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically Pablo de Lara
  2017-07-31 12:32 ` [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference Sergio Gonzalez Monroy
  1 sibling, 2 replies; 15+ messages in thread
From: Pablo de Lara @ 2017-07-31  9:18 UTC (permalink / raw)
  To: sergio.gonzalez.monroy, thomas; +Cc: dev, Pablo de Lara

When register a crypto driver, a cryptodev driver
structure was being allocated, using malloc.
Since this call may fail, it is safer to allocate
this memory statically in each PMD, so driver registration
will never fail.

Coverity issue: 158645

Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---

Changes in v2:

- Allocate statically the cryptodev driver structure,
  instead of using malloc, that can potentially fail.

 drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
 drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
 drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
 drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
 drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
 drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
 drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
 drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
 drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
 drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
 lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
 lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
 lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30 +++++++++++++++++++++++++++++
 14 files changed, 83 insertions(+), 47 deletions(-)

diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
index d9c91d0..b59f399 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
@@ -630,10 +630,13 @@ static struct rte_vdev_driver aesni_gcm_pmd_drv = {
 	.remove = aesni_gcm_remove
 };
 
+static struct cryptodev_driver aesni_gcm_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_AESNI_GCM_PMD, aesni_gcm_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_AESNI_GCM_PMD, cryptodev_aesni_gcm_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_AESNI_GCM_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_gcm_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_gcm_crypto_drv, aesni_gcm_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
index 16e1451..5846bc9 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
@@ -823,10 +823,14 @@ static struct rte_vdev_driver cryptodev_aesni_mb_pmd_drv = {
 	.remove = cryptodev_aesni_mb_remove
 };
 
+static struct cryptodev_driver aesni_mb_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_AESNI_MB_PMD, cryptodev_aesni_mb_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_AESNI_MB_PMD, cryptodev_aesni_mb_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_AESNI_MB_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_aesni_mb_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_mb_crypto_drv,
+		cryptodev_aesni_mb_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/armv8/rte_armv8_pmd.c b/drivers/crypto/armv8/rte_armv8_pmd.c
index c3ba439..9ab22cc 100644
--- a/drivers/crypto/armv8/rte_armv8_pmd.c
+++ b/drivers/crypto/armv8/rte_armv8_pmd.c
@@ -908,15 +908,18 @@ cryptodev_armv8_crypto_uninit(struct rte_vdev_device *vdev)
 	return 0;
 }
 
-static struct rte_vdev_driver armv8_crypto_drv = {
+static struct rte_vdev_driver armv8_crypto_pmd_drv = {
 	.probe = cryptodev_armv8_crypto_init,
 	.remove = cryptodev_armv8_crypto_uninit
 };
 
-RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_ARMV8_PMD, armv8_crypto_drv);
+static struct cryptodev_driver armv8_crypto_drv;
+
+RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_ARMV8_PMD, armv8_crypto_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_ARMV8_PMD, cryptodev_armv8_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_ARMV8_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(armv8_crypto_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(armv8_crypto_drv, armv8_crypto_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index e0f6cfc..dae296f 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -2008,5 +2008,8 @@ static struct rte_dpaa2_driver rte_dpaa2_sec_driver = {
 	.remove = cryptodev_dpaa2_sec_remove,
 };
 
+static struct cryptodev_driver dpaa2_sec_crypto_drv;
+
 RTE_PMD_REGISTER_DPAA2(CRYPTODEV_NAME_DPAA2_SEC_PMD, rte_dpaa2_sec_driver);
-RTE_PMD_REGISTER_CRYPTO_DRIVER(rte_dpaa2_sec_driver, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(dpaa2_sec_crypto_drv, rte_dpaa2_sec_driver,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/kasumi/rte_kasumi_pmd.c b/drivers/crypto/kasumi/rte_kasumi_pmd.c
index 38cd8a9..84cce34 100644
--- a/drivers/crypto/kasumi/rte_kasumi_pmd.c
+++ b/drivers/crypto/kasumi/rte_kasumi_pmd.c
@@ -661,10 +661,13 @@ static struct rte_vdev_driver cryptodev_kasumi_pmd_drv = {
 	.remove = cryptodev_kasumi_remove
 };
 
+static struct cryptodev_driver kasumi_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_KASUMI_PMD, cryptodev_kasumi_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_KASUMI_PMD, cryptodev_kasumi_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_KASUMI_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_kasumi_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(kasumi_crypto_drv, cryptodev_kasumi_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/null/null_crypto_pmd.c b/drivers/crypto/null/null_crypto_pmd.c
index 2c82725..d5d2bb3 100644
--- a/drivers/crypto/null/null_crypto_pmd.c
+++ b/drivers/crypto/null/null_crypto_pmd.c
@@ -286,10 +286,13 @@ static struct rte_vdev_driver cryptodev_null_pmd_drv = {
 	.remove = cryptodev_null_remove_dev,
 };
 
+static struct cryptodev_driver null_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_NULL_PMD, cryptodev_null_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_NULL_PMD, cryptodev_null_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_NULL_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_null_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(null_crypto_drv, cryptodev_null_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 0bd5f98..5b863c8 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -1502,10 +1502,13 @@ static struct rte_vdev_driver cryptodev_openssl_pmd_drv = {
 	.remove = cryptodev_openssl_remove
 };
 
+static struct cryptodev_driver openssl_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_OPENSSL_PMD,
 	cryptodev_openssl_pmd_drv);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_OPENSSL_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_openssl_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(openssl_crypto_drv, cryptodev_openssl_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/qat/rte_qat_cryptodev.c b/drivers/crypto/qat/rte_qat_cryptodev.c
index 7d56fca..3d9f3c8 100644
--- a/drivers/crypto/qat/rte_qat_cryptodev.c
+++ b/drivers/crypto/qat/rte_qat_cryptodev.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2015-2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2015-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -169,6 +169,9 @@ static struct rte_pci_driver rte_qat_pmd = {
 	.remove = crypto_qat_pci_remove
 };
 
+static struct cryptodev_driver qat_crypto_drv;
+
 RTE_PMD_REGISTER_PCI(CRYPTODEV_NAME_QAT_SYM_PMD, rte_qat_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(CRYPTODEV_NAME_QAT_SYM_PMD, pci_id_qat_map);
-RTE_PMD_REGISTER_CRYPTO_DRIVER(rte_qat_pmd, cryptodev_qat_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(qat_crypto_drv, rte_qat_pmd,
+		cryptodev_qat_driver_id);
diff --git a/drivers/crypto/scheduler/scheduler_pmd.c b/drivers/crypto/scheduler/scheduler_pmd.c
index 400fc4f..3170f7f 100644
--- a/drivers/crypto/scheduler/scheduler_pmd.c
+++ b/drivers/crypto/scheduler/scheduler_pmd.c
@@ -505,6 +505,8 @@ static struct rte_vdev_driver cryptodev_scheduler_pmd_drv = {
 	.remove = cryptodev_scheduler_remove
 };
 
+static struct cryptodev_driver scheduler_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_SCHEDULER_PMD,
 	cryptodev_scheduler_pmd_drv);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_SCHEDULER_PMD,
@@ -512,5 +514,6 @@ RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_SCHEDULER_PMD,
 	"max_nb_sessions=<int> "
 	"socket_id=<int> "
 	"slave=<name>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_scheduler_pmd_drv,
+RTE_PMD_REGISTER_CRYPTO_DRIVER(scheduler_crypto_drv,
+		cryptodev_scheduler_pmd_drv,
 		cryptodev_driver_id);
diff --git a/drivers/crypto/snow3g/rte_snow3g_pmd.c b/drivers/crypto/snow3g/rte_snow3g_pmd.c
index dad4506..b2f6222 100644
--- a/drivers/crypto/snow3g/rte_snow3g_pmd.c
+++ b/drivers/crypto/snow3g/rte_snow3g_pmd.c
@@ -661,10 +661,13 @@ static struct rte_vdev_driver cryptodev_snow3g_pmd_drv = {
 	.remove = cryptodev_snow3g_remove
 };
 
+static struct cryptodev_driver snow3g_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_SNOW3G_PMD, cryptodev_snow3g_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_SNOW3G_PMD, cryptodev_snow3g_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_SNOW3G_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_snow3g_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(snow3g_crypto_drv, cryptodev_snow3g_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index b301711..70966d4 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -565,9 +565,12 @@ static struct rte_vdev_driver cryptodev_zuc_pmd_drv = {
 	.remove = cryptodev_zuc_remove
 };
 
+static struct cryptodev_driver zuc_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_ZUC_PMD, cryptodev_zuc_pmd_drv);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_ZUC_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_zuc_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(zuc_crypto_drv, cryptodev_zuc_pmd_drv,
+		cryptodev_driver_id);
diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 327d7e8..a239395 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -1362,12 +1362,6 @@ TAILQ_HEAD(cryptodev_driver_list, cryptodev_driver);
 static struct cryptodev_driver_list cryptodev_driver_list =
 	TAILQ_HEAD_INITIALIZER(cryptodev_driver_list);
 
-struct cryptodev_driver {
-	TAILQ_ENTRY(cryptodev_driver) next; /**< Next in list. */
-	const struct rte_driver *driver;
-	uint8_t id;
-};
-
 int
 rte_cryptodev_driver_id_get(const char *name)
 {
@@ -1399,15 +1393,13 @@ rte_cryptodev_driver_name_get(uint8_t driver_id)
 }
 
 uint8_t
-rte_cryptodev_allocate_driver(const struct rte_driver *drv)
+rte_cryptodev_allocate_driver(struct cryptodev_driver *crypto_drv,
+		const struct rte_driver *drv)
 {
-	struct cryptodev_driver *driver;
-
-	driver = malloc(sizeof(*driver));
-	driver->driver = drv;
-	driver->id = nb_drivers;
+	crypto_drv->driver = drv;
+	crypto_drv->id = nb_drivers;
 
-	TAILQ_INSERT_TAIL(&cryptodev_driver_list, driver, next);
+	TAILQ_INSERT_TAIL(&cryptodev_driver_list, crypto_drv, next);
 
 	return nb_drivers++;
 }
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 7ec9c4b..5225a5b 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -1025,26 +1025,6 @@ int rte_cryptodev_driver_id_get(const char *name);
  */
 const char *rte_cryptodev_driver_name_get(uint8_t driver_id);
 
-/**
- * @internal
- * Allocate Cryptodev driver.
- *
- * @param driver
- *   Pointer to rte_driver.
- * @return
- *  The driver type identifier
- */
-uint8_t rte_cryptodev_allocate_driver(const struct rte_driver *driver);
-
-
-#define RTE_PMD_REGISTER_CRYPTO_DRIVER(drv, driver_id)\
-RTE_INIT(init_ ##driver_id);\
-static void init_ ##driver_id(void)\
-{\
-	driver_id = rte_cryptodev_allocate_driver(&(drv).driver);\
-}
-
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
index c983eb2..ba074e1 100644
--- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
+++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
@@ -65,6 +65,13 @@ struct rte_cryptodev_global {
 	uint8_t max_devs;		/**< Max number of devices */
 };
 
+/* Cryptodev driver, containing the driver ID */
+struct cryptodev_driver {
+	TAILQ_ENTRY(cryptodev_driver) next; /**< Next in list. */
+	const struct rte_driver *driver;
+	uint8_t id;
+};
+
 /** pointer to global crypto devices data structure. */
 extern struct rte_cryptodev_global *rte_cryptodev_globals;
 
@@ -405,6 +412,29 @@ void rte_cryptodev_pmd_callback_process(struct rte_cryptodev *dev,
 int
 rte_cryptodev_pmd_create_dev_name(char *name, const char *dev_name_prefix);
 
+/**
+ * @internal
+ * Allocate Cryptodev driver.
+ *
+ * @param crypto_drv
+ *   Pointer to cryptodev_driver.
+ * @param drv
+ *   Pointer to rte_driver.
+ *
+ * @return
+ *  The driver type identifier
+ */
+uint8_t rte_cryptodev_allocate_driver(struct cryptodev_driver *crypto_drv,
+		const struct rte_driver *drv);
+
+
+#define RTE_PMD_REGISTER_CRYPTO_DRIVER(crypto_drv, drv, driver_id)\
+RTE_INIT(init_ ##driver_id);\
+static void init_ ##driver_id(void)\
+{\
+	driver_id = rte_cryptodev_allocate_driver(&crypto_drv, &(drv).driver);\
+}
+
 static inline void *
 get_session_private_data(const struct rte_cryptodev_sym_session *sess,
 		uint8_t driver_id) {
-- 
2.9.4

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-07-31  9:18 ` [dpdk-dev] [PATCH v2] " Pablo de Lara
@ 2017-07-31 19:33   ` Thomas Monjalon
  2017-08-01  7:10     ` De Lara Guarch, Pablo
  2017-08-01  8:13     ` Sergio Gonzalez Monroy
  2017-08-16  2:41   ` [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically Pablo de Lara
  1 sibling, 2 replies; 15+ messages in thread
From: Thomas Monjalon @ 2017-07-31 19:33 UTC (permalink / raw)
  To: Pablo de Lara; +Cc: sergio.gonzalez.monroy, dev

31/07/2017 11:18, Pablo de Lara:
> When register a crypto driver, a cryptodev driver
> structure was being allocated, using malloc.
> Since this call may fail, it is safer to allocate
> this memory statically in each PMD, so driver registration
> will never fail.
> 
> Coverity issue: 158645
> 
> Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
> 
> Changes in v2:
> 
> - Allocate statically the cryptodev driver structure,
>   instead of using malloc, that can potentially fail.
> 
>  drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
>  drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
>  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
>  drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
>  drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
>  drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
>  drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
>  drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
>  drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
>  drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
>  lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
>  lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
>  lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30 +++++++++++++++++++++++++++++
>  14 files changed, 83 insertions(+), 47 deletions(-)

This is a big change for a small/unlikely issue.
The main benefit of this patch is an allocation cleanup.
I think it is better to wait 17.11 cycle to integrate it.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-07-31 19:33   ` Thomas Monjalon
@ 2017-08-01  7:10     ` De Lara Guarch, Pablo
  2017-08-01  8:13     ` Sergio Gonzalez Monroy
  1 sibling, 0 replies; 15+ messages in thread
From: De Lara Guarch, Pablo @ 2017-08-01  7:10 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Gonzalez Monroy, Sergio, dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, July 31, 2017 8:33 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
> 
> 31/07/2017 11:18, Pablo de Lara:
> > When register a crypto driver, a cryptodev driver structure was being
> > allocated, using malloc.
> > Since this call may fail, it is safer to allocate this memory
> > statically in each PMD, so driver registration will never fail.
> >
> > Coverity issue: 158645
> >
> > Fixes: 7a364faef185 ("cryptodev: remove crypto device type
> > enumeration")
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > ---
> >
> > Changes in v2:
> >
> > - Allocate statically the cryptodev driver structure,
> >   instead of using malloc, that can potentially fail.
> >
> >  drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
> >  drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
> >  drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
> >  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
> >  drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
> >  drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
> >  drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
> >  drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
> >  drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
> >  drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
> >  drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
> >  lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
> >  lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
> >  lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30
> +++++++++++++++++++++++++++++
> >  14 files changed, 83 insertions(+), 47 deletions(-)
> 
> This is a big change for a small/unlikely issue.
> The main benefit of this patch is an allocation cleanup.
> I think it is better to wait 17.11 cycle to integrate it.

Makes sense, Thomas.

Thanks,
Pablo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-07-31 19:33   ` Thomas Monjalon
  2017-08-01  7:10     ` De Lara Guarch, Pablo
@ 2017-08-01  8:13     ` Sergio Gonzalez Monroy
  2017-08-01  9:35       ` Thomas Monjalon
  1 sibling, 1 reply; 15+ messages in thread
From: Sergio Gonzalez Monroy @ 2017-08-01  8:13 UTC (permalink / raw)
  To: Thomas Monjalon, Pablo de Lara; +Cc: dev

On 31/07/2017 20:33, Thomas Monjalon wrote:
> 31/07/2017 11:18, Pablo de Lara:
>> When register a crypto driver, a cryptodev driver
>> structure was being allocated, using malloc.
>> Since this call may fail, it is safer to allocate
>> this memory statically in each PMD, so driver registration
>> will never fail.
>>
>> Coverity issue: 158645
>>
>> Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")
>>
>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>> ---
>>
>> Changes in v2:
>>
>> - Allocate statically the cryptodev driver structure,
>>    instead of using malloc, that can potentially fail.
>>
>>   drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
>>   drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
>>   drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
>>   drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
>>   drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
>>   drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
>>   drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
>>   drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
>>   drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
>>   drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
>>   drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
>>   lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
>>   lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
>>   lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30 +++++++++++++++++++++++++++++
>>   14 files changed, 83 insertions(+), 47 deletions(-)
> This is a big change for a small/unlikely issue.
> The main benefit of this patch is an allocation cleanup.
> I think it is better to wait 17.11 cycle to integrate it.

We initially thought of exit given that it is a constructor and if you 
fail to allocate memory at this stage, things are likely not going to 
work out anyway.

The patch is an API change, do we really want to break again (we are 
breaking in this release) next release?

Thanks,
Sergio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-08-01  8:13     ` Sergio Gonzalez Monroy
@ 2017-08-01  9:35       ` Thomas Monjalon
  2017-08-01 10:17         ` Sergio Gonzalez Monroy
  0 siblings, 1 reply; 15+ messages in thread
From: Thomas Monjalon @ 2017-08-01  9:35 UTC (permalink / raw)
  To: Sergio Gonzalez Monroy; +Cc: Pablo de Lara, dev

01/08/2017 10:13, Sergio Gonzalez Monroy:
> On 31/07/2017 20:33, Thomas Monjalon wrote:
> > 31/07/2017 11:18, Pablo de Lara:
> >> When register a crypto driver, a cryptodev driver
> >> structure was being allocated, using malloc.
> >> Since this call may fail, it is safer to allocate
> >> this memory statically in each PMD, so driver registration
> >> will never fail.
> >>
> >> Coverity issue: 158645
> >>
> >> Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")
> >>
> >> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> >> ---
> >>
> >> Changes in v2:
> >>
> >> - Allocate statically the cryptodev driver structure,
> >>    instead of using malloc, that can potentially fail.
> >>
> >>   drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
> >>   drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
> >>   drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
> >>   drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
> >>   drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
> >>   drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
> >>   drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
> >>   drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
> >>   drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
> >>   drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
> >>   drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
> >>   lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
> >>   lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
> >>   lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30 +++++++++++++++++++++++++++++
> >>   14 files changed, 83 insertions(+), 47 deletions(-)
> > This is a big change for a small/unlikely issue.
> > The main benefit of this patch is an allocation cleanup.
> > I think it is better to wait 17.11 cycle to integrate it.
> 
> We initially thought of exit given that it is a constructor and if you 
> fail to allocate memory at this stage, things are likely not going to 
> work out anyway.

You don't know how the application wants to manage it.

> The patch is an API change, do we really want to break again (we are 
> breaking in this release) next release?

Good question. Any opinions?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-08-01  9:35       ` Thomas Monjalon
@ 2017-08-01 10:17         ` Sergio Gonzalez Monroy
  2017-08-01 10:48           ` De Lara Guarch, Pablo
  0 siblings, 1 reply; 15+ messages in thread
From: Sergio Gonzalez Monroy @ 2017-08-01 10:17 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Pablo de Lara, dev

On 01/08/2017 10:35, Thomas Monjalon wrote:
> 01/08/2017 10:13, Sergio Gonzalez Monroy:
>> On 31/07/2017 20:33, Thomas Monjalon wrote:
>>> 31/07/2017 11:18, Pablo de Lara:
>>>> When register a crypto driver, a cryptodev driver
>>>> structure was being allocated, using malloc.
>>>> Since this call may fail, it is safer to allocate
>>>> this memory statically in each PMD, so driver registration
>>>> will never fail.
>>>>
>>>> Coverity issue: 158645
>>>>
>>>> Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")
>>>>
>>>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>>
>>>> - Allocate statically the cryptodev driver structure,
>>>>     instead of using malloc, that can potentially fail.
>>>>
>>>>    drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
>>>>    drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
>>>>    drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
>>>>    drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
>>>>    drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
>>>>    drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
>>>>    drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
>>>>    drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
>>>>    drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
>>>>    drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
>>>>    drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
>>>>    lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
>>>>    lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
>>>>    lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30 +++++++++++++++++++++++++++++
>>>>    14 files changed, 83 insertions(+), 47 deletions(-)
>>> This is a big change for a small/unlikely issue.
>>> The main benefit of this patch is an allocation cleanup.
>>> I think it is better to wait 17.11 cycle to integrate it.
>> We initially thought of exit given that it is a constructor and if you
>> fail to allocate memory at this stage, things are likely not going to
>> work out anyway.
> You don't know how the application wants to manage it.

IMHO setting an internal variable indicating an error in constructors 
and then reporting the problem during EAL init seems overly complex.
I think the proposed change is a cleaner solution.

>> The patch is an API change, do we really want to break again (we are
>> breaking in this release) next release?
> Good question. Any opinions?

Merge the patch unless there are already outstanding and/or planned 
changes for the next release that are going to break ABI/API?


Thanks,
Sergio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-08-01 10:17         ` Sergio Gonzalez Monroy
@ 2017-08-01 10:48           ` De Lara Guarch, Pablo
  2017-08-01 12:36             ` Thomas Monjalon
  0 siblings, 1 reply; 15+ messages in thread
From: De Lara Guarch, Pablo @ 2017-08-01 10:48 UTC (permalink / raw)
  To: Gonzalez Monroy, Sergio, Thomas Monjalon; +Cc: dev, Jan Blunck



> -----Original Message-----
> From: Gonzalez Monroy, Sergio
> Sent: Tuesday, August 1, 2017 11:18 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v2] cryptodev: fix NULL pointer dereference
> 
> On 01/08/2017 10:35, Thomas Monjalon wrote:
> > 01/08/2017 10:13, Sergio Gonzalez Monroy:
> >> On 31/07/2017 20:33, Thomas Monjalon wrote:
> >>> 31/07/2017 11:18, Pablo de Lara:
> >>>> When register a crypto driver, a cryptodev driver structure was
> >>>> being allocated, using malloc.
> >>>> Since this call may fail, it is safer to allocate this memory
> >>>> statically in each PMD, so driver registration will never fail.
> >>>>
> >>>> Coverity issue: 158645
> >>>>
> >>>> Fixes: 7a364faef185 ("cryptodev: remove crypto device type
> >>>> enumeration")
> >>>>
> >>>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>>
> >>>> - Allocate statically the cryptodev driver structure,
> >>>>     instead of using malloc, that can potentially fail.
> >>>>
> >>>>    drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
> >>>>    drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
> >>>>    drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
> >>>>    drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
> >>>>    drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
> >>>>    drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
> >>>>    drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
> >>>>    drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
> >>>>    drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
> >>>>    drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
> >>>>    drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
> >>>>    lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
> >>>>    lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
> >>>>    lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30
> +++++++++++++++++++++++++++++
> >>>>    14 files changed, 83 insertions(+), 47 deletions(-)
> >>> This is a big change for a small/unlikely issue.
> >>> The main benefit of this patch is an allocation cleanup.
> >>> I think it is better to wait 17.11 cycle to integrate it.
> >> We initially thought of exit given that it is a constructor and if
> >> you fail to allocate memory at this stage, things are likely not
> >> going to work out anyway.
> > You don't know how the application wants to manage it.
> 
> IMHO setting an internal variable indicating an error in constructors and
> then reporting the problem during EAL init seems overly complex.
> I think the proposed change is a cleaner solution.
> 
> >> The patch is an API change, do we really want to break again (we are
> >> breaking in this release) next release?
> > Good question. Any opinions?
> 
> Merge the patch unless there are already outstanding and/or planned
> changes for the next release that are going to break ABI/API?

There is another patchset that was postponed for next release, because the
compilation was broken in one of the patches (just double checked and it is easy to fix),
and by then, I thought that no ABI/API was being broken,
but it will be (my bad here). This is the patchset I am talking about:

[PATCH v2 0/4] cryptodev vdev changes for -rc2
 http://dpdk.org/ml/archives/dev/2017-July/071160.html

So we have two options here:
1 - Get both patches now, since we are breaking the ABI in this release (as Sergio pointed out).
2 - Postpone both changes to next release.

I would go for option 1, as there are no other changes expected for next release
(only one function, rte_cryptodev_create_vdev, will be removed).

Thanks,
Pablo

> 
> 
> Thanks,
> Sergio

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference
  2017-08-01 10:48           ` De Lara Guarch, Pablo
@ 2017-08-01 12:36             ` Thomas Monjalon
  0 siblings, 0 replies; 15+ messages in thread
From: Thomas Monjalon @ 2017-08-01 12:36 UTC (permalink / raw)
  To: De Lara Guarch, Pablo, Gonzalez Monroy, Sergio; +Cc: dev, Jan Blunck

01/08/2017 12:48, De Lara Guarch, Pablo:
> From: Gonzalez Monroy, Sergio
> > On 01/08/2017 10:35, Thomas Monjalon wrote:
> > > 01/08/2017 10:13, Sergio Gonzalez Monroy:
> > >> On 31/07/2017 20:33, Thomas Monjalon wrote:
> > >>> 31/07/2017 11:18, Pablo de Lara:
> > >>>> When register a crypto driver, a cryptodev driver structure was
> > >>>> being allocated, using malloc.
> > >>>> Since this call may fail, it is safer to allocate this memory
> > >>>> statically in each PMD, so driver registration will never fail.
> > >>>>
> > >>>> Coverity issue: 158645
> > >>>>
> > >>>> Fixes: 7a364faef185 ("cryptodev: remove crypto device type
> > >>>> enumeration")
> > >>>>
> > >>>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > >>>> ---
> > >>>>
> > >>>> Changes in v2:
> > >>>>
> > >>>> - Allocate statically the cryptodev driver structure,
> > >>>>     instead of using malloc, that can potentially fail.
> > >>>>
> > >>>>    drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
> > >>>>    drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
> > >>>>    drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
> > >>>>    drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
> > >>>>    drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
> > >>>>    drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
> > >>>>    drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
> > >>>>    drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
> > >>>>    drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
> > >>>>    drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
> > >>>>    drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
> > >>>>    lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
> > >>>>    lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
> > >>>>    lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30
> > +++++++++++++++++++++++++++++
> > >>>>    14 files changed, 83 insertions(+), 47 deletions(-)
> > >>> This is a big change for a small/unlikely issue.
> > >>> The main benefit of this patch is an allocation cleanup.
> > >>> I think it is better to wait 17.11 cycle to integrate it.
> > >> We initially thought of exit given that it is a constructor and if
> > >> you fail to allocate memory at this stage, things are likely not
> > >> going to work out anyway.
> > > You don't know how the application wants to manage it.
> > 
> > IMHO setting an internal variable indicating an error in constructors and
> > then reporting the problem during EAL init seems overly complex.
> > I think the proposed change is a cleaner solution.
> > 
> > >> The patch is an API change, do we really want to break again (we are
> > >> breaking in this release) next release?
> > > Good question. Any opinions?
> > 
> > Merge the patch unless there are already outstanding and/or planned
> > changes for the next release that are going to break ABI/API?
> 
> There is another patchset that was postponed for next release, because the
> compilation was broken in one of the patches (just double checked and it is easy to fix),
> and by then, I thought that no ABI/API was being broken,
> but it will be (my bad here). This is the patchset I am talking about:
> 
> [PATCH v2 0/4] cryptodev vdev changes for -rc2
>  http://dpdk.org/ml/archives/dev/2017-July/071160.html
> 
> So we have two options here:
> 1 - Get both patches now, since we are breaking the ABI in this release (as Sergio pointed out).
> 2 - Postpone both changes to next release.
> 
> I would go for option 1, as there are no other changes expected for next release
> (only one function, rte_cryptodev_create_vdev, will be removed).

Given that there is a new release every 3 months, I prefer the safe way.
Anyway, if a function is going to be removed, the API and ABI will change.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically
  2017-07-31  9:18 ` [dpdk-dev] [PATCH v2] " Pablo de Lara
  2017-07-31 19:33   ` Thomas Monjalon
@ 2017-08-16  2:41   ` Pablo de Lara
  2017-09-04 10:38     ` Rybalchenko, Kirill
  1 sibling, 1 reply; 15+ messages in thread
From: Pablo de Lara @ 2017-08-16  2:41 UTC (permalink / raw)
  To: declan.doherty, fiona.trahe, deepak.k.jain, john.griffin,
	jerin.jacob, akhil.goyal, hemant.agrawal
  Cc: dev, Pablo de Lara

When register a crypto driver, a cryptodev driver
structure was being allocated, using malloc.
Since this call may fail, it is safer to allocate
this memory statically in each PMD, so driver registration
will never fail.

Coverity issue: 158645

Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---

Changes in v3:

- Added documentation (removed deprecation notice and added API Change)
- Modified title to something more explicit

Changes in v2:

- Allocate statically the cryptodev driver structure,
  instead of using malloc, that can potentially fail.

 doc/guides/rel_notes/deprecation.rst        |  6 ------
 doc/guides/rel_notes/release_17_11.rst      |  5 +++++
 drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
 drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
 drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
 drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
 drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
 drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
 drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
 drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
 drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
 drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
 lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
 lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
 lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30 +++++++++++++++++++++++++++++
 16 files changed, 88 insertions(+), 53 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 3362f33..8be626e 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -106,12 +106,6 @@ Deprecation Notices
 
   - ``rte_cryptodev_vdev_pmd_init``
 
-* cryptodev: the following function will have an extra parameter, passing a
-  statically allocated crypto driver structure, instead of calling malloc,
-  in 17.11:
-
-  - ``rte_cryptodev_allocate_driver``
-
 * librte_meter: The API will change to accommodate configuration profiles.
   Most of the API functions will have an additional opaque parameter.
 
diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
index 170f4f9..cdb99f4 100644
--- a/doc/guides/rel_notes/release_17_11.rst
+++ b/doc/guides/rel_notes/release_17_11.rst
@@ -110,6 +110,11 @@ API Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **Modified the rte_cryptodev_allocate_driver function in the cryptodev library.**
+
+  The function ``rte_cryptodev_allocate_driver()`` has been modified.
+  An extra parameter ``struct cryptodev_driver *crypto_drv`` has been added.
+
 
 ABI Changes
 -----------
diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
index d9c91d0..b59f399 100644
--- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
+++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
@@ -630,10 +630,13 @@ static struct rte_vdev_driver aesni_gcm_pmd_drv = {
 	.remove = aesni_gcm_remove
 };
 
+static struct cryptodev_driver aesni_gcm_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_AESNI_GCM_PMD, aesni_gcm_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_AESNI_GCM_PMD, cryptodev_aesni_gcm_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_AESNI_GCM_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_gcm_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_gcm_crypto_drv, aesni_gcm_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
index 16e1451..5846bc9 100644
--- a/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
+++ b/drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c
@@ -823,10 +823,14 @@ static struct rte_vdev_driver cryptodev_aesni_mb_pmd_drv = {
 	.remove = cryptodev_aesni_mb_remove
 };
 
+static struct cryptodev_driver aesni_mb_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_AESNI_MB_PMD, cryptodev_aesni_mb_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_AESNI_MB_PMD, cryptodev_aesni_mb_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_AESNI_MB_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_aesni_mb_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(aesni_mb_crypto_drv,
+		cryptodev_aesni_mb_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/armv8/rte_armv8_pmd.c b/drivers/crypto/armv8/rte_armv8_pmd.c
index a5c39c9..b0d68d1 100644
--- a/drivers/crypto/armv8/rte_armv8_pmd.c
+++ b/drivers/crypto/armv8/rte_armv8_pmd.c
@@ -882,15 +882,18 @@ cryptodev_armv8_crypto_uninit(struct rte_vdev_device *vdev)
 	return 0;
 }
 
-static struct rte_vdev_driver armv8_crypto_drv = {
+static struct rte_vdev_driver armv8_crypto_pmd_drv = {
 	.probe = cryptodev_armv8_crypto_init,
 	.remove = cryptodev_armv8_crypto_uninit
 };
 
-RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_ARMV8_PMD, armv8_crypto_drv);
+static struct cryptodev_driver armv8_crypto_drv;
+
+RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_ARMV8_PMD, armv8_crypto_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_ARMV8_PMD, cryptodev_armv8_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_ARMV8_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(armv8_crypto_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(armv8_crypto_drv, armv8_crypto_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index e0f6cfc..dae296f 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -2008,5 +2008,8 @@ static struct rte_dpaa2_driver rte_dpaa2_sec_driver = {
 	.remove = cryptodev_dpaa2_sec_remove,
 };
 
+static struct cryptodev_driver dpaa2_sec_crypto_drv;
+
 RTE_PMD_REGISTER_DPAA2(CRYPTODEV_NAME_DPAA2_SEC_PMD, rte_dpaa2_sec_driver);
-RTE_PMD_REGISTER_CRYPTO_DRIVER(rte_dpaa2_sec_driver, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(dpaa2_sec_crypto_drv, rte_dpaa2_sec_driver,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/kasumi/rte_kasumi_pmd.c b/drivers/crypto/kasumi/rte_kasumi_pmd.c
index 38cd8a9..84cce34 100644
--- a/drivers/crypto/kasumi/rte_kasumi_pmd.c
+++ b/drivers/crypto/kasumi/rte_kasumi_pmd.c
@@ -661,10 +661,13 @@ static struct rte_vdev_driver cryptodev_kasumi_pmd_drv = {
 	.remove = cryptodev_kasumi_remove
 };
 
+static struct cryptodev_driver kasumi_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_KASUMI_PMD, cryptodev_kasumi_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_KASUMI_PMD, cryptodev_kasumi_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_KASUMI_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_kasumi_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(kasumi_crypto_drv, cryptodev_kasumi_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/null/null_crypto_pmd.c b/drivers/crypto/null/null_crypto_pmd.c
index 2c82725..d5d2bb3 100644
--- a/drivers/crypto/null/null_crypto_pmd.c
+++ b/drivers/crypto/null/null_crypto_pmd.c
@@ -286,10 +286,13 @@ static struct rte_vdev_driver cryptodev_null_pmd_drv = {
 	.remove = cryptodev_null_remove_dev,
 };
 
+static struct cryptodev_driver null_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_NULL_PMD, cryptodev_null_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_NULL_PMD, cryptodev_null_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_NULL_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_null_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(null_crypto_drv, cryptodev_null_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 0bd5f98..5b863c8 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -1502,10 +1502,13 @@ static struct rte_vdev_driver cryptodev_openssl_pmd_drv = {
 	.remove = cryptodev_openssl_remove
 };
 
+static struct cryptodev_driver openssl_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_OPENSSL_PMD,
 	cryptodev_openssl_pmd_drv);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_OPENSSL_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_openssl_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(openssl_crypto_drv, cryptodev_openssl_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/qat/rte_qat_cryptodev.c b/drivers/crypto/qat/rte_qat_cryptodev.c
index 7d56fca..3d9f3c8 100644
--- a/drivers/crypto/qat/rte_qat_cryptodev.c
+++ b/drivers/crypto/qat/rte_qat_cryptodev.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2015-2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2015-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -169,6 +169,9 @@ static struct rte_pci_driver rte_qat_pmd = {
 	.remove = crypto_qat_pci_remove
 };
 
+static struct cryptodev_driver qat_crypto_drv;
+
 RTE_PMD_REGISTER_PCI(CRYPTODEV_NAME_QAT_SYM_PMD, rte_qat_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(CRYPTODEV_NAME_QAT_SYM_PMD, pci_id_qat_map);
-RTE_PMD_REGISTER_CRYPTO_DRIVER(rte_qat_pmd, cryptodev_qat_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(qat_crypto_drv, rte_qat_pmd,
+		cryptodev_qat_driver_id);
diff --git a/drivers/crypto/scheduler/scheduler_pmd.c b/drivers/crypto/scheduler/scheduler_pmd.c
index 400fc4f..3170f7f 100644
--- a/drivers/crypto/scheduler/scheduler_pmd.c
+++ b/drivers/crypto/scheduler/scheduler_pmd.c
@@ -505,6 +505,8 @@ static struct rte_vdev_driver cryptodev_scheduler_pmd_drv = {
 	.remove = cryptodev_scheduler_remove
 };
 
+static struct cryptodev_driver scheduler_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_SCHEDULER_PMD,
 	cryptodev_scheduler_pmd_drv);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_SCHEDULER_PMD,
@@ -512,5 +514,6 @@ RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_SCHEDULER_PMD,
 	"max_nb_sessions=<int> "
 	"socket_id=<int> "
 	"slave=<name>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_scheduler_pmd_drv,
+RTE_PMD_REGISTER_CRYPTO_DRIVER(scheduler_crypto_drv,
+		cryptodev_scheduler_pmd_drv,
 		cryptodev_driver_id);
diff --git a/drivers/crypto/snow3g/rte_snow3g_pmd.c b/drivers/crypto/snow3g/rte_snow3g_pmd.c
index dad4506..b2f6222 100644
--- a/drivers/crypto/snow3g/rte_snow3g_pmd.c
+++ b/drivers/crypto/snow3g/rte_snow3g_pmd.c
@@ -661,10 +661,13 @@ static struct rte_vdev_driver cryptodev_snow3g_pmd_drv = {
 	.remove = cryptodev_snow3g_remove
 };
 
+static struct cryptodev_driver snow3g_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_SNOW3G_PMD, cryptodev_snow3g_pmd_drv);
 RTE_PMD_REGISTER_ALIAS(CRYPTODEV_NAME_SNOW3G_PMD, cryptodev_snow3g_pmd);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_SNOW3G_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_snow3g_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(snow3g_crypto_drv, cryptodev_snow3g_pmd_drv,
+		cryptodev_driver_id);
diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index b301711..70966d4 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -565,9 +565,12 @@ static struct rte_vdev_driver cryptodev_zuc_pmd_drv = {
 	.remove = cryptodev_zuc_remove
 };
 
+static struct cryptodev_driver zuc_crypto_drv;
+
 RTE_PMD_REGISTER_VDEV(CRYPTODEV_NAME_ZUC_PMD, cryptodev_zuc_pmd_drv);
 RTE_PMD_REGISTER_PARAM_STRING(CRYPTODEV_NAME_ZUC_PMD,
 	"max_nb_queue_pairs=<int> "
 	"max_nb_sessions=<int> "
 	"socket_id=<int>");
-RTE_PMD_REGISTER_CRYPTO_DRIVER(cryptodev_zuc_pmd_drv, cryptodev_driver_id);
+RTE_PMD_REGISTER_CRYPTO_DRIVER(zuc_crypto_drv, cryptodev_zuc_pmd_drv,
+		cryptodev_driver_id);
diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 327d7e8..a239395 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -1362,12 +1362,6 @@ TAILQ_HEAD(cryptodev_driver_list, cryptodev_driver);
 static struct cryptodev_driver_list cryptodev_driver_list =
 	TAILQ_HEAD_INITIALIZER(cryptodev_driver_list);
 
-struct cryptodev_driver {
-	TAILQ_ENTRY(cryptodev_driver) next; /**< Next in list. */
-	const struct rte_driver *driver;
-	uint8_t id;
-};
-
 int
 rte_cryptodev_driver_id_get(const char *name)
 {
@@ -1399,15 +1393,13 @@ rte_cryptodev_driver_name_get(uint8_t driver_id)
 }
 
 uint8_t
-rte_cryptodev_allocate_driver(const struct rte_driver *drv)
+rte_cryptodev_allocate_driver(struct cryptodev_driver *crypto_drv,
+		const struct rte_driver *drv)
 {
-	struct cryptodev_driver *driver;
-
-	driver = malloc(sizeof(*driver));
-	driver->driver = drv;
-	driver->id = nb_drivers;
+	crypto_drv->driver = drv;
+	crypto_drv->id = nb_drivers;
 
-	TAILQ_INSERT_TAIL(&cryptodev_driver_list, driver, next);
+	TAILQ_INSERT_TAIL(&cryptodev_driver_list, crypto_drv, next);
 
 	return nb_drivers++;
 }
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index 7ec9c4b..5225a5b 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -1025,26 +1025,6 @@ int rte_cryptodev_driver_id_get(const char *name);
  */
 const char *rte_cryptodev_driver_name_get(uint8_t driver_id);
 
-/**
- * @internal
- * Allocate Cryptodev driver.
- *
- * @param driver
- *   Pointer to rte_driver.
- * @return
- *  The driver type identifier
- */
-uint8_t rte_cryptodev_allocate_driver(const struct rte_driver *driver);
-
-
-#define RTE_PMD_REGISTER_CRYPTO_DRIVER(drv, driver_id)\
-RTE_INIT(init_ ##driver_id);\
-static void init_ ##driver_id(void)\
-{\
-	driver_id = rte_cryptodev_allocate_driver(&(drv).driver);\
-}
-
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
index c983eb2..ba074e1 100644
--- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
+++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
@@ -65,6 +65,13 @@ struct rte_cryptodev_global {
 	uint8_t max_devs;		/**< Max number of devices */
 };
 
+/* Cryptodev driver, containing the driver ID */
+struct cryptodev_driver {
+	TAILQ_ENTRY(cryptodev_driver) next; /**< Next in list. */
+	const struct rte_driver *driver;
+	uint8_t id;
+};
+
 /** pointer to global crypto devices data structure. */
 extern struct rte_cryptodev_global *rte_cryptodev_globals;
 
@@ -405,6 +412,29 @@ void rte_cryptodev_pmd_callback_process(struct rte_cryptodev *dev,
 int
 rte_cryptodev_pmd_create_dev_name(char *name, const char *dev_name_prefix);
 
+/**
+ * @internal
+ * Allocate Cryptodev driver.
+ *
+ * @param crypto_drv
+ *   Pointer to cryptodev_driver.
+ * @param drv
+ *   Pointer to rte_driver.
+ *
+ * @return
+ *  The driver type identifier
+ */
+uint8_t rte_cryptodev_allocate_driver(struct cryptodev_driver *crypto_drv,
+		const struct rte_driver *drv);
+
+
+#define RTE_PMD_REGISTER_CRYPTO_DRIVER(crypto_drv, drv, driver_id)\
+RTE_INIT(init_ ##driver_id);\
+static void init_ ##driver_id(void)\
+{\
+	driver_id = rte_cryptodev_allocate_driver(&crypto_drv, &(drv).driver);\
+}
+
 static inline void *
 get_session_private_data(const struct rte_cryptodev_sym_session *sess,
 		uint8_t driver_id) {
-- 
2.9.4

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically
  2017-08-16  2:41   ` [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically Pablo de Lara
@ 2017-09-04 10:38     ` Rybalchenko, Kirill
  2017-09-06 10:27       ` De Lara Guarch, Pablo
  0 siblings, 1 reply; 15+ messages in thread
From: Rybalchenko, Kirill @ 2017-09-04 10:38 UTC (permalink / raw)
  To: De Lara Guarch, Pablo, Doherty, Declan, Trahe, Fiona, Jain,
	Deepak K, Griffin, John, jerin.jacob, akhil.goyal,
	hemant.agrawal
  Cc: dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> Sent: Wednesday 16 August 2017 03:42
> To: Doherty, Declan <declan.doherty@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
> Griffin, John <john.griffin@intel.com>; jerin.jacob@caviumnetworks.com;
> akhil.goyal@nxp.com; hemant.agrawal@nxp.com
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically
> 
> When register a crypto driver, a cryptodev driver structure was being
> allocated, using malloc.
> Since this call may fail, it is safer to allocate this memory statically in each
> PMD, so driver registration will never fail.
> 
> Coverity issue: 158645
> 
> Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
> 
> Changes in v3:
> 
> - Added documentation (removed deprecation notice and added API
> Change)
> - Modified title to something more explicit
> 
> Changes in v2:
> 
> - Allocate statically the cryptodev driver structure,
>   instead of using malloc, that can potentially fail.
> 
>  doc/guides/rel_notes/deprecation.rst        |  6 ------
>  doc/guides/rel_notes/release_17_11.rst      |  5 +++++
>  drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
>  drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
>  drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
>  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
>  drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
>  drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
>  drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
>  drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
>  drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
>  drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
>  drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
>  lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
>  lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
>  lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30
> +++++++++++++++++++++++++++++
>  16 files changed, 88 insertions(+), 53 deletions(-)
> 

Reviewed-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically
  2017-09-04 10:38     ` Rybalchenko, Kirill
@ 2017-09-06 10:27       ` De Lara Guarch, Pablo
  0 siblings, 0 replies; 15+ messages in thread
From: De Lara Guarch, Pablo @ 2017-09-06 10:27 UTC (permalink / raw)
  To: Rybalchenko, Kirill, Doherty, Declan, Trahe, Fiona, Jain,
	Deepak K, Griffin, John, jerin.jacob, akhil.goyal,
	hemant.agrawal
  Cc: dev



> -----Original Message-----
> From: Rybalchenko, Kirill
> Sent: Monday, September 4, 2017 11:39 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty,
> Declan <declan.doherty@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
> Jain, Deepak K <deepak.k.jain@intel.com>; Griffin, John
> <john.griffin@intel.com>; jerin.jacob@caviumnetworks.com;
> akhil.goyal@nxp.com; hemant.agrawal@nxp.com
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure
> statically
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> > Sent: Wednesday 16 August 2017 03:42
> > To: Doherty, Declan <declan.doherty@intel.com>; Trahe, Fiona
> > <fiona.trahe@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
> > Griffin, John <john.griffin@intel.com>;
> > jerin.jacob@caviumnetworks.com; akhil.goyal@nxp.com;
> > hemant.agrawal@nxp.com
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Subject: [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure
> > statically
> >
> > When register a crypto driver, a cryptodev driver structure was being
> > allocated, using malloc.
> > Since this call may fail, it is safer to allocate this memory
> > statically in each PMD, so driver registration will never fail.
> >
> > Coverity issue: 158645
> >
> > Fixes: 7a364faef185 ("cryptodev: remove crypto device type
> > enumeration")
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > ---
> >
> > Changes in v3:
> >
> > - Added documentation (removed deprecation notice and added API
> > Change)
> > - Modified title to something more explicit
> >
> > Changes in v2:
> >
> > - Allocate statically the cryptodev driver structure,
> >   instead of using malloc, that can potentially fail.
> >
> >  doc/guides/rel_notes/deprecation.rst        |  6 ------
> >  doc/guides/rel_notes/release_17_11.rst      |  5 +++++
> >  drivers/crypto/aesni_gcm/aesni_gcm_pmd.c    |  5 ++++-
> >  drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c  |  6 +++++-
> >  drivers/crypto/armv8/rte_armv8_pmd.c        |  9 ++++++---
> >  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  5 ++++-
> >  drivers/crypto/kasumi/rte_kasumi_pmd.c      |  5 ++++-
> >  drivers/crypto/null/null_crypto_pmd.c       |  5 ++++-
> >  drivers/crypto/openssl/rte_openssl_pmd.c    |  5 ++++-
> >  drivers/crypto/qat/rte_qat_cryptodev.c      |  7 +++++--
> >  drivers/crypto/scheduler/scheduler_pmd.c    |  5 ++++-
> >  drivers/crypto/snow3g/rte_snow3g_pmd.c      |  5 ++++-
> >  drivers/crypto/zuc/rte_zuc_pmd.c            |  5 ++++-
> >  lib/librte_cryptodev/rte_cryptodev.c        | 18 +++++------------
> >  lib/librte_cryptodev/rte_cryptodev.h        | 20 -------------------
> >  lib/librte_cryptodev/rte_cryptodev_pmd.h    | 30
> > +++++++++++++++++++++++++++++
> >  16 files changed, 88 insertions(+), 53 deletions(-)
> >
> 
> Reviewed-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> 

Applied to dpdk-next-crypto.

Pablo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference
  2017-07-31  2:30 [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference Pablo de Lara
  2017-07-31  9:18 ` [dpdk-dev] [PATCH v2] " Pablo de Lara
@ 2017-07-31 12:32 ` Sergio Gonzalez Monroy
  2017-07-31 15:22   ` Thomas Monjalon
  1 sibling, 1 reply; 15+ messages in thread
From: Sergio Gonzalez Monroy @ 2017-07-31 12:32 UTC (permalink / raw)
  To: Pablo de Lara, declan.doherty; +Cc: dev

On 31/07/2017 03:30, Pablo de Lara wrote:
> When registering a crypto driver, if memory allocation
> fails, application should exit and do not allow
> a NULL pointer dereference.
>
> Coverity issue: 158645
>
> Fixes: 7a364faef185 ("cryptodev: remove crypto device type enumeration")
>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
>   lib/librte_cryptodev/rte_cryptodev.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> index 327d7e8..4492b0d 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -1404,6 +1404,12 @@ rte_cryptodev_allocate_driver(const struct rte_driver *drv)
>   	struct cryptodev_driver *driver;
>   
>   	driver = malloc(sizeof(*driver));
> +
> +	if (driver == NULL)
> +		rte_exit(EXIT_FAILURE,
> +			"Could not allocate memory for crypto driver %u\n",
> +			nb_drivers);
> +
>   	driver->driver = drv;
>   	driver->id = nb_drivers;
>   

Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference
  2017-07-31 12:32 ` [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference Sergio Gonzalez Monroy
@ 2017-07-31 15:22   ` Thomas Monjalon
  2017-08-01  7:09     ` De Lara Guarch, Pablo
  0 siblings, 1 reply; 15+ messages in thread
From: Thomas Monjalon @ 2017-07-31 15:22 UTC (permalink / raw)
  To: Pablo de Lara; +Cc: dev, Sergio Gonzalez Monroy, declan.doherty

31/07/2017 14:32, Sergio Gonzalez Monroy:
> On 31/07/2017 03:30, Pablo de Lara wrote:
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -1404,6 +1404,12 @@ rte_cryptodev_allocate_driver(const struct rte_driver *drv)
> >   	struct cryptodev_driver *driver;
> >   
> >   	driver = malloc(sizeof(*driver));
> > +
> > +	if (driver == NULL)
> > +		rte_exit(EXIT_FAILURE,
> > +			"Could not allocate memory for crypto driver %u\n",
> > +			nb_drivers);
> > +
> >   	driver->driver = drv;
> >   	driver->id = nb_drivers;
> >   
> 
> Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

NACK rte_exit/rte_panic in libraries.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference
  2017-07-31 15:22   ` Thomas Monjalon
@ 2017-08-01  7:09     ` De Lara Guarch, Pablo
  0 siblings, 0 replies; 15+ messages in thread
From: De Lara Guarch, Pablo @ 2017-08-01  7:09 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Gonzalez Monroy, Sergio, Doherty, Declan

Hi Thomas,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, July 31, 2017 4:23 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; Gonzalez Monroy, Sergio
> <sergio.gonzalez.monroy@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference
> 
> 31/07/2017 14:32, Sergio Gonzalez Monroy:
> > On 31/07/2017 03:30, Pablo de Lara wrote:
> > > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > > @@ -1404,6 +1404,12 @@ rte_cryptodev_allocate_driver(const struct
> rte_driver *drv)
> > >   	struct cryptodev_driver *driver;
> > >
> > >   	driver = malloc(sizeof(*driver));
> > > +
> > > +	if (driver == NULL)
> > > +		rte_exit(EXIT_FAILURE,
> > > +			"Could not allocate memory for crypto driver %u\n",
> > > +			nb_drivers);
> > > +
> > >   	driver->driver = drv;
> > >   	driver->id = nb_drivers;
> > >
> >
> > Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> 
> NACK rte_exit/rte_panic in libraries.

I have sent a v2 that allocates statically the structure,
instead of calling malloc, so no rte_exit is required.

Thanks,
Pablo

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-09-06 10:27 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-31  2:30 [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference Pablo de Lara
2017-07-31  9:18 ` [dpdk-dev] [PATCH v2] " Pablo de Lara
2017-07-31 19:33   ` Thomas Monjalon
2017-08-01  7:10     ` De Lara Guarch, Pablo
2017-08-01  8:13     ` Sergio Gonzalez Monroy
2017-08-01  9:35       ` Thomas Monjalon
2017-08-01 10:17         ` Sergio Gonzalez Monroy
2017-08-01 10:48           ` De Lara Guarch, Pablo
2017-08-01 12:36             ` Thomas Monjalon
2017-08-16  2:41   ` [dpdk-dev] [PATCH v3] cryptodev: allocate driver structure statically Pablo de Lara
2017-09-04 10:38     ` Rybalchenko, Kirill
2017-09-06 10:27       ` De Lara Guarch, Pablo
2017-07-31 12:32 ` [dpdk-dev] [PATCH] cryptodev: fix NULL pointer dereference Sergio Gonzalez Monroy
2017-07-31 15:22   ` Thomas Monjalon
2017-08-01  7:09     ` De Lara Guarch, Pablo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).