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

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

[Oleksandr Tyshchenko]

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;
 		free_io_pgtable_ops(domain->iop);
 		return -EBUSY;
 	}
-- 
2.7.4

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

[Joerg Roedel]

On Mon, Aug 21, 2017 at 03:40:41PM +0300, 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(+)

Applied, thanks.

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

[Oleksandr Tyshchenko]

Hi,

On Tue, Aug 22, 2017 at 5:24 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Aug 21, 2017 at 03:40:41PM +0300, 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(+)
>
> Applied, thanks.
Thank you.

>

-- 
Regards,

Oleksandr Tyshchenko

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

[Laurent Pinchart]

Hi Joerg,

On Tuesday, 22 August 2017 17:24:30 EEST Joerg Roedel wrote:
> On Mon, Aug 21, 2017 at 03:40:41PM +0300, 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(+)
> 
> Applied, thanks.

It would be nice to give reviewers a week before applying a patch, especially 
when it has no review at all.

-- 
Regards,

Laurent Pinchart

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

[Joerg Roedel]

Hi Laurent,

On Wed, Aug 23, 2017 at 12:15:35AM +0300, Laurent Pinchart wrote:
> It would be nice to give reviewers a week before applying a patch, especially 
> when it has no review at all.

It's not pushed yet and it looked simple enough during my review that I
felt ok to take it. If you disagree with it, I can easily remove it
again. Let me know what you prefer.


Regards,

	Joerg

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

[Laurent Pinchart]

Hi Oleksandr,

Thank you for the patch.

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.

>  		free_io_pgtable_ops(domain->iop);
>  		return -EBUSY;
>  	}


-- 
Regards,

Laurent Pinchart

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

[Oleksandr Tyshchenko]

Hi, Laurent.

On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> Hi Oleksandr,
>
> Thank you for the patch.
>
> 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()?

...
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

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

[Robin Murphy]

On 23/08/17 10:36, Oleksandr Tyshchenko wrote:
> Hi, Laurent.
> 
> On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart
> <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
>> Hi Oleksandr,
>>
>> Thank you for the patch.
>>
>> 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.

Robin.

> ...
> 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
>>
> 
> 
> 

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

[Oleksandr Tyshchenko]

Hi, Robin

On Wed, Aug 23, 2017 at 1:05 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> On 23/08/17 10:36, Oleksandr Tyshchenko wrote:
>> Hi, Laurent.
>>
>> On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart
>> <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
>>> Hi Oleksandr,
>>>
>>> Thank you for the patch.
>>>
>>> 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.

>
> Robin.
>
>> ...
>> 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

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

[Laurent Pinchart]

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

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

[Oleksandr Tyshchenko]

Hi, Laurent

On Wed, Aug 23, 2017 at 4:46 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> 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.
Make sense.

>
> Will you send a v2 ?
Yes.

>
> --
> Regards,
>
> Laurent Pinchart
>

-- 
Regards,

Oleksandr Tyshchenko