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

[Ahsan Atta <ahsan.atta@intel.com>]

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.