Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
Hi, Laurent On Wed, Aug 23, 2017 at 4:46 PM, Laurent Pinchartwrote: > 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 >> >> 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 >> --- >> >> 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
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> > 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 > --- > > 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
Hi, Robin On Wed, Aug 23, 2017 at 1:05 PM, Robin Murphywrote: > On 23/08/17 10:36, Oleksandr Tyshchenko wrote: >> Hi, Laurent. >> >> On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart >> wrote: >>> Hi Oleksandr, >>> >>> Thank you for the patch. >>> >>> On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote: From: Oleksandr Tyshchenko 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 --- 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, >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, >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 iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
On 23/08/17 10:36, Oleksandr Tyshchenko wrote: > Hi, Laurent. > > On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart >wrote: >> Hi Oleksandr, >> >> Thank you for the patch. >> >> On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote: >>> From: Oleksandr Tyshchenko >>> >>> 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 >>> --- >>> 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, >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, >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 >> > > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
Hi, Laurent. On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchartwrote: > Hi Oleksandr, > > Thank you for the patch. > > On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko >> >> 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 >> --- >> 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, >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, >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 iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
Hi Oleksandr, Thank you for the patch. On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko> > 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 > --- > 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
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> > > > 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 > > --- > > > > 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
Hi, On Tue, Aug 22, 2017 at 5:24 PM, Joerg Roedelwrote: > On Mon, Aug 21, 2017 at 03:40:41PM +0300, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko >> >> 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 >> --- >> drivers/iommu/ipmmu-vmsa.c | 4 >> 1 file changed, 4 insertions(+) > > Applied, thanks. Thank you. > -- Regards, Oleksandr Tyshchenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/ipmmu-vmsa: Set context_id to non-existent value if allocation failed
On Mon, Aug 21, 2017 at 03:40:41PM +0300, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko> > 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 > --- > drivers/iommu/ipmmu-vmsa.c | 4 > 1 file changed, 4 insertions(+) Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu