* [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 related [flat|nested] 7+ messages in thread
[parent not found: <1503498702-27996-1-git-send-email-olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* 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
[parent not found: <CAPD2p-nA0XX2KqhzzeCrWwM4eN6EKhD_EnuVynCGGA+SnO=x8Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* 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
[parent not found: <CAPD2p-m1guANCo+y3AvmFKK=RfPf1iFRj65bh2+gwxBOAauQvQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* 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.