[dpdk-dev] [PATCH] cryptodev: fix session init return value

[Trahe, Fiona]

Hi Pablo,

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Monday, July 24, 2017 9:54 AM
> To: zbigniew.bodek at caviumnetworks.com; jerin.jacob at caviumnetworks.com; akhil.goyal at nxp.com;
> hemant.agrawal at nxp.com; Trahe, Fiona <fiona.trahe at intel.com>; Jain, Deepak K
> <deepak.k.jain at intel.com>; Griffin, John <john.griffin at intel.com>
> Cc: dev at dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>
> Subject: [PATCH] cryptodev: fix session init return value
> 
> When calling rte_cryptodev_sym_session_init(),
> if there was an error, it returned -1, regardless
> the error.
> Instead, it should return the specific error code, which can
> be valuable for the application for error handling.
> 
> Fixes: b3bbd9e5f265 ("cryptodev: support device independent sessions")
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch at intel.com>
> ---


Trimmed to just the relevant QAT sections


> diff --git a/drivers/crypto/qat/qat_crypto.c b/drivers/crypto/qat/qat_crypto.c
> index d92de35..e115540 100644
> --- a/drivers/crypto/qat/qat_crypto.c
> +++ b/drivers/crypto/qat/qat_crypto.c
> @@ -171,16 +171,19 @@ bpi_cipher_decrypt(uint8_t *src, uint8_t *dst,
>  /** Creates a context in either AES or DES in ECB mode
>   *  Depends on openssl libcrypto
>   */
> -static void *
> +static int
>  bpi_cipher_ctx_init(enum rte_crypto_cipher_algorithm cryptodev_algo,
>  		enum rte_crypto_cipher_operation direction __rte_unused,
> -					uint8_t *key)
> +		uint8_t *key, EVP_CIPHER_CTX **ctx)

My preference is to use a void ** ctx here. So all EVP refs are encapsulated within these fns
and if we ever wanted to use a lib other than openssl the changes would be confined to the
internals of these functions.



> @@ -499,44 +525,52 @@ qat_crypto_set_session_parameters(struct rte_cryptodev *dev,
>  	qat_cmd_id = qat_get_cmd_id(xform);
>  	if (qat_cmd_id < 0 || qat_cmd_id >= ICP_QAT_FW_LA_CMD_DELIMITER) {
>  		PMD_DRV_LOG(ERR, "Unsupported xform chain requested");
> -		return -1;
> +		return -ENOTSUP;
>  	}
>  	session->qat_cmd = (enum icp_qat_fw_la_cmd_id)qat_cmd_id;
>  	switch (session->qat_cmd) {
>  	case ICP_QAT_FW_LA_CMD_CIPHER:
> -		if (qat_crypto_sym_configure_session_cipher(dev, xform, session) < 0)
> -			return -1;
> +		ret = qat_crypto_sym_configure_session_cipher(dev, xform, session);
> +		if (ret < 0)
> +			return ret;
>  		break;
>  	case ICP_QAT_FW_LA_CMD_AUTH:
> -		if (qat_crypto_sym_configure_session_auth(dev, xform, session) < 0)
> -			return -1;
> +		ret = qat_crypto_sym_configure_session_auth(dev, xform, session);
> +		if (ret < 0)
> +			return ret;
>  		break;
>  	case ICP_QAT_FW_LA_CMD_CIPHER_HASH:
>  		if (xform->type == RTE_CRYPTO_SYM_XFORM_AEAD) {
> -			if (qat_crypto_sym_configure_session_aead(xform,
> -					session) < 0)
> -				return -1;
> +			ret = qat_crypto_sym_configure_session_aead(xform,
> +					session);
> +			if (ret < 0)
> +				return ret;
>  		} else {
> -			if (qat_crypto_sym_configure_session_cipher(dev,
> -					xform, session) < 0)
> -				return -1;
> -			if (qat_crypto_sym_configure_session_auth(dev,
> -					xform, session) < 0)
> -				return -1;
> +			ret = qat_crypto_sym_configure_session_cipher(dev,
> +					xform, session);
> +			if (ret < 0)
> +				return ret;
> +			ret = qat_crypto_sym_configure_session_auth(dev,
> +					xform, session);
> +			if (ret < 0)
> +				return ret;

In this case it is also necessary to undo what was done during the previous fn
qat_crypto_sym_configure_session_cipher(), i.e. add
                if (session->bpi_ctx) {
		bpi_cipher_ctx_free(session->bpi_ctx);
		session->bpi_ctx = NULL;
	}
OR encapsulate this in a new fn:
qat_crypto_sym_clear_session_cipher()

Note, this is a pre-existing bug, just highlighted by this change.