Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Oleksandr,

On Wednesday, 23 August 2017 14:58:47 EEST Oleksandr Tyshchenko wrote:
> On Wed, Aug 23, 2017 at 1:05 PM, Robin Murphy wrote:
> > On 23/08/17 10:36, Oleksandr Tyshchenko wrote:
> >> On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart wrote:
> >>> On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote:
> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
> >>>> 
> >>>> 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 <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
> >>>> ---
> >>>> 
> >>>>  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()?
> > 
> > This looks a lot more reasonable - reserving a free context is both
> > quicker and more likely to fail (due to limited hardware resources) than
> > setting up a pagetable, so it makes a lot of sense to do that before
> > anything else.
> 
> Agree.

That looks good to me too. In general I prefer initializing everything needed 
by the error path before calling anything that could trigger that error path, 
instead of initializing variables to magic values that mean part of the 
cleanup should be skipped.

Will you send a v2 ?

-- 
Regards,

Laurent Pinchart