* [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal
@ 2020-06-09 21:24 Ayush Sawal
2020-06-09 21:24 ` [PATCH net-next 1/2] Crypto/chcr: Calculate src and dst sg lengths separately for dma map Ayush Sawal
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ayush Sawal @ 2020-06-09 21:24 UTC (permalink / raw)
To: davem, herbert; +Cc: netdev, manojmalviya, Ayush Sawal
Patch 1: This fixes the kernel panic which occurs due to the accessing
of a zero length sg.
Patch 2: Avoiding unregistering the algorithm if cra_refcnt is not 1.
Ayush Sawal (2):
Crypto/chcr: Calculate src and dst sg lengths separately for dma map
Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
drivers/crypto/chelsio/chcr_algo.c | 81 ++++++++++++++++++++++--------
1 file changed, 59 insertions(+), 22 deletions(-)
--
2.26.0.rc1.11.g30e9940
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 1/2] Crypto/chcr: Calculate src and dst sg lengths separately for dma map
2020-06-09 21:24 [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal Ayush Sawal
@ 2020-06-09 21:24 ` Ayush Sawal
2020-06-09 21:24 ` [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms Ayush Sawal
2020-06-11 0:05 ` [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal David Miller
2 siblings, 0 replies; 8+ messages in thread
From: Ayush Sawal @ 2020-06-09 21:24 UTC (permalink / raw)
To: davem, herbert; +Cc: netdev, manojmalviya, Ayush Sawal
This patch calculates src and dst sg lengths separately for
dma mapping in case of aead operation.
This fixes a panic which occurs due to the accessing of a zero
length sg.
Panic:
[ 138.173225] kernel BUG at drivers/iommu/intel-iommu.c:1184!
Signed-off-by: Ayush Sawal <ayush.sawal@chelsio.com>
---
drivers/crypto/chelsio/chcr_algo.c | 63 +++++++++++++++++++++---------
1 file changed, 45 insertions(+), 18 deletions(-)
diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
index f26a7a15551a..f8b55137cf7d 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -2590,11 +2590,22 @@ int chcr_aead_dma_map(struct device *dev,
struct chcr_aead_reqctx *reqctx = aead_request_ctx(req);
struct crypto_aead *tfm = crypto_aead_reqtfm(req);
unsigned int authsize = crypto_aead_authsize(tfm);
- int dst_size;
+ int src_len, dst_len;
- dst_size = req->assoclen + req->cryptlen + (op_type ?
- 0 : authsize);
- if (!req->cryptlen || !dst_size)
+ /* calculate and handle src and dst sg length separately
+ * for inplace and out-of place operations
+ */
+ if (req->src == req->dst) {
+ src_len = req->assoclen + req->cryptlen + (op_type ?
+ 0 : authsize);
+ dst_len = src_len;
+ } else {
+ src_len = req->assoclen + req->cryptlen;
+ dst_len = req->assoclen + req->cryptlen + (op_type ?
+ -authsize : authsize);
+ }
+
+ if (!req->cryptlen || !src_len || !dst_len)
return 0;
reqctx->iv_dma = dma_map_single(dev, reqctx->iv, (IV + reqctx->b0_len),
DMA_BIDIRECTIONAL);
@@ -2606,20 +2617,23 @@ int chcr_aead_dma_map(struct device *dev,
reqctx->b0_dma = 0;
if (req->src == req->dst) {
error = dma_map_sg(dev, req->src,
- sg_nents_for_len(req->src, dst_size),
+ sg_nents_for_len(req->src, src_len),
DMA_BIDIRECTIONAL);
if (!error)
goto err;
} else {
- error = dma_map_sg(dev, req->src, sg_nents(req->src),
+ error = dma_map_sg(dev, req->src,
+ sg_nents_for_len(req->src, src_len),
DMA_TO_DEVICE);
if (!error)
goto err;
- error = dma_map_sg(dev, req->dst, sg_nents(req->dst),
+ error = dma_map_sg(dev, req->dst,
+ sg_nents_for_len(req->dst, dst_len),
DMA_FROM_DEVICE);
if (!error) {
- dma_unmap_sg(dev, req->src, sg_nents(req->src),
- DMA_TO_DEVICE);
+ dma_unmap_sg(dev, req->src,
+ sg_nents_for_len(req->src, src_len),
+ DMA_TO_DEVICE);
goto err;
}
}
@@ -2637,24 +2651,37 @@ void chcr_aead_dma_unmap(struct device *dev,
struct chcr_aead_reqctx *reqctx = aead_request_ctx(req);
struct crypto_aead *tfm = crypto_aead_reqtfm(req);
unsigned int authsize = crypto_aead_authsize(tfm);
- int dst_size;
+ int src_len, dst_len;
- dst_size = req->assoclen + req->cryptlen + (op_type ?
- 0 : authsize);
- if (!req->cryptlen || !dst_size)
+ /* calculate and handle src and dst sg length separately
+ * for inplace and out-of place operations
+ */
+ if (req->src == req->dst) {
+ src_len = req->assoclen + req->cryptlen + (op_type ?
+ 0 : authsize);
+ dst_len = src_len;
+ } else {
+ src_len = req->assoclen + req->cryptlen;
+ dst_len = req->assoclen + req->cryptlen + (op_type ?
+ -authsize : authsize);
+ }
+
+ if (!req->cryptlen || !src_len || !dst_len)
return;
dma_unmap_single(dev, reqctx->iv_dma, (IV + reqctx->b0_len),
DMA_BIDIRECTIONAL);
if (req->src == req->dst) {
dma_unmap_sg(dev, req->src,
- sg_nents_for_len(req->src, dst_size),
+ sg_nents_for_len(req->src, src_len),
DMA_BIDIRECTIONAL);
} else {
- dma_unmap_sg(dev, req->src, sg_nents(req->src),
- DMA_TO_DEVICE);
- dma_unmap_sg(dev, req->dst, sg_nents(req->dst),
- DMA_FROM_DEVICE);
+ dma_unmap_sg(dev, req->src,
+ sg_nents_for_len(req->src, src_len),
+ DMA_TO_DEVICE);
+ dma_unmap_sg(dev, req->dst,
+ sg_nents_for_len(req->dst, dst_len),
+ DMA_FROM_DEVICE);
}
}
--
2.26.0.rc1.11.g30e9940
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
2020-06-09 21:24 [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal Ayush Sawal
2020-06-09 21:24 ` [PATCH net-next 1/2] Crypto/chcr: Calculate src and dst sg lengths separately for dma map Ayush Sawal
@ 2020-06-09 21:24 ` Ayush Sawal
2020-06-11 3:48 ` Herbert Xu
2020-06-11 0:05 ` [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal David Miller
2 siblings, 1 reply; 8+ messages in thread
From: Ayush Sawal @ 2020-06-09 21:24 UTC (permalink / raw)
To: davem, herbert; +Cc: netdev, manojmalviya, Ayush Sawal
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;
+ }
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;
}
--
2.26.0.rc1.11.g30e9940
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal
2020-06-09 21:24 [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal Ayush Sawal
2020-06-09 21:24 ` [PATCH net-next 1/2] Crypto/chcr: Calculate src and dst sg lengths separately for dma map Ayush Sawal
2020-06-09 21:24 ` [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms Ayush Sawal
@ 2020-06-11 0:05 ` David Miller
2020-06-11 3:49 ` Herbert Xu
2 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2020-06-11 0:05 UTC (permalink / raw)
To: ayush.sawal; +Cc: herbert, netdev, manojmalviya
From: Ayush Sawal <ayush.sawal@chelsio.com>
Date: Wed, 10 Jun 2020 02:54:30 +0530
> Patch 1: This fixes the kernel panic which occurs due to the accessing
> of a zero length sg.
>
> Patch 2: Avoiding unregistering the algorithm if cra_refcnt is not 1.
>
> Ayush Sawal (2):
> Crypto/chcr: Calculate src and dst sg lengths separately for dma map
> Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
Series applied, thanks.
Maybe we can start handling these changes via the crypto tree at some
point?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
2020-06-09 21:24 ` [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms Ayush Sawal
@ 2020-06-11 3:48 ` Herbert Xu
2020-06-11 6:08 ` Ayush Sawal
0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2020-06-11 3:48 UTC (permalink / raw)
To: Ayush Sawal; +Cc: davem, netdev, manojmalviya
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,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal
2020-06-11 0:05 ` [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal David Miller
@ 2020-06-11 3:49 ` Herbert Xu
0 siblings, 0 replies; 8+ messages in thread
From: Herbert Xu @ 2020-06-11 3:49 UTC (permalink / raw)
To: David Miller; +Cc: ayush.sawal, netdev, manojmalviya
On Wed, Jun 10, 2020 at 05:05:43PM -0700, David Miller wrote:
>
> Maybe we can start handling these changes via the crypto tree at some
> point?
Yes that's good point Dave. How about we push changes for chcr_algo
via the crypto tree and the rest via netdev?
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
2020-06-11 3:48 ` Herbert Xu
@ 2020-06-11 6:08 ` Ayush Sawal
2020-06-11 6:42 ` Herbert Xu
0 siblings, 1 reply; 8+ messages in thread
From: Ayush Sawal @ 2020-06-11 6:08 UTC (permalink / raw)
To: Herbert Xu; +Cc: ayush.sawal, davem, netdev, manojmalviya
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,
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms
2020-06-11 6:08 ` Ayush Sawal
@ 2020-06-11 6:42 ` Herbert Xu
0 siblings, 0 replies; 8+ messages in thread
From: Herbert Xu @ 2020-06-11 6:42 UTC (permalink / raw)
To: Ayush Sawal; +Cc: ayush.sawal, davem, netdev, manojmalviya
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,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-11 6:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-09 21:24 [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal Ayush Sawal
2020-06-09 21:24 ` [PATCH net-next 1/2] Crypto/chcr: Calculate src and dst sg lengths separately for dma map Ayush Sawal
2020-06-09 21:24 ` [PATCH net-next 2/2] Crypto/chcr: Checking cra_refcnt before unregistering the algorithms Ayush Sawal
2020-06-11 3:48 ` Herbert Xu
2020-06-11 6:08 ` Ayush Sawal
2020-06-11 6:42 ` Herbert Xu
2020-06-11 0:05 ` [PATCH net-next 0/2] Fixing issues in dma mapping and driver removal David Miller
2020-06-11 3:49 ` Herbert Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.