All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
@ 2017-08-23 14:31 Oleksandr Tyshchenko
       [not found] ` <1503498702-27996-1-git-send-email-olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Oleksandr Tyshchenko @ 2017-08-23 14:31 UTC (permalink / raw)
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw,
	geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
	damm+renesas-yzvPICuk2ACczHhG9Qg4qA, will.deacon-5wv7dgnIgG8,
	Oleksandr Tyshchenko, Laurent Pinchart

From: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>

Reserving a free context is both quicker and more likely to fail
(due to limited hardware resources) than setting up a pagetable.
What is more the pagetable init/cleanup code could require
the context to be set up.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
CC: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
CC: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>

---
This patch fixes a bug during rollback logic:
In ipmmu_domain_init_context() we are trying to find an unused context
and if operation 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".


v1 here:
https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
---
 drivers/iommu/ipmmu-vmsa.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 2a38aa1..382f387 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
 	return ret;
 }
 
+static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
+				      unsigned int context_id)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&mmu->lock, flags);
+
+	clear_bit(context_id, mmu->ctx);
+	mmu->domains[context_id] = NULL;
+
+	spin_unlock_irqrestore(&mmu->lock, flags);
+}
+
 static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 {
 	u64 ttbr;
@@ -370,22 +383,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);
@@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 	return 0;
 }
 
-static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
-				      unsigned int context_id)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&mmu->lock, flags);
-
-	clear_bit(context_id, mmu->ctx);
-	mmu->domains[context_id] = NULL;
-
-	spin_unlock_irqrestore(&mmu->lock, flags);
-}
-
 static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
 {
 	/*
-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
       [not found] ` <1503498702-27996-1-git-send-email-olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-08-28 13:23   ` Oleksandr Tyshchenko
       [not found]     ` <CAPD2p-nA0XX2KqhzzeCrWwM4eN6EKhD_EnuVynCGGA+SnO=x8Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2017-08-29 13:01   ` Laurent Pinchart
  2017-08-30 13:11   ` Joerg Roedel
  2 siblings, 1 reply; 7+ messages in thread
From: Oleksandr Tyshchenko @ 2017-08-28 13:23 UTC (permalink / raw)
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw,
	geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
	damm+renesas-yzvPICuk2ACczHhG9Qg4qA, Will Deacon,
	Oleksandr Tyshchenko, Laurent Pinchart

Hi, all.

Any comments?

On Wed, Aug 23, 2017 at 5:31 PM, Oleksandr Tyshchenko
<olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
>
> Reserving a free context is both quicker and more likely to fail
> (due to limited hardware resources) than setting up a pagetable.
> What is more the pagetable init/cleanup code could require
> the context to be set up.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
> CC: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> CC: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>
> ---
> This patch fixes a bug during rollback logic:
> In ipmmu_domain_init_context() we are trying to find an unused context
> and if operation 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".
>
>
> v1 here:
> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
> ---
>  drivers/iommu/ipmmu-vmsa.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 2a38aa1..382f387 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
>         return ret;
>  }
>
> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> +                                     unsigned int context_id)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&mmu->lock, flags);
> +
> +       clear_bit(context_id, mmu->ctx);
> +       mmu->domains[context_id] = NULL;
> +
> +       spin_unlock_irqrestore(&mmu->lock, flags);
> +}
> +
>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>  {
>         u64 ttbr;
> @@ -370,22 +383,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);
> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>         return 0;
>  }
>
> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> -                                     unsigned int context_id)
> -{
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&mmu->lock, flags);
> -
> -       clear_bit(context_id, mmu->ctx);
> -       mmu->domains[context_id] = NULL;
> -
> -       spin_unlock_irqrestore(&mmu->lock, flags);
> -}
> -
>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>  {
>         /*
> --
> 2.7.4
>



-- 
Regards,

Oleksandr Tyshchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
       [not found]     ` <CAPD2p-nA0XX2KqhzzeCrWwM4eN6EKhD_EnuVynCGGA+SnO=x8Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-08-29 10:49       ` Robin Murphy
  0 siblings, 0 replies; 7+ messages in thread
From: Robin Murphy @ 2017-08-29 10:49 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw,
	geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, Will Deacon,
	damm+renesas-yzvPICuk2ACczHhG9Qg4qA, Oleksandr Tyshchenko,
	Laurent Pinchart

On 28/08/17 14:23, Oleksandr Tyshchenko wrote:
> Hi, all.
> 
> Any comments?
> 
> On Wed, Aug 23, 2017 at 5:31 PM, Oleksandr Tyshchenko
> <olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
>>
>> Reserving a free context is both quicker and more likely to fail
>> (due to limited hardware resources) than setting up a pagetable.
>> What is more the pagetable init/cleanup code could require
>> the context to be set up.

Looks OK to me,

Reviewed-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
>> CC: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> CC: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>>
>> ---
>> This patch fixes a bug during rollback logic:
>> In ipmmu_domain_init_context() we are trying to find an unused context
>> and if operation 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".
>>
>>
>> v1 here:
>> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
>> ---
>>  drivers/iommu/ipmmu-vmsa.c | 42 +++++++++++++++++++++---------------------
>>  1 file changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>> index 2a38aa1..382f387 100644
>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
>>         return ret;
>>  }
>>
>> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> +                                     unsigned int context_id)
>> +{
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&mmu->lock, flags);
>> +
>> +       clear_bit(context_id, mmu->ctx);
>> +       mmu->domains[context_id] = NULL;
>> +
>> +       spin_unlock_irqrestore(&mmu->lock, flags);
>> +}
>> +
>>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>>  {
>>         u64 ttbr;
>> @@ -370,22 +383,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);
>> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>>         return 0;
>>  }
>>
>> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> -                                     unsigned int context_id)
>> -{
>> -       unsigned long flags;
>> -
>> -       spin_lock_irqsave(&mmu->lock, flags);
>> -
>> -       clear_bit(context_id, mmu->ctx);
>> -       mmu->domains[context_id] = NULL;
>> -
>> -       spin_unlock_irqrestore(&mmu->lock, flags);
>> -}
>> -
>>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>>  {
>>         /*
>> --
>> 2.7.4
>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
       [not found] ` <1503498702-27996-1-git-send-email-olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2017-08-28 13:23   ` Oleksandr Tyshchenko
@ 2017-08-29 13:01   ` Laurent Pinchart
  2017-08-29 13:30     ` Oleksandr Tyshchenko
  2017-08-30 13:11   ` Joerg Roedel
  2 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2017-08-29 13:01 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
	damm+renesas-yzvPICuk2ACczHhG9Qg4qA, will.deacon-5wv7dgnIgG8,
	Oleksandr Tyshchenko,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA

Hi Oleksandr,

Thank you for the patch.

On Wednesday, 23 August 2017 17:31:42 EEST Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
> 
> Reserving a free context is both quicker and more likely to fail
> (due to limited hardware resources) than setting up a pagetable.
> What is more the pagetable init/cleanup code could require
> the context to be set up.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
> CC: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> CC: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> ---
> This patch fixes a bug during rollback logic:
> In ipmmu_domain_init_context() we are trying to find an unused context
> and if operation 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".
> 
> 
> v1 here:
> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
> ---
>  drivers/iommu/ipmmu-vmsa.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 2a38aa1..382f387 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct
> ipmmu_vmsa_device *mmu, return ret;
>  }
> 
> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> +				      unsigned int context_id)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mmu->lock, flags);
> +
> +	clear_bit(context_id, mmu->ctx);
> +	mmu->domains[context_id] = NULL;
> +
> +	spin_unlock_irqrestore(&mmu->lock, flags);
> +}
> +
>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>  {
>  	u64 ttbr;
> @@ -370,22 +383,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);
> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct
> ipmmu_vmsa_domain *domain) return 0;
>  }
> 
> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> -				      unsigned int context_id)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&mmu->lock, flags);
> -
> -	clear_bit(context_id, mmu->ctx);
> -	mmu->domains[context_id] = NULL;
> -
> -	spin_unlock_irqrestore(&mmu->lock, flags);
> -}
> -
>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>  {
>  	/*


-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
  2017-08-29 13:01   ` Laurent Pinchart
@ 2017-08-29 13:30     ` Oleksandr Tyshchenko
       [not found]       ` <CAPD2p-m1guANCo+y3AvmFKK=RfPf1iFRj65bh2+gwxBOAauQvQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Oleksandr Tyshchenko @ 2017-08-29 13:30 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
	damm+renesas-yzvPICuk2ACczHhG9Qg4qA, Will Deacon,
	Oleksandr Tyshchenko,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA

Hi Laurent, Robin

On Tue, Aug 29, 2017 at 4:01 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> Hi Oleksandr,
>
> Thank you for the patch.
>
> On Wednesday, 23 August 2017 17:31:42 EEST Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
>>
>> Reserving a free context is both quicker and more likely to fail
>> (due to limited hardware resources) than setting up a pagetable.
>> What is more the pagetable init/cleanup code could require
>> the context to be set up.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
>> CC: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> CC: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>
>> ---
>> This patch fixes a bug during rollback logic:
>> In ipmmu_domain_init_context() we are trying to find an unused context
>> and if operation 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".
>>
>>
>> v1 here:
>> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
>> ---
>>  drivers/iommu/ipmmu-vmsa.c | 42 +++++++++++++++++++++---------------------
>>  1 file changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>> index 2a38aa1..382f387 100644
>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct
>> ipmmu_vmsa_device *mmu, return ret;
>>  }
>>
>> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> +                                   unsigned int context_id)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&mmu->lock, flags);
>> +
>> +     clear_bit(context_id, mmu->ctx);
>> +     mmu->domains[context_id] = NULL;
>> +
>> +     spin_unlock_irqrestore(&mmu->lock, flags);
>> +}
>> +
>>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>>  {
>>       u64 ttbr;
>> @@ -370,22 +383,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);
>> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct
>> ipmmu_vmsa_domain *domain) return 0;
>>  }
>>
>> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> -                                   unsigned int context_id)
>> -{
>> -     unsigned long flags;
>> -
>> -     spin_lock_irqsave(&mmu->lock, flags);
>> -
>> -     clear_bit(context_id, mmu->ctx);
>> -     mmu->domains[context_id] = NULL;
>> -
>> -     spin_unlock_irqrestore(&mmu->lock, flags);
>> -}
>> -
>>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>>  {
>>       /*
>
>
> --
> Regards,
>
> Laurent Pinchart
>

Thank you for the review.
Shall I resend patch with added R-bs?

-- 
Regards,

Oleksandr Tyshchenko

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
       [not found]       ` <CAPD2p-m1guANCo+y3AvmFKK=RfPf1iFRj65bh2+gwxBOAauQvQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-08-29 14:22         ` Joerg Roedel
  0 siblings, 0 replies; 7+ messages in thread
From: Joerg Roedel @ 2017-08-29 14:22 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
	damm+renesas-yzvPICuk2ACczHhG9Qg4qA, Will Deacon,
	Oleksandr Tyshchenko,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	Laurent Pinchart

On Tue, Aug 29, 2017 at 04:30:55PM +0300, Oleksandr Tyshchenko wrote:
> Thank you for the review.
> Shall I resend patch with added R-bs?

No, I add them when applying the patch.


Thanks,

	Joerg

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
       [not found] ` <1503498702-27996-1-git-send-email-olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2017-08-28 13:23   ` Oleksandr Tyshchenko
  2017-08-29 13:01   ` Laurent Pinchart
@ 2017-08-30 13:11   ` Joerg Roedel
  2 siblings, 0 replies; 7+ messages in thread
From: Joerg Roedel @ 2017-08-30 13:11 UTC (permalink / raw)
  To: Oleksandr Tyshchenko
  Cc: laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw,
	geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
	damm+renesas-yzvPICuk2ACczHhG9Qg4qA, will.deacon-5wv7dgnIgG8,
	Oleksandr Tyshchenko,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	Laurent Pinchart

On Wed, Aug 23, 2017 at 05:31:42PM +0300, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
> 
> Reserving a free context is both quicker and more likely to fail
> (due to limited hardware resources) than setting up a pagetable.
> What is more the pagetable init/cleanup code could require
> the context to be set up.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko-uRwfk40T5oI@public.gmane.org>
> CC: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> CC: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>

Applied, thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-08-30 13:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-23 14:31 [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable Oleksandr Tyshchenko
     [not found] ` <1503498702-27996-1-git-send-email-olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-08-28 13:23   ` Oleksandr Tyshchenko
     [not found]     ` <CAPD2p-nA0XX2KqhzzeCrWwM4eN6EKhD_EnuVynCGGA+SnO=x8Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-08-29 10:49       ` Robin Murphy
2017-08-29 13:01   ` Laurent Pinchart
2017-08-29 13:30     ` Oleksandr Tyshchenko
     [not found]       ` <CAPD2p-m1guANCo+y3AvmFKK=RfPf1iFRj65bh2+gwxBOAauQvQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-08-29 14:22         ` Joerg Roedel
2017-08-30 13:11   ` Joerg Roedel

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.