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

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

[Ahsan Atta]

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;
+		}
 	}
 	set_bit(ADF_STATUS_CRYPTO_ALGS_REGISTERED, &accel_dev->status);
 
diff --git a/drivers/crypto/intel/qat/qat_common/qat_algs.c b/drivers/crypto/intel/qat/qat_common/qat_algs.c
index 91663805d9e6..f954e7c0e4d0 100644
--- a/drivers/crypto/intel/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/intel/qat/qat_common/qat_algs.c
@@ -1320,19 +1320,22 @@ int qat_algs_register(void)
 	ret = crypto_register_skciphers(qat_skciphers,
 					ARRAY_SIZE(qat_skciphers));
 	if (ret)
-		goto unlock;
+		goto err_dec;
 
 	ret = crypto_register_aeads(qat_aeads, ARRAY_SIZE(qat_aeads));
 	if (ret)
-		goto unreg_algs;
+		goto err_unreg_skciphers;
 
-unlock:
 	mutex_unlock(&algs_lock);
-	return ret;
+	return 0;
 
-unreg_algs:
+err_unreg_skciphers:
 	crypto_unregister_skciphers(qat_skciphers, ARRAY_SIZE(qat_skciphers));
-	goto unlock;
+err_dec:
+	active_devs--;
+unlock:
+	mutex_unlock(&algs_lock);
+	return ret;
 }
 
 void qat_algs_unregister(void)
diff --git a/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c b/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c
index 1049c583f84f..77d59e977472 100644
--- a/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c
+++ b/drivers/crypto/intel/qat/qat_common/qat_asym_algs.c
@@ -1340,13 +1340,25 @@ int qat_asym_algs_register(void)
 	int ret = 0;
 
 	mutex_lock(&algs_lock);
-	if (++active_devs == 1) {
-		rsa.base.cra_flags = 0;
-		ret = crypto_register_akcipher(&rsa);
-		if (ret)
-			goto unlock;
-		ret = crypto_register_kpp(&dh);
-	}
+	if (++active_devs != 1)
+		goto unlock;
+
+	rsa.base.cra_flags = 0;
+	ret = crypto_register_akcipher(&rsa);
+	if (ret)
+		goto err_dec;
+
+	ret = crypto_register_kpp(&dh);
+	if (ret)
+		goto err_unreg_akcipher;
+
+	mutex_unlock(&algs_lock);
+	return 0;
+
+err_unreg_akcipher:
+	crypto_unregister_akcipher(&rsa);
+err_dec:
+	active_devs--;
 unlock:
 	mutex_unlock(&algs_lock);
 	return ret;
-- 
2.50.1

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

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

[Thomas Huth]

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);...

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

[Ahsan Atta]

On Mon, Sep 07, 2026 at 03:51:19PM +0200, Thomas Huth wrote:
> 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.

Hi,
Thanks for your feedback. I'll send a v2.

Regards,
Ahsan
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.