Re: [PATCH] crypto: qat - fix active_devs leak on crypto alg registration failure

[Thomas Huth <thuth@redhat.com>]

On 02/09/2026 12.12, Ahsan Atta wrote:
> adf_dev_start() registered both alg sets in a single condition.
> If qat_algs_register() succeeds but qat_asym_algs_register() fails,
> ADF_STATUS_CRYPTO_ALGS_REGISTERED is left clear, so adf_dev_stop() skips
> qat_algs_unregister(): the skciphers and aeads stay registered with
> active_devs stuck at 1, pinning the module.
> 
> Both helpers leak active_devs internally as well - they increment it and
> return without decrementing when a crypto_register_*() call fails, and
> the asym path also leaves the already-registered akcipher behind.
> 
> Split the registration in adf_dev_start() so that an asym failure
> unregisters qat_algs, and unwind active_devs (and the akcipher) on the
> error paths of both register helpers.
> 
> Fixes: 9b2f33a1bfcd ("crypto: qat - fix unregistration of crypto algorithms")
> Signed-off-by: Ahsan Atta <ahsan.atta@intel.com>
> Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> ---
>   .../crypto/intel/qat/qat_common/adf_init.c    | 22 +++++++++++-----
>   .../crypto/intel/qat/qat_common/qat_algs.c    | 15 ++++++-----
>   .../intel/qat/qat_common/qat_asym_algs.c      | 26 ++++++++++++++-----
>   3 files changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_init.c b/drivers/crypto/intel/qat/qat_common/adf_init.c
> index 3e39c53814de..558467dea1a5 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_init.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_init.c
> @@ -260,13 +260,21 @@ static int adf_dev_start(struct adf_accel_dev *accel_dev)
>   	clear_bit(ADF_STATUS_STARTING, &accel_dev->status);
>   	set_bit(ADF_STATUS_STARTED, &accel_dev->status);
>   
> -	if (!list_empty(&accel_dev->crypto_list) &&
> -	    (qat_algs_register() || qat_asym_algs_register())) {
> -		dev_err(&GET_DEV(accel_dev),
> -			"Failed to register crypto algs\n");
> -		set_bit(ADF_STATUS_STARTING, &accel_dev->status);
> -		clear_bit(ADF_STATUS_STARTED, &accel_dev->status);
> -		return -EFAULT;
> +	if (!list_empty(&accel_dev->crypto_list)) {
> +		if (qat_algs_register()) {
> +			dev_err(&GET_DEV(accel_dev), "Failed to register crypto algs\n");
> +			set_bit(ADF_STATUS_STARTING, &accel_dev->status);
> +			clear_bit(ADF_STATUS_STARTED, &accel_dev->status);
> +			return -EFAULT;
> +		}
> +
> +		if (qat_asym_algs_register()) {
> +			dev_err(&GET_DEV(accel_dev), "Failed to register crypto asym algs\n");
> +			qat_algs_unregister();
> +			set_bit(ADF_STATUS_STARTING, &accel_dev->status);
> +			clear_bit(ADF_STATUS_STARTED, &accel_dev->status);
> +			return -EFAULT;

EFAULT is the error code that should be used if accessing memory failed, 
it's a very bad choice for an error code in this case here.
So while you're at it, could you maybe change this to return a more sane 
value here? I think the best option is likely to pass along the return value 
from qat_algs_register / qat_asym_algs_register.

  Thanks,
   Thomas

> +		}
>   	}
>   	set_bit(ADF_STATUS_CRYPTO_ALGS_REGISTERED, &accel_dev->status);...