| Message ID | 20200609212432.2467-3-ayush.sawal@chelsio.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | Fixing issues in dma mapping and driver removal | expand |
On Wed, Jun 10, 2020 at 02:54:32AM +0530, Ayush Sawal wrote: > This patch puts a check for algorithm unregister, to avoid removal of > driver if the algorithm is under use. > > Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com> > --- > drivers/crypto/chelsio/chcr_algo.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c > index f8b55137cf7d..4c2553672b6f 100644 > --- a/drivers/crypto/chelsio/chcr_algo.c > +++ b/drivers/crypto/chelsio/chcr_algo.c > @@ -4391,22 +4391,32 @@ static int chcr_unregister_alg(void) > for (i = 0; i < ARRAY_SIZE(driver_algs); i++) { > switch (driver_algs[i].type & CRYPTO_ALG_TYPE_MASK) { > case CRYPTO_ALG_TYPE_SKCIPHER: > - if (driver_algs[i].is_registered) > + if (driver_algs[i].is_registered && refcount_read( > + &driver_algs[i].alg.skcipher.base.cra_refcnt) > + == 1) { > crypto_unregister_skcipher( > &driver_algs[i].alg.skcipher); > + driver_algs[i].is_registered = 0; > + } This is wrong. cra_refcnt must not be used directly by drivers. Normally driver unregister is stopped by the module reference count. This is not the case for your driver because of the existence of a path of unregistration that is not tied to module removal. To support that properly, we need to add code to the Crypto API to handle this, as opposed to adding hacks to the driver. Thanks,
Hi Herbert, On 6/11/2020 9:18 AM, Herbert Xu wrote: > On Wed, Jun 10, 2020 at 02:54:32AM +0530, Ayush Sawal wrote: >> This patch puts a check for algorithm unregister, to avoid removal of >> driver if the algorithm is under use. >> >> Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com> >> --- >> drivers/crypto/chelsio/chcr_algo.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c >> index f8b55137cf7d..4c2553672b6f 100644 >> --- a/drivers/crypto/chelsio/chcr_algo.c >> +++ b/drivers/crypto/chelsio/chcr_algo.c >> @@ -4391,22 +4391,32 @@ static int chcr_unregister_alg(void) >> for (i = 0; i < ARRAY_SIZE(driver_algs); i++) { >> switch (driver_algs[i].type & CRYPTO_ALG_TYPE_MASK) { >> case CRYPTO_ALG_TYPE_SKCIPHER: >> - if (driver_algs[i].is_registered) >> + if (driver_algs[i].is_registered && refcount_read( >> + &driver_algs[i].alg.skcipher.base.cra_refcnt) >> + == 1) { >> crypto_unregister_skcipher( >> &driver_algs[i].alg.skcipher); >> + driver_algs[i].is_registered = 0; >> + } > This is wrong. cra_refcnt must not be used directly by drivers. > > Normally driver unregister is stopped by the module reference > count. This is not the case for your driver because of the existence > of a path of unregistration that is not tied to module removal. > > To support that properly, we need to add code to the Crypto API > to handle this, as opposed to adding hacks to the driver. Sorry for this hack, Our problem was when ipsec is under use and device is dettached, then chcr_unregister_alg() is called which unregisters the algorithms, but as ipsec is established the cra_refcnt is not 1 and it gives a kernel bug. So i put a check of cra_refcnt there, taking the reference of a crypto driver "marvell/octeontx/otx_cptvf_algs.c" is_any_alg_used(void) function where cra_refcnt is checked before unregistering the algorithms. Thanks, Ayush > > Thanks,
On Thu, Jun 11, 2020 at 11:38:39AM +0530, Ayush Sawal wrote: > > Sorry for this hack, Our problem was when ipsec is under use and device is > dettached, then chcr_unregister_alg() > is called which unregisters the algorithms, but as ipsec is established the > cra_refcnt is not 1 and it gives a kernel bug. > So i put a check of cra_refcnt there, taking the reference of a crypto > driver "marvell/octeontx/otx_cptvf_algs.c" > is_any_alg_used(void) function where cra_refcnt is checked before > unregistering the algorithms. I understand. The question is how do you want to deal with the exception. IOW do you want to leave the algorithm still registered? If you can keep the algorithm registered you might as well never unregister it in the first place. If it has to go then this code path must wait for the users to disappear first. Cheers,
diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c index f8b55137cf7d..4c2553672b6f 100644 --- a/drivers/crypto/chelsio/chcr_algo.c +++ b/drivers/crypto/chelsio/chcr_algo.c @@ -4391,22 +4391,32 @@ static int chcr_unregister_alg(void) for (i = 0; i < ARRAY_SIZE(driver_algs); i++) { switch (driver_algs[i].type & CRYPTO_ALG_TYPE_MASK) { case CRYPTO_ALG_TYPE_SKCIPHER: - if (driver_algs[i].is_registered) + if (driver_algs[i].is_registered && refcount_read( + &driver_algs[i].alg.skcipher.base.cra_refcnt) + == 1) { crypto_unregister_skcipher( &driver_algs[i].alg.skcipher); + driver_algs[i].is_registered = 0; + } break; case CRYPTO_ALG_TYPE_AEAD: - if (driver_algs[i].is_registered) + if (driver_algs[i].is_registered && refcount_read( + &driver_algs[i].alg.aead.base.cra_refcnt) == 1) { crypto_unregister_aead( &driver_algs[i].alg.aead); + driver_algs[i].is_registered = 0; + } break; case CRYPTO_ALG_TYPE_AHASH: - if (driver_algs[i].is_registered) + if (driver_algs[i].is_registered && refcount_read( + &driver_algs[i].alg.hash.halg.base.cra_refcnt) + == 1) { crypto_unregister_ahash( &driver_algs[i].alg.hash); + driver_algs[i].is_registered = 0; + } break; } - driver_algs[i].is_registered = 0; } return 0; }
This patch puts a check for algorithm unregister, to avoid removal of driver if the algorithm is under use. Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com> --- drivers/crypto/chelsio/chcr_algo.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)