* [PATCH v2 0/2] crypto: qat: fix couple crashes duing error handling @ 2021-03-12 14:19 Tong Zhang 2021-03-12 14:19 ` [PATCH v2 1/2] crypto: qat - dont release uninitialized resources Tong Zhang 2021-03-12 14:19 ` [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init Tong Zhang 0 siblings, 2 replies; 9+ messages in thread From: Tong Zhang @ 2021-03-12 14:19 UTC (permalink / raw) To: Giovanni Cabiddu, Herbert Xu, David S. Miller, Tong Zhang, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, linux-kernel, Andy Shevchenko There are a couple of issues in qat error handling. Those drivers tries to release resources that is not initialized. This patch series tries to fix crashes caused by incorrect error handling. v2: removed excessive dump in commit log Tong Zhang (2): crypto: qat - dont release uninitialized resources crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init drivers/crypto/qat/qat_c3xxxvf/adf_drv.c | 4 ++-- drivers/crypto/qat/qat_c62xvf/adf_drv.c | 4 ++-- drivers/crypto/qat/qat_common/adf_vf_isr.c | 17 +++++++++++++---- drivers/crypto/qat/qat_dh895xccvf/adf_drv.c | 4 ++-- 4 files changed, 19 insertions(+), 10 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] crypto: qat - dont release uninitialized resources 2021-03-12 14:19 [PATCH v2 0/2] crypto: qat: fix couple crashes duing error handling Tong Zhang @ 2021-03-12 14:19 ` Tong Zhang 2021-03-12 14:19 ` [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init Tong Zhang 1 sibling, 0 replies; 9+ messages in thread From: Tong Zhang @ 2021-03-12 14:19 UTC (permalink / raw) To: Giovanni Cabiddu, Herbert Xu, David S. Miller, Tong Zhang, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, linux-kernel, Andy Shevchenko adf_vf_isr_resource_alloc() is not unwinding correctly when error happens and it trys to release uninitialized resources. To fix this, only release initialized resources. [ 1.792845] Trying to free already-free IRQ 11 [ 1.793091] WARNING: CPU: 0 PID: 182 at kernel/irq/manage.c:1821 free_irq+0x202/0x380 [ 1.801340] Call Trace: [ 1.801477] adf_vf_isr_resource_free+0x32/0xb0 [intel_qat] [ 1.801785] adf_vf_isr_resource_alloc+0x14d/0x150 [intel_qat] [ 1.802105] adf_dev_init+0xba/0x140 [intel_qat] Signed-off-by: Tong Zhang <ztong0001@gmail.com> --- drivers/crypto/qat/qat_common/adf_vf_isr.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/crypto/qat/qat_common/adf_vf_isr.c b/drivers/crypto/qat/qat_common/adf_vf_isr.c index 38d316a42ba6..888388acb6bd 100644 --- a/drivers/crypto/qat/qat_common/adf_vf_isr.c +++ b/drivers/crypto/qat/qat_common/adf_vf_isr.c @@ -261,17 +261,26 @@ int adf_vf_isr_resource_alloc(struct adf_accel_dev *accel_dev) goto err_out; if (adf_setup_pf2vf_bh(accel_dev)) - goto err_out; + goto err_disable_msi; if (adf_setup_bh(accel_dev)) - goto err_out; + goto err_cleanup_pf2vf_bh; if (adf_request_msi_irq(accel_dev)) - goto err_out; + goto err_cleanup_bh; return 0; + +err_cleanup_bh: + adf_cleanup_bh(accel_dev); + +err_cleanup_pf2vf_bh: + adf_cleanup_pf2vf_bh(accel_dev); + +err_disable_msi: + adf_disable_msi(accel_dev); + err_out: - adf_vf_isr_resource_free(accel_dev); return -EFAULT; } EXPORT_SYMBOL_GPL(adf_vf_isr_resource_alloc); -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init 2021-03-12 14:19 [PATCH v2 0/2] crypto: qat: fix couple crashes duing error handling Tong Zhang 2021-03-12 14:19 ` [PATCH v2 1/2] crypto: qat - dont release uninitialized resources Tong Zhang @ 2021-03-12 14:19 ` Tong Zhang 2021-03-12 15:35 ` Andy Shevchenko 1 sibling, 1 reply; 9+ messages in thread From: Tong Zhang @ 2021-03-12 14:19 UTC (permalink / raw) To: Giovanni Cabiddu, Herbert Xu, David S. Miller, Tong Zhang, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, linux-kernel, Andy Shevchenko ADF_STATUS_PF_RUNNING is (only) used and checked by adf_vf2pf_shutdown() before calling adf_iov_putmsg()->mutex_lock(vf2pf_lock), however the vf2pf_lock is initialized in adf_dev_init(), which can fail and when it fail, the vf2pf_lock is either not initialized or destroyed, a subsequent use of vf2pf_lock will cause issue. To fix this issue, only set this flag if adf_dev_init() returns 0. [ 7.178404] BUG: KASAN: user-memory-access in __mutex_lock.isra.0+0x1ac/0x7c0 [ 7.180345] Call Trace: [ 7.182576] mutex_lock+0xc9/0xd0 [ 7.183257] adf_iov_putmsg+0x118/0x1a0 [intel_qat] [ 7.183541] adf_vf2pf_shutdown+0x4d/0x7b [intel_qat] [ 7.183834] adf_dev_shutdown+0x172/0x2b0 [intel_qat] [ 7.184127] adf_probe+0x5e9/0x600 [qat_dh895xccvf] Signed-off-by: Tong Zhang <ztong0001@gmail.com> --- drivers/crypto/qat/qat_c3xxxvf/adf_drv.c | 4 ++-- drivers/crypto/qat/qat_c62xvf/adf_drv.c | 4 ++-- drivers/crypto/qat/qat_dh895xccvf/adf_drv.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c b/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c index 1d1532e8fb6d..067ca5e17d38 100644 --- a/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c +++ b/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c @@ -184,12 +184,12 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) goto out_err_free_reg; - set_bit(ADF_STATUS_PF_RUNNING, &accel_dev->status); - ret = adf_dev_init(accel_dev); if (ret) goto out_err_dev_shutdown; + set_bit(ADF_STATUS_PF_RUNNING, &accel_dev->status); + ret = adf_dev_start(accel_dev); if (ret) goto out_err_dev_stop; diff --git a/drivers/crypto/qat/qat_c62xvf/adf_drv.c b/drivers/crypto/qat/qat_c62xvf/adf_drv.c index 04742a6d91ca..51ea88c0b17d 100644 --- a/drivers/crypto/qat/qat_c62xvf/adf_drv.c +++ b/drivers/crypto/qat/qat_c62xvf/adf_drv.c @@ -184,12 +184,12 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) goto out_err_free_reg; - set_bit(ADF_STATUS_PF_RUNNING, &accel_dev->status); - ret = adf_dev_init(accel_dev); if (ret) goto out_err_dev_shutdown; + set_bit(ADF_STATUS_PF_RUNNING, &accel_dev->status); + ret = adf_dev_start(accel_dev); if (ret) goto out_err_dev_stop; diff --git a/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c b/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c index c972554a755e..29999da716cc 100644 --- a/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c +++ b/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c @@ -184,12 +184,12 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) goto out_err_free_reg; - set_bit(ADF_STATUS_PF_RUNNING, &accel_dev->status); - ret = adf_dev_init(accel_dev); if (ret) goto out_err_dev_shutdown; + set_bit(ADF_STATUS_PF_RUNNING, &accel_dev->status); + ret = adf_dev_start(accel_dev); if (ret) goto out_err_dev_stop; -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init 2021-03-12 14:19 ` [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init Tong Zhang @ 2021-03-12 15:35 ` Andy Shevchenko 2021-03-12 15:48 ` Tong Zhang 0 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2021-03-12 15:35 UTC (permalink / raw) To: Tong Zhang Cc: Giovanni Cabiddu, Herbert Xu, David S. Miller, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, Linux Kernel Mailing List, Andy Shevchenko On Fri, Mar 12, 2021 at 4:21 PM Tong Zhang <ztong0001@gmail.com> wrote: > > ADF_STATUS_PF_RUNNING is (only) used and checked by adf_vf2pf_shutdown() > before calling adf_iov_putmsg()->mutex_lock(vf2pf_lock), however the > vf2pf_lock is initialized in adf_dev_init(), which can fail and when it > fail, the vf2pf_lock is either not initialized or destroyed, a subsequent > use of vf2pf_lock will cause issue. > To fix this issue, only set this flag if adf_dev_init() returns 0. > > [ 7.178404] BUG: KASAN: user-memory-access in __mutex_lock.isra.0+0x1ac/0x7c0 > [ 7.180345] Call Trace: > [ 7.182576] mutex_lock+0xc9/0xd0 > [ 7.183257] adf_iov_putmsg+0x118/0x1a0 [intel_qat] > [ 7.183541] adf_vf2pf_shutdown+0x4d/0x7b [intel_qat] > [ 7.183834] adf_dev_shutdown+0x172/0x2b0 [intel_qat] > [ 7.184127] adf_probe+0x5e9/0x600 [qat_dh895xccvf] Don't you miss the tag I gave? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init 2021-03-12 15:35 ` Andy Shevchenko @ 2021-03-12 15:48 ` Tong Zhang 2021-03-12 15:57 ` Andy Shevchenko 0 siblings, 1 reply; 9+ messages in thread From: Tong Zhang @ 2021-03-12 15:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Giovanni Cabiddu, Herbert Xu, David S. Miller, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, Linux Kernel Mailing List, Andy Shevchenko Hi Andy, Complete newbie here, could you please remind me of the tag you are referring to? I am not really familiar with the process. Thanks, - Tong On Fri, Mar 12, 2021 at 10:35 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 12, 2021 at 4:21 PM Tong Zhang <ztong0001@gmail.com> wrote: > > > > ADF_STATUS_PF_RUNNING is (only) used and checked by adf_vf2pf_shutdown() > > before calling adf_iov_putmsg()->mutex_lock(vf2pf_lock), however the > > vf2pf_lock is initialized in adf_dev_init(), which can fail and when it > > fail, the vf2pf_lock is either not initialized or destroyed, a subsequent > > use of vf2pf_lock will cause issue. > > To fix this issue, only set this flag if adf_dev_init() returns 0. > > > > [ 7.178404] BUG: KASAN: user-memory-access in __mutex_lock.isra.0+0x1ac/0x7c0 > > [ 7.180345] Call Trace: > > [ 7.182576] mutex_lock+0xc9/0xd0 > > [ 7.183257] adf_iov_putmsg+0x118/0x1a0 [intel_qat] > > [ 7.183541] adf_vf2pf_shutdown+0x4d/0x7b [intel_qat] > > [ 7.183834] adf_dev_shutdown+0x172/0x2b0 [intel_qat] > > [ 7.184127] adf_probe+0x5e9/0x600 [qat_dh895xccvf] > > Don't you miss the tag I gave? > > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init 2021-03-12 15:48 ` Tong Zhang @ 2021-03-12 15:57 ` Andy Shevchenko 2021-03-12 16:10 ` Tong Zhang 0 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2021-03-12 15:57 UTC (permalink / raw) To: Tong Zhang Cc: Giovanni Cabiddu, Herbert Xu, David S. Miller, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, Linux Kernel Mailing List, Andy Shevchenko On Fri, Mar 12, 2021 at 5:48 PM Tong Zhang <ztong0001@gmail.com> wrote: Please, do not top post when replying to the email. > Complete newbie here, could you please remind me of the tag you are > referring to? Reviewed-by IIRC. > I am not really familiar with the process. Have you read [1]? The chapters 11-13 refer to the tags. > On Fri, Mar 12, 2021 at 10:35 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Mar 12, 2021 at 4:21 PM Tong Zhang <ztong0001@gmail.com> wrote: > > > > > > ADF_STATUS_PF_RUNNING is (only) used and checked by adf_vf2pf_shutdown() > > > before calling adf_iov_putmsg()->mutex_lock(vf2pf_lock), however the > > > vf2pf_lock is initialized in adf_dev_init(), which can fail and when it > > > fail, the vf2pf_lock is either not initialized or destroyed, a subsequent > > > use of vf2pf_lock will cause issue. > > > To fix this issue, only set this flag if adf_dev_init() returns 0. > > > > > > [ 7.178404] BUG: KASAN: user-memory-access in __mutex_lock.isra.0+0x1ac/0x7c0 > > > [ 7.180345] Call Trace: > > > [ 7.182576] mutex_lock+0xc9/0xd0 > > > [ 7.183257] adf_iov_putmsg+0x118/0x1a0 [intel_qat] > > > [ 7.183541] adf_vf2pf_shutdown+0x4d/0x7b [intel_qat] > > > [ 7.183834] adf_dev_shutdown+0x172/0x2b0 [intel_qat] > > > [ 7.184127] adf_probe+0x5e9/0x600 [qat_dh895xccvf] > > > > Don't you miss the tag I gave? [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init 2021-03-12 15:57 ` Andy Shevchenko @ 2021-03-12 16:10 ` Tong Zhang 2021-03-12 16:16 ` Andy Shevchenko 0 siblings, 1 reply; 9+ messages in thread From: Tong Zhang @ 2021-03-12 16:10 UTC (permalink / raw) To: Andy Shevchenko Cc: Giovanni Cabiddu, Herbert Xu, David S. Miller, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, Linux Kernel Mailing List, Andy Shevchenko Hi Andy, I am not really sure this reviewed by line should be added by me -- IMHO from my past experience this line is added by the actual person who reviewed it on a per-patch and version basis I can send out another revision adding this Reviewed-by line if you think this is something should be done by me, but I don't feel I have such power to do this since I am not that guy and I am not a maintainer. Thanks, - Tong ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init 2021-03-12 16:10 ` Tong Zhang @ 2021-03-12 16:16 ` Andy Shevchenko 2021-03-12 16:23 ` Tong Zhang 0 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2021-03-12 16:16 UTC (permalink / raw) To: Tong Zhang Cc: Giovanni Cabiddu, Herbert Xu, David S. Miller, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, Linux Kernel Mailing List, Andy Shevchenko On Fri, Mar 12, 2021 at 6:10 PM Tong Zhang <ztong0001@gmail.com> wrote: > I am not really sure this reviewed by line should be added by me -- > IMHO from my past experience this line is added by the actual person > who reviewed it on a per-patch and version basis > I can send out another revision adding this Reviewed-by line if you > think this is something should be done by me, > but I don't feel I have such power to do this since I am not that guy > and I am not a maintainer. If you are sending a new version, it's your responsibility to collect all tags, except the cases when the code in question drastically changed between versions of the series. So, please add a tag (and feel free to add the same tag to the patch 1) and resend as v3. Thanks! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init 2021-03-12 16:16 ` Andy Shevchenko @ 2021-03-12 16:23 ` Tong Zhang 0 siblings, 0 replies; 9+ messages in thread From: Tong Zhang @ 2021-03-12 16:23 UTC (permalink / raw) To: Andy Shevchenko Cc: Giovanni Cabiddu, Herbert Xu, David S. Miller, Fiona Trahe, Wojciech Ziemba, qat-linux, linux-crypto, Linux Kernel Mailing List, Andy Shevchenko Thanks Andy, I have sent v3 with the suggested tag fix. - Tong ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-12 16:24 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-12 14:19 [PATCH v2 0/2] crypto: qat: fix couple crashes duing error handling Tong Zhang 2021-03-12 14:19 ` [PATCH v2 1/2] crypto: qat - dont release uninitialized resources Tong Zhang 2021-03-12 14:19 ` [PATCH v2 2/2] crypto: qat: ADF_STATUS_PF_RUNNING should be set after adf_dev_init Tong Zhang 2021-03-12 15:35 ` Andy Shevchenko 2021-03-12 15:48 ` Tong Zhang 2021-03-12 15:57 ` Andy Shevchenko 2021-03-12 16:10 ` Tong Zhang 2021-03-12 16:16 ` Andy Shevchenko 2021-03-12 16:23 ` Tong Zhang
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.