Test environment:
Linux kernel version: 5.10.y
Architecture: ARM Cortex-A
Processor: NXP Layerscape LS1028
Crash in reboot tests:
Reproducibility: 1%
If a job ring is still allocated, Once caam_jr_remove() returned,
jrpriv will be freed and the registers will get unmapped.Then
caam_jr_interrupt will get error irqstate value.
So such a job ring will probably crash.Crash info is below:
--------------------------------------
RBS Sys: Restart ordered by epghd(0x1)
RBS Sys: RESTARTING
caam_jr 8030000.jr: Device is busy
caam_jr 8020000.jr: Device is busy
caam_jr 8010000.jr: Device is busy
arm-smmu 5000000.iommu: disabling translation
caam_jr 8010000.jr: job ring error: irqstate: 00000103
------------[ cut here ]------------
kernel BUG at drivers/crypto/caam/jr.c:288!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
Hardware name: freescale ls1028a/ls1028a, BIOS 2019.10+fsl+g3d542a3d22
pstate: 60000085 (nZCv daIf -PAN -UAO -TCO BTYPE=--)
pc : caam_jr_interrupt+0x128/0x130
lr : caam_jr_interrupt+0x128/0x130
sp : ffff80001144be50
x29: ffff80001144be50 x28: ffff800010f61008
x27: ffff800011228000 x26: ffff800010f61008
x25: ffff000027904800 x24: 0000000000000072
x23: ffff8000113ba140 x22: 0000000000000001
x21: ffff800011433000 x20: ffff000027904e80
x19: 0000000000000103 x18: 0000000000000030
x17: 0000000000000000 x16: 0000000000000000
x15: ffffffffffffffff x14: ffff8000113ebcb8
x13: 0000000000000008 x12: fffffffffffcac8f
x11: ffff00000038bb00 x10: ffff8000112a1e90
x9 : ffff8000100a99c0 x8 : ffff800011249e90
x7 : ffff8000112a1e90 x6 : 0000000000000000
x5 : 0000000000000000 x4 : 0000000000000000
x3 : 0000000000000000 x2 : 0000000000000000
x1 : 0000000000000000 x0 : ffff0000279ac600
Call trace:
caam_jr_interrupt+0x128/0x130
__handle_irq_event_percpu+0x84/0x2b0
handle_irq_event+0x6c/0xfc
handle_fasteoi_irq+0xc8/0x230
__handle_domain_irq+0xb8/0x130
gic_handle_irq+0x90/0x158
el1_irq+0xcc/0x180
_raw_spin_lock_irq+0x0/0x90
caam_rng_read_one.constprop.0+0x248/0x370
caam_read+0x8c/0xb0
hwrng_fillfn+0xfc/0x1cc
kthread+0x14c/0x160
ret_from_fork+0x10/0x30
Code: 2a1303e2 d00029a1 910ee021 940b2b1d (d4210000)
---[ end trace f04d90f3ad0da5f4 ]---
Kernel panic - not syncing: Oops - BUG: Fatal exception in interrupt
Kernel Offset: disabled
CPU features: 0x28040022,21002008
Memory Limit: none
--------------------------------------
Disabling interrupts is to ensure that the device removal
operation is not interrupted.
Signed-off-by: Kun Song <[email protected]>
Reviewed-by: Hen Guo <[email protected]>
---
drivers/crypto/caam/jr.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 6f669966ba2c..d191e8caa1ad 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -135,6 +135,10 @@ static int caam_jr_remove(struct platform_device *pdev)
jrdev = &pdev->dev;
jrpriv = dev_get_drvdata(jrdev);
+ /* Disabling interrupts is ensure that the device removal operation
+ * is not interrupted by interrupts.
+ */
+ devm_free_irq(jrdev, jrpriv->irq, jrdev);
if (jrpriv->hwrng)
caam_rng_exit(jrdev->parent);
--
2.26.1
Hi Kun
Have you seen this issue in later kernel versions > 5.10 ?
Regards
Gaurav Jain
> -----Original Message-----
> From: Kun Song <[email protected]>
> Sent: Thursday, December 21, 2023 3:02 PM
> To: Horia Geanta <[email protected]>; Aymen Sghaier
> <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: [PATCH v5.10.y] crypto: caam/jr - Fix possible caam_jr crash
>
> Test environment:
> Linux kernel version: 5.10.y
> Architecture: ARM Cortex-A
> Processor: NXP Layerscape LS1028
>
> Crash in reboot tests:
> Reproducibility: 1%
>
> If a job ring is still allocated, Once caam_jr_remove() returned, jrpriv will be
> freed and the registers will get unmapped.Then caam_jr_interrupt will get error
> irqstate value.
> So such a job ring will probably crash.Crash info is below:
> --------------------------------------
> RBS Sys: Restart ordered by epghd(0x1)
> RBS Sys: RESTARTING
> caam_jr 8030000.jr: Device is busy
> caam_jr 8020000.jr: Device is busy
> caam_jr 8010000.jr: Device is busy
> arm-smmu 5000000.iommu: disabling translation caam_jr 8010000.jr: job ring
> error: irqstate: 00000103 ------------[ cut here ]------------ kernel BUG at
> drivers/crypto/caam/jr.c:288!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP Hardware name: freescale
> ls1028a/ls1028a, BIOS 2019.10+fsl+g3d542a3d22
> pstate: 60000085 (nZCv daIf -PAN -UAO -TCO BTYPE=--) pc :
> caam_jr_interrupt+0x128/0x130 lr : caam_jr_interrupt+0x128/0x130 sp :
> ffff80001144be50
> x29: ffff80001144be50 x28: ffff800010f61008
> x27: ffff800011228000 x26: ffff800010f61008
> x25: ffff000027904800 x24: 0000000000000072
> x23: ffff8000113ba140 x22: 0000000000000001
> x21: ffff800011433000 x20: ffff000027904e80
> x19: 0000000000000103 x18: 0000000000000030
> x17: 0000000000000000 x16: 0000000000000000
> x15: ffffffffffffffff x14: ffff8000113ebcb8
> x13: 0000000000000008 x12: fffffffffffcac8f
> x11: ffff00000038bb00 x10: ffff8000112a1e90
> x9 : ffff8000100a99c0 x8 : ffff800011249e90
> x7 : ffff8000112a1e90 x6 : 0000000000000000
> x5 : 0000000000000000 x4 : 0000000000000000
> x3 : 0000000000000000 x2 : 0000000000000000
> x1 : 0000000000000000 x0 : ffff0000279ac600 Call trace:
> caam_jr_interrupt+0x128/0x130
> __handle_irq_event_percpu+0x84/0x2b0
> handle_irq_event+0x6c/0xfc
> handle_fasteoi_irq+0xc8/0x230
> __handle_domain_irq+0xb8/0x130
> gic_handle_irq+0x90/0x158
> el1_irq+0xcc/0x180
> _raw_spin_lock_irq+0x0/0x90
> caam_rng_read_one.constprop.0+0x248/0x370
> caam_read+0x8c/0xb0
> hwrng_fillfn+0xfc/0x1cc
> kthread+0x14c/0x160
> ret_from_fork+0x10/0x30
> Code: 2a1303e2 d00029a1 910ee021 940b2b1d (d4210000) ---[ end trace
> f04d90f3ad0da5f4 ]--- Kernel panic - not syncing: Oops - BUG: Fatal exception in
> interrupt Kernel Offset: disabled CPU features: 0x28040022,21002008 Memory
> Limit: none
> --------------------------------------
>
> Disabling interrupts is to ensure that the device removal operation is not
> interrupted.
>
> Signed-off-by: Kun Song <[email protected]>
> Reviewed-by: Hen Guo <[email protected]>
> ---
> drivers/crypto/caam/jr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index
> 6f669966ba2c..d191e8caa1ad 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -135,6 +135,10 @@ static int caam_jr_remove(struct platform_device
> *pdev)
> jrdev = &pdev->dev;
> jrpriv = dev_get_drvdata(jrdev);
>
> + /* Disabling interrupts is ensure that the device removal operation
> + * is not interrupted by interrupts.
> + */
> + devm_free_irq(jrdev, jrpriv->irq, jrdev);
> if (jrpriv->hwrng)
> caam_rng_exit(jrdev->parent);
>
> --
> 2.26.1
>
>Hi Kun
>
>Have you seen this issue in later kernel versions > 5.10 ?
>
>Regards
>Gaurav Jain
Hi, Gaurav
This race issue can only be reproduced in the customer's complex environment.
We were unable to reproduce it in our test enviroment, so we are not sure if the same issue can be reproduced on the latest kernel.
But by reviewing the code, it should be present in later versions.
Thanks!
BR/SK
Hello Gaurav,
I hope you receive this email. I'm following up on a patch I submitted a few weeks ago. There doesn't seem to be any response yet and I want to make sure it gets pushed forward.
I know you're busy and thank you for taking the time to focus on this.If you have any concerns or feedback please let me know and I'll be happy to address it.
Best regards,
SK
Hello SK
I am submitting and replying patches using [email protected]
In Later kernel versions we have provided fixes related to job ring flush and there are other changes as well.
It would be better to add these changes on top of 5.10 tree as we also run multiple tests and not observed this issue.
Regards
Gaurav Jain
> -----Original Message-----
> From: Kun Song <[email protected]>
> Sent: Wednesday, January 10, 2024 7:09 AM
> To: Gaurav Jain (OSS) <[email protected]>
> Cc: [email protected]; Varun Sethi <[email protected]>; Aymen Sghaier
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Horia Geanta
> <[email protected]>; [email protected]; Meenakshi Aggarwal
> <[email protected]>
> Subject: [REMINDER] Re: [PATCH v5.10.y] crypto: caam/jr - Fix possible caam_jr
> crash
>
> Hello Gaurav,
>
> I hope you receive this email. I'm following up on a patch I submitted a few
> weeks ago. There doesn't seem to be any response yet and I want to make sure
> it gets pushed forward.
>
> I know you're busy and thank you for taking the time to focus on this.If you
> have any concerns or feedback please let me know and I'll be happy to address it.
>
> Best regards,
> SK
Hello SK
I am submitting and replying patches using [email protected]
In Later kernel versions we have provided fixes related to job ring flush and there are other changes as well.
It would be better to add these changes on top of 5.10 tree as we also run multiple tests and not observed this issue.
Regards
Gaurav Jain
> -----Original Message-----
> From: Kun Song <[email protected]>
> Sent: Wednesday, January 10, 2024 7:09 AM
> To: Gaurav Jain (OSS) <[email protected]>
> Cc: [email protected]; Varun Sethi <[email protected]>; Aymen Sghaier
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Horia Geanta
> <[email protected]>; [email protected]; Meenakshi Aggarwal
> <[email protected]>
> Subject: [REMINDER] Re: [PATCH v5.10.y] crypto: caam/jr - Fix possible caam_jr
> crash
>
> Hello Gaurav,
>
> I hope you receive this email. I'm following up on a patch I submitted a few
> weeks ago. There doesn't seem to be any response yet and I want to make sure
> it gets pushed forward.
>
> I know you're busy and thank you for taking the time to focus on this.If you
> have any concerns or feedback please let me know and I'll be happy to address it.
>
> Best regards,
> SK
>Hello SK
>
>I am submitting and replying patches using [email protected]
>
>In Later kernel versions we have provided fixes related to job ring flush and there are other changes as well.
>It would be better to add these changes on top of 5.10 tree as we also run multiple tests and not observed this issue.
>
>Regards
>Gaurav Jain
Hi, Gaurav
Can you identify which commits?
Our version has added some related commits based on 5.10
-------------------------------------------------
04ff8e37a2df crypto: caam/jr - add .shutdown hook
f7ea4a6a6511 LF-3093-1 crypto: caam/jr - fix caam-keygen exit / clean-up
c41daf27fa44 LF-3079 crypto: caam/jr - fix shared IRQ line handling
1f12127de72a MLK-24912-2 crypto: caam - fix RNG vs. hwrng kthread race
04e3a61f9bb5 MLK-24912-1 crypto: caam/jr - update jr_list during suspend/resume
f48d9e23262e MLK-24420-3 crypto: caam - add ioctl calls for black keys and blobs generation
5c98742fbf60 MLK-24420-2 crypto: caam - add support for black keys and blobs
-------------------------------------------------
But We have no way to port this commit because its architecture has changed too much.
commit 304a2efe9d55875c6805f3c2957bc39ceebbc5c0
crypto: caam/jr - Convert to platform remove callback returning void
On your test enviroment, modify function caam_jr_remove
Test scenarios1:
if &jrpriv->tfm_count is not zero,whether will crash?
+atomic_inc(&jrpriv->tfm_count);
if (atomic_read(&jrpriv->tfm_count)) {
Thanks!
BR/SK
> -----Original Message-----
> From: Kun Song <[email protected]>
> Sent: Wednesday, January 10, 2024 1:56 PM
> To: Gaurav Jain <[email protected]>
> Cc: [email protected]; Varun Sethi <[email protected]>; Aymen Sghaier
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Horia Geanta
> <[email protected]>; [email protected]; Meenakshi Aggarwal
> <[email protected]>; [email protected]
> Subject: [EXT] RE: [PATCH v5.10.y] crypto: caam/jr - Fix possible caam_jr crash
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> >Hello SK
> >
> >I am submitting and replying patches using [email protected]
> >
> >In Later kernel versions we have provided fixes related to job ring flush and
> there are other changes as well.
> >It would be better to add these changes on top of 5.10 tree as we also run
> multiple tests and not observed this issue.
> >
> >Regards
> >Gaurav Jain
>
> Hi, Gaurav
>
> Can you identify which commits?
> Our version has added some related commits based on 5.10
> -------------------------------------------------
> 04ff8e37a2df crypto: caam/jr - add .shutdown hook
> f7ea4a6a6511 LF-3093-1 crypto: caam/jr - fix caam-keygen exit / clean-up
> c41daf27fa44 LF-3079 crypto: caam/jr - fix shared IRQ line handling
> 1f12127de72a MLK-24912-2 crypto: caam - fix RNG vs. hwrng kthread race
> 04e3a61f9bb5 MLK-24912-1 crypto: caam/jr - update jr_list during
> suspend/resume f48d9e23262e MLK-24420-3 crypto: caam - add ioctl calls for
> black keys and blobs generation
> 5c98742fbf60 MLK-24420-2 crypto: caam - add support for black keys and blobs
> -------------------------------------------------
https://github.com/torvalds/linux/commit/06e39357c36b0d3cc2779d08ed04cb389eaa22ba - drivers: crypto: caam/jr - Allow quiesce when quiesced
apply this one as well.
>
> But We have no way to port this commit because its architecture has changed
> too much.
> commit 304a2efe9d55875c6805f3c2957bc39ceebbc5c0
> crypto: caam/jr - Convert to platform remove callback returning void
Ok. I think this can be skipped.
>
>
> On your test enviroment, modify function caam_jr_remove Test scenarios1:
> if &jrpriv->tfm_count is not zero,whether will crash?
>
> +atomic_inc(&jrpriv->tfm_count);
> if (atomic_read(&jrpriv->tfm_count)) {
This way jobring flush operation will not be triggered and reusing jobring result in error.
>
>
> Thanks!
> BR/SK
>https://github.com/torvalds/linux/commit/06e39357c36b0d3cc2779d08ed04cb389eaa22ba - drivers: crypto: caam/jr - Allow quiesce when quiesced
>apply this one as well.
Hi, Gaurav
Our version has backport this commiti(06e39357c36b0d3cc2779d08ed04cb389eaa22ba), but the problem still exists.
When crash occurs, the log will output
<3>caam_jr 8030000.jr: Device is busy
<3>caam_jr 8020000.jr: Device is busy
<3>caam_jr 8010000.jr: Device is busy
<3>caam_jr 8010000.jr: job ring error: irqstate: 00000103
Thanks!
BR/SK
> -----Original Message-----
> From: Kun Song <[email protected]>
> Sent: Wednesday, January 10, 2024 5:27 PM
> To: Gaurav Jain <[email protected]>
> Cc: [email protected]; Varun Sethi <[email protected]>; Aymen Sghaier
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Horia Geanta
> <[email protected]>; [email protected]; Meenakshi Aggarwal
> <[email protected]>; [email protected]
> Subject: [EXT] RE: [PATCH v5.10.y] crypto: caam/jr - Fix possible caam_jr crash
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> >https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
> >b.com%2Ftorvalds%2Flinux%2Fcommit%2F06e39357c36b0d3cc2779d08ed04c
> b389eaa22ba&data=05%7C02%7Cgaurav.jain%40nxp.com%7Ce1aa575fb6344e
> d3c60f08dc11d342fd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 638404846267170151%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&s
> data=x37XZurb8Lu%2BGnJtkPUeCkBnZfOROXxo6rl4XNBzM18%3D&reserved=0 -
> drivers: crypto: caam/jr - Allow quiesce when quiesced apply this one as well.
>
> Hi, Gaurav
>
> Our version has backport this
> commiti(06e39357c36b0d3cc2779d08ed04cb389eaa22ba), but the problem still
> exists.
>
> When crash occurs, the log will output
>
> <3>caam_jr 8030000.jr: Device is busy
> <3>caam_jr 8020000.jr: Device is busy
> <3>caam_jr 8010000.jr: Device is busy
> <3>caam_jr 8010000.jr: job ring error: irqstate: 00000103
I have discussed this issue internally and this is a known issue. when caam_jr_remove is exited with EBUSY, caam JR is not released.
Disabling interrupt is only avoiding the crash for the jobs sitting in input ring.
As mentioned in the commit https://github.com/torvalds/linux/commit/304a2efe9d55875c6805f3c2957bc39ceebbc5c0
jrpriv & other register are freed automatically. So any calls to JR driver via crypto api will result in crash.
Regarding this problem, discussion have happened in the past but not concluded.
I will reinitiate the discussion to have a proper fix and will add you as well.
Regards
Gaurav Jain
>
>
> Thanks!
> BR/SK
On 12/21/2023 11:32 AM, Kun Song wrote:
> Test environment:
> Linux kernel version: 5.10.y
> Architecture: ARM Cortex-A
> Processor: NXP Layerscape LS1028
>
> Crash in reboot tests:
> Reproducibility: 1%
>
Replying here to comment on the log.
I've added all the people from the latest reply, i.e.:
https://lore.kernel.org/linux-crypto/AM0PR04MB6004AECDD044F1E6BF3B6732E7682@AM0PR04MB6004.eurprd04.prod.outlook.com
> If a job ring is still allocated, Once caam_jr_remove() returned,
> jrpriv will be freed and the registers will get unmapped.Then
In this case, most likely the root cause is different (see below).
> caam_jr_interrupt will get error irqstate value.
> So such a job ring will probably crash.Crash info is below:
> --------------------------------------
> RBS Sys: Restart ordered by epghd(0x1)
> RBS Sys: RESTARTING
This looks like a system restart.
> caam_jr 8030000.jr: Device is busy
> caam_jr 8020000.jr: Device is busy
> caam_jr 8010000.jr: Device is busy
For some reason, there are still tfms accounted for on all these caam job rings.
Maybe the system restart is not handled correctly at some point,
hence some resource leaks (unallocated tfms).
As already discussed, exiting early from caam_jr .remove() callback will leave
the HW unquiesced (e.g. job rings not flushed), interrupts still active.
> arm-smmu 5000000.iommu: disabling translation
From here onward caam memory accesses won't be translated.
> caam_jr 8010000.jr: job ring error: irqstate: 00000103
The error code means caam was not able to write the status of the job
it just finished in the output job ring (which is allocated in memory).
Most likely this happened due to iommu no longer translating the access.
>
> Disabling interrupts is to ensure that the device removal
> operation is not interrupted.
>
> Signed-off-by: Kun Song <[email protected]>
> Reviewed-by: Hen Guo <[email protected]>
> ---
> drivers/crypto/caam/jr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index 6f669966ba2c..d191e8caa1ad 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -135,6 +135,10 @@ static int caam_jr_remove(struct platform_device *pdev)
> jrdev = &pdev->dev;
> jrpriv = dev_get_drvdata(jrdev);
>
> + /* Disabling interrupts is ensure that the device removal operation
> + * is not interrupted by interrupts.
> + */
> + devm_free_irq(jrdev, jrpriv->irq, jrdev);
> if (jrpriv->hwrng)
> caam_rng_exit(jrdev->parent);
>
As pointed out by previous discussions, this is not enough.
Crashes could still occur due to crypto API users calling into caam driver,
which would be in an inconsistent state.
Whether .remove() being called is triggered by a system restart or
a manual device unbinding [1] is irrelevant, the driver mustn't crash.
I think Herbert's suggestion [2] on how to deal with HW devices going away
makes sense.
Not sure if the changes (thinking of crypto API part) could go into LTS kernels.
If not, we'll have to stick to caam driver-only changes (but IIUC
other crypto drivers are having the same issue with HW devices going away [3]).
Thanks,
Horia
[1] https://lore.kernel.org/linux-crypto/VI1PR04MB7023A7EC91599A537CB6A487EEB30@VI1PR04MB7023.eurprd04.prod.outlook.com/
[2] https://lore.kernel.org/linux-crypto/[email protected]/
[3] https://lore.kernel.org/linux-crypto/[email protected]/