| Message ID | 1409781429-27593-4-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kees, On Wed, Sep 03, 2014 at 10:57:04PM +0100, Kees Cook wrote: > This is used from set_fixmap() and clear_fixmap() via asm-generic/fixmap.h. > Also makes sure that the fixmap allocation fits into the expected range. > > Based on patch by Rabin Vincent. [...] > +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) > +{ > + unsigned long vaddr = __fix_to_virt(idx); > + pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr); > + > + /* Make sure fixmap region does not exceed available allocation. */ > + BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) > > + FIXADDR_END); > + BUG_ON(idx >= __end_of_fixed_addresses); > + > + if (pgprot_val(prot)) > + set_pte_at(NULL, vaddr, pte, > + pfn_pte(phys >> PAGE_SHIFT, prot)); > + else > + pte_clear(NULL, vaddr, pte); > + > + /* > + * Given the potential a15 tlbi errata, we can only do tlb flushes > + * with interrupts disabled. Callers must have taken care of this. > + */ > + WARN_ON_ONCE(!irqs_disabled()); > + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); Aha, this explains why we were confusing each other! The issue is that interrupts must be *enabled*, so this code does the exact opposite of what we need. I think this got lost in a sea of double negatives during the last round of review. Will
On Thu, Sep 4, 2014 at 10:03 AM, Will Deacon <will.deacon@arm.com> wrote: > Hi Kees, > > On Wed, Sep 03, 2014 at 10:57:04PM +0100, Kees Cook wrote: >> This is used from set_fixmap() and clear_fixmap() via asm-generic/fixmap.h. >> Also makes sure that the fixmap allocation fits into the expected range. >> >> Based on patch by Rabin Vincent. > > [...] > >> +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) >> +{ >> + unsigned long vaddr = __fix_to_virt(idx); >> + pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr); >> + >> + /* Make sure fixmap region does not exceed available allocation. */ >> + BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) > >> + FIXADDR_END); >> + BUG_ON(idx >= __end_of_fixed_addresses); >> + >> + if (pgprot_val(prot)) >> + set_pte_at(NULL, vaddr, pte, >> + pfn_pte(phys >> PAGE_SHIFT, prot)); >> + else >> + pte_clear(NULL, vaddr, pte); >> + >> + /* >> + * Given the potential a15 tlbi errata, we can only do tlb flushes >> + * with interrupts disabled. Callers must have taken care of this. >> + */ >> + WARN_ON_ONCE(!irqs_disabled()); >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); > > Aha, this explains why we were confusing each other! The issue is that > interrupts must be *enabled*, so this code does the exact opposite of > what we need. > > I think this got lost in a sea of double negatives during the last round > of review. Ah! If this is the case, perhaps we can get away with local_flush_tlb_kernel_range() then? -Kees
On Thu, Sep 04, 2014 at 06:23:42PM +0100, Kees Cook wrote: > On Thu, Sep 4, 2014 at 10:03 AM, Will Deacon <will.deacon@arm.com> wrote: > > Hi Kees, > > > > On Wed, Sep 03, 2014 at 10:57:04PM +0100, Kees Cook wrote: > >> This is used from set_fixmap() and clear_fixmap() via asm-generic/fixmap.h. > >> Also makes sure that the fixmap allocation fits into the expected range. > >> > >> Based on patch by Rabin Vincent. > > > > [...] > > > >> +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) > >> +{ > >> + unsigned long vaddr = __fix_to_virt(idx); > >> + pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr); > >> + > >> + /* Make sure fixmap region does not exceed available allocation. */ > >> + BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) > > >> + FIXADDR_END); > >> + BUG_ON(idx >= __end_of_fixed_addresses); > >> + > >> + if (pgprot_val(prot)) > >> + set_pte_at(NULL, vaddr, pte, > >> + pfn_pte(phys >> PAGE_SHIFT, prot)); > >> + else > >> + pte_clear(NULL, vaddr, pte); > >> + > >> + /* > >> + * Given the potential a15 tlbi errata, we can only do tlb flushes > >> + * with interrupts disabled. Callers must have taken care of this. > >> + */ > >> + WARN_ON_ONCE(!irqs_disabled()); > >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); > > > > Aha, this explains why we were confusing each other! The issue is that > > interrupts must be *enabled*, so this code does the exact opposite of > > what we need. > > > > I think this got lost in a sea of double negatives during the last round > > of review. > > Ah! If this is the case, perhaps we can get away with > local_flush_tlb_kernel_range() then? That's a bit tricky, since you need to ensure that preemption is disabled until the mapping is put back like it was. Will
On Thu, Sep 4, 2014 at 10:27 AM, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Sep 04, 2014 at 06:23:42PM +0100, Kees Cook wrote: >> On Thu, Sep 4, 2014 at 10:03 AM, Will Deacon <will.deacon@arm.com> wrote: >> > Hi Kees, >> > >> > On Wed, Sep 03, 2014 at 10:57:04PM +0100, Kees Cook wrote: >> >> This is used from set_fixmap() and clear_fixmap() via asm-generic/fixmap.h. >> >> Also makes sure that the fixmap allocation fits into the expected range. >> >> >> >> Based on patch by Rabin Vincent. >> > >> > [...] >> > >> >> +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) >> >> +{ >> >> + unsigned long vaddr = __fix_to_virt(idx); >> >> + pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr); >> >> + >> >> + /* Make sure fixmap region does not exceed available allocation. */ >> >> + BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) > >> >> + FIXADDR_END); >> >> + BUG_ON(idx >= __end_of_fixed_addresses); >> >> + >> >> + if (pgprot_val(prot)) >> >> + set_pte_at(NULL, vaddr, pte, >> >> + pfn_pte(phys >> PAGE_SHIFT, prot)); >> >> + else >> >> + pte_clear(NULL, vaddr, pte); >> >> + >> >> + /* >> >> + * Given the potential a15 tlbi errata, we can only do tlb flushes >> >> + * with interrupts disabled. Callers must have taken care of this. >> >> + */ >> >> + WARN_ON_ONCE(!irqs_disabled()); >> >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); >> > >> > Aha, this explains why we were confusing each other! The issue is that >> > interrupts must be *enabled*, so this code does the exact opposite of >> > what we need. >> > >> > I think this got lost in a sea of double negatives during the last round >> > of review. >> >> Ah! If this is the case, perhaps we can get away with >> local_flush_tlb_kernel_range() then? > > That's a bit tricky, since you need to ensure that preemption is disabled > until the mapping is put back like it was. Even with CONFIG_ARM_ERRATA_798181 enabled, I don't see a problem here using flush_tlb_kernel_range(). When doing the ftrace work, this was trivial to trip over, so I think we're in a good state here. I've tested this on real hardware now too, and nothing falls over. I'll get rid of the comment and the WARN_ON_ONCE, but AFAICT, this is safe. Do you have a suggestion about what needs fixing? -Kees
On Fri, Sep 05, 2014 at 08:41:08PM +0100, Kees Cook wrote: > On Thu, Sep 4, 2014 at 10:27 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Thu, Sep 04, 2014 at 06:23:42PM +0100, Kees Cook wrote: > >> On Thu, Sep 4, 2014 at 10:03 AM, Will Deacon <will.deacon@arm.com> wrote: > >> > Hi Kees, > >> > > >> > On Wed, Sep 03, 2014 at 10:57:04PM +0100, Kees Cook wrote: > >> >> This is used from set_fixmap() and clear_fixmap() via asm-generic/fixmap.h. > >> >> Also makes sure that the fixmap allocation fits into the expected range. > >> >> > >> >> Based on patch by Rabin Vincent. > >> > > >> > [...] > >> > > >> >> +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) > >> >> +{ > >> >> + unsigned long vaddr = __fix_to_virt(idx); > >> >> + pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr); > >> >> + > >> >> + /* Make sure fixmap region does not exceed available allocation. */ > >> >> + BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) > > >> >> + FIXADDR_END); > >> >> + BUG_ON(idx >= __end_of_fixed_addresses); > >> >> + > >> >> + if (pgprot_val(prot)) > >> >> + set_pte_at(NULL, vaddr, pte, > >> >> + pfn_pte(phys >> PAGE_SHIFT, prot)); > >> >> + else > >> >> + pte_clear(NULL, vaddr, pte); > >> >> + > >> >> + /* > >> >> + * Given the potential a15 tlbi errata, we can only do tlb flushes > >> >> + * with interrupts disabled. Callers must have taken care of this. > >> >> + */ > >> >> + WARN_ON_ONCE(!irqs_disabled()); > >> >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); > >> > > >> > Aha, this explains why we were confusing each other! The issue is that > >> > interrupts must be *enabled*, so this code does the exact opposite of > >> > what we need. > >> > > >> > I think this got lost in a sea of double negatives during the last round > >> > of review. > >> > >> Ah! If this is the case, perhaps we can get away with > >> local_flush_tlb_kernel_range() then? > > > > That's a bit tricky, since you need to ensure that preemption is disabled > > until the mapping is put back like it was. > > Even with CONFIG_ARM_ERRATA_798181 enabled, I don't see a problem here > using flush_tlb_kernel_range(). When doing the ftrace work, this was > trivial to trip over, so I think we're in a good state here. > > I've tested this on real hardware now too, and nothing falls over. > I'll get rid of the comment and the WARN_ON_ONCE, but AFAICT, this is > safe. But was that hardware actually affected by the erratum? There is a runtime check which will avoid the cross-call if not. > Do you have a suggestion about what needs fixing? The easiest thing to do would be ensuring that __set_fixmap is always called with interrupts enabled. If you can guarantee that, then you need to rework the locking so that interrupts aren't disabled. I admit that I've lost a fair amount of the context here, but that's the fundamental issue. Will
On Mon, Sep 8, 2014 at 3:39 AM, Will Deacon <will.deacon@arm.com> wrote: > On Fri, Sep 05, 2014 at 08:41:08PM +0100, Kees Cook wrote: >> On Thu, Sep 4, 2014 at 10:27 AM, Will Deacon <will.deacon@arm.com> wrote: >> > On Thu, Sep 04, 2014 at 06:23:42PM +0100, Kees Cook wrote: >> >> On Thu, Sep 4, 2014 at 10:03 AM, Will Deacon <will.deacon@arm.com> wrote: >> >> > Hi Kees, >> >> > >> >> > On Wed, Sep 03, 2014 at 10:57:04PM +0100, Kees Cook wrote: >> >> >> This is used from set_fixmap() and clear_fixmap() via asm-generic/fixmap.h. >> >> >> Also makes sure that the fixmap allocation fits into the expected range. >> >> >> >> >> >> Based on patch by Rabin Vincent. >> >> > >> >> > [...] >> >> > >> >> >> +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) >> >> >> +{ >> >> >> + unsigned long vaddr = __fix_to_virt(idx); >> >> >> + pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr); >> >> >> + >> >> >> + /* Make sure fixmap region does not exceed available allocation. */ >> >> >> + BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) > >> >> >> + FIXADDR_END); >> >> >> + BUG_ON(idx >= __end_of_fixed_addresses); >> >> >> + >> >> >> + if (pgprot_val(prot)) >> >> >> + set_pte_at(NULL, vaddr, pte, >> >> >> + pfn_pte(phys >> PAGE_SHIFT, prot)); >> >> >> + else >> >> >> + pte_clear(NULL, vaddr, pte); >> >> >> + >> >> >> + /* >> >> >> + * Given the potential a15 tlbi errata, we can only do tlb flushes >> >> >> + * with interrupts disabled. Callers must have taken care of this. >> >> >> + */ >> >> >> + WARN_ON_ONCE(!irqs_disabled()); >> >> >> + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); >> >> > >> >> > Aha, this explains why we were confusing each other! The issue is that >> >> > interrupts must be *enabled*, so this code does the exact opposite of >> >> > what we need. >> >> > >> >> > I think this got lost in a sea of double negatives during the last round >> >> > of review. >> >> >> >> Ah! If this is the case, perhaps we can get away with >> >> local_flush_tlb_kernel_range() then? >> > >> > That's a bit tricky, since you need to ensure that preemption is disabled >> > until the mapping is put back like it was. >> >> Even with CONFIG_ARM_ERRATA_798181 enabled, I don't see a problem here >> using flush_tlb_kernel_range(). When doing the ftrace work, this was >> trivial to trip over, so I think we're in a good state here. >> >> I've tested this on real hardware now too, and nothing falls over. >> I'll get rid of the comment and the WARN_ON_ONCE, but AFAICT, this is >> safe. > > But was that hardware actually affected by the erratum? There is a runtime > check which will avoid the cross-call if not. > >> Do you have a suggestion about what needs fixing? > > The easiest thing to do would be ensuring that __set_fixmap is always called > with interrupts enabled. If you can guarantee that, then you need to rework > the locking so that interrupts aren't disabled. I admit that I've lost a > fair amount of the context here, but that's the fundamental issue. In retesting, it hit the problem. Ugh. I will investigate what can be done. Thanks, -Kees
diff --git a/arch/arm/include/asm/fixmap.h b/arch/arm/include/asm/fixmap.h index d984ca69ce19..714606f70425 100644 --- a/arch/arm/include/asm/fixmap.h +++ b/arch/arm/include/asm/fixmap.h @@ -14,6 +14,8 @@ enum fixed_addresses { __end_of_fixed_addresses }; +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); + #include <asm-generic/fixmap.h> #endif diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 7fa0966cd15f..fa3a667852cb 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -22,6 +22,7 @@ #include <asm/cputype.h> #include <asm/sections.h> #include <asm/cachetype.h> +#include <asm/fixmap.h> #include <asm/sections.h> #include <asm/setup.h> #include <asm/smp_plat.h> @@ -392,6 +393,30 @@ SET_MEMORY_FN(rw, pte_set_rw) SET_MEMORY_FN(x, pte_set_x) SET_MEMORY_FN(nx, pte_set_nx) +void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) +{ + unsigned long vaddr = __fix_to_virt(idx); + pte_t *pte = pte_offset_kernel(pmd_off_k(vaddr), vaddr); + + /* Make sure fixmap region does not exceed available allocation. */ + BUILD_BUG_ON(FIXADDR_START + (__end_of_fixed_addresses * PAGE_SIZE) > + FIXADDR_END); + BUG_ON(idx >= __end_of_fixed_addresses); + + if (pgprot_val(prot)) + set_pte_at(NULL, vaddr, pte, + pfn_pte(phys >> PAGE_SHIFT, prot)); + else + pte_clear(NULL, vaddr, pte); + + /* + * Given the potential a15 tlbi errata, we can only do tlb flushes + * with interrupts disabled. Callers must have taken care of this. + */ + WARN_ON_ONCE(!irqs_disabled()); + flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE); +} + /* * Adjust the PMD section entries according to the CPU in use. */