| Message ID | 1596781478-12216-4-git-send-email-mansur@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Venus - Handle race conditions in concurrency | expand |
Hi, On 8/7/20 9:24 AM, Mansur Alisha Shaik wrote: > In concurrency usecase and reboot scenario we are trying > to map fw.iommu_domain which is already unmapped during I guess you want to say "to unmap iommu domain which is already unmapped" ? > shutdown. This is causing NULL pointer dereference crash. > > This case is handled by adding necessary checks. > > Call trace: > __iommu_map+0x4c/0x348 > iommu_map+0x5c/0x70 > venus_boot+0x184/0x230 [venus_core] > venus_sys_error_handler+0xa0/0x14c [venus_core] > process_one_work+0x210/0x3d0 > worker_thread+0x248/0x3f4 > kthread+0x11c/0x12c > ret_from_fork+0x10/0x18 > > Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org> > --- > drivers/media/platform/qcom/venus/firmware.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index 8801a6a..c427e88 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core) > > iommu = core->fw.iommu_domain; > > - unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped); > - if (unmapped != mapped) > - dev_err(dev, "failed to unmap firmware\n"); > + if (core->fw.mapped_mem_size && iommu) { > + unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped); > + > + if (unmapped != mapped) > + dev_err(dev, "failed to unmap firmware\n"); > + else > + core->fw.mapped_mem_size = 0; > + } > > return 0; > } > @@ -288,7 +293,11 @@ void venus_firmware_deinit(struct venus_core *core) > iommu = core->fw.iommu_domain; > > iommu_detach_device(iommu, core->fw.dev); > - iommu_domain_free(iommu); > + > + if (core->fw.iommu_domain) { > + iommu_domain_free(iommu); > + core->fw.iommu_domain = NULL; > + } > > platform_device_unregister(to_platform_device(core->fw.dev)); > } >
On 8/7/20 9:24 AM, Mansur Alisha Shaik wrote: > In concurrency usecase and reboot scenario we are trying > to map fw.iommu_domain which is already unmapped during > shutdown. This is causing NULL pointer dereference crash. > > This case is handled by adding necessary checks. > > Call trace: > __iommu_map+0x4c/0x348 > iommu_map+0x5c/0x70 > venus_boot+0x184/0x230 [venus_core] > venus_sys_error_handler+0xa0/0x14c [venus_core] > process_one_work+0x210/0x3d0 > worker_thread+0x248/0x3f4 > kthread+0x11c/0x12c > ret_from_fork+0x10/0x18 > > Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org> > --- > drivers/media/platform/qcom/venus/firmware.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index 8801a6a..c427e88 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core) > > iommu = core->fw.iommu_domain; > > - unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped); > - if (unmapped != mapped) > - dev_err(dev, "failed to unmap firmware\n"); > + if (core->fw.mapped_mem_size && iommu) { > + unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped); > + > + if (unmapped != mapped) > + dev_err(dev, "failed to unmap firmware\n"); > + else > + core->fw.mapped_mem_size = 0; > + } > > return 0; > } > @@ -288,7 +293,11 @@ void venus_firmware_deinit(struct venus_core *core) > iommu = core->fw.iommu_domain; > > iommu_detach_device(iommu, core->fw.dev); > - iommu_domain_free(iommu); > + > + if (core->fw.iommu_domain) { why not just ? if (iommu) > + iommu_domain_free(iommu); > + core->fw.iommu_domain = NULL; > + } > > platform_device_unregister(to_platform_device(core->fw.dev)); > } >
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c index 8801a6a..c427e88 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core) iommu = core->fw.iommu_domain; - unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped); - if (unmapped != mapped) - dev_err(dev, "failed to unmap firmware\n"); + if (core->fw.mapped_mem_size && iommu) { + unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped); + + if (unmapped != mapped) + dev_err(dev, "failed to unmap firmware\n"); + else + core->fw.mapped_mem_size = 0; + } return 0; } @@ -288,7 +293,11 @@ void venus_firmware_deinit(struct venus_core *core) iommu = core->fw.iommu_domain; iommu_detach_device(iommu, core->fw.dev); - iommu_domain_free(iommu); + + if (core->fw.iommu_domain) { + iommu_domain_free(iommu); + core->fw.iommu_domain = NULL; + } platform_device_unregister(to_platform_device(core->fw.dev)); }
In concurrency usecase and reboot scenario we are trying to map fw.iommu_domain which is already unmapped during shutdown. This is causing NULL pointer dereference crash. This case is handled by adding necessary checks. Call trace: __iommu_map+0x4c/0x348 iommu_map+0x5c/0x70 venus_boot+0x184/0x230 [venus_core] venus_sys_error_handler+0xa0/0x14c [venus_core] process_one_work+0x210/0x3d0 worker_thread+0x248/0x3f4 kthread+0x11c/0x12c ret_from_fork+0x10/0x18 Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org> --- drivers/media/platform/qcom/venus/firmware.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)