Hi, Laurent.
On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart
<[email protected]> wrote:
> Hi Oleksandr,
>
> Thank you for the patch.
>
> On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <[email protected]>
>>
>> In ipmmu_domain_init_context() we are trying to allocate context and
>> if allocation fails we will call free_io_pgtable_ops(),
>> but "domain->context_id" hasn't been initialized yet (likely it is 0
>> because of kzalloc). Having the following call stack:
>> free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
>> ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
>> we will get a mistaken cache flush for a context pointed by
>> uninitialized "domain->context_id".
>>
>> So, set context_id to non-existent value (IPMMU_CTX_MAX) before calling
>> free_io_pgtable_ops() and check it for a valid value (< IPMMU_CTX_MAX)
>> before calling ipmmu_tlb_invalidate().
>>
>> Signed-off-by: Oleksandr Tyshchenko <[email protected]>
>> ---
>> drivers/iommu/ipmmu-vmsa.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>> index 2a38aa1..5b226c0 100644
>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -303,6 +303,9 @@ static void ipmmu_tlb_flush_all(void *cookie)
>> {
>> struct ipmmu_vmsa_domain *domain = cookie;
>>
>> + if (domain->context_id >= IPMMU_CTX_MAX)
>> + return;
>> +
>> ipmmu_tlb_invalidate(domain);
>> }
>>
>> @@ -380,6 +383,7 @@ static int ipmmu_domain_init_context(struct
>> ipmmu_vmsa_domain *domain) */
>> ret = ipmmu_domain_allocate_context(domain->mmu, domain);
>> if (ret == IPMMU_CTX_MAX) {
>> + domain->context_id = IPMMU_CTX_MAX;
>
> Wouldn't it make more sense to allocate the pgtable ops after initializing the
> context (moving the ipmmu_domain_allocate_context() call to the very end of
> the function) ? That way we would be less dependent on changes to pgtable ops
> init/cleanup code that could require the context to be set up.
Why not. But, not sure about the very end of the function. Since for
writing some HW registers down the function (IMTTLBR0/IMTTUBR0,
IMMAIR0)
we need to have what pgtable ops sets up for us (ttbr, mair). What
about to just swap alloc_io_pgtable_ops() and
ipmmu_domain_allocate_context()?
...
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 2a38aa1..90af1c7 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -370,22 +370,22 @@ static int ipmmu_domain_init_context(struct
ipmmu_vmsa_domain *domain)
*/
domain->cfg.iommu_dev = domain->mmu->dev;
- domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
- domain);
- if (!domain->iop)
- return -EINVAL;
-
/*
* Find an unused context.
*/
ret = ipmmu_domain_allocate_context(domain->mmu, domain);
- if (ret == IPMMU_CTX_MAX) {
- free_io_pgtable_ops(domain->iop);
+ if (ret == IPMMU_CTX_MAX)
return -EBUSY;
- }
domain->context_id = ret;
+ domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
+ domain);
+ if (!domain->iop) {
+ ipmmu_domain_free_context(domain->mmu, domain->context_id);
+ return -EINVAL;
+ }
+
/* TTBR0 */
ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
ipmmu_ctx_write(domain, IMTTLBR0, ttbr);
...
>
>> free_io_pgtable_ops(domain->iop);
>> return -EBUSY;
>> }
>
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
