On Fri, Dec 11, 2015 at 07:10:31PM +0000, Will Deacon wrote:
> On Fri, Dec 11, 2015 at 06:19:52PM +0000, Mark Rutland wrote:
> > > > > + /* Ensure the zero page is visible to the page table walker */
> > > > > + dsb(ishst);
> > > >
> > > > I think this should live in early_alloc (likewise in late_alloc).
> > > >
> > > > In the other cases we call early_alloc or late_allot we assume the
> > > > zeroing is visible to the page table walker.
> > > >
> > > > For example in in alloc_init_pte we do:
> > > >
> > > > if (pmd_none(*pmd) || pmd_sect(*pmd)) {
> > > > pte = alloc(PTRS_PER_PTE * sizeof(pte_t));
> > > > if (pmd_sect(*pmd))
> > > > split_pmd(pmd, pte);
> > > > __pmd_populate(pmd, __pa(pte), PMD_TYPE_TABLE);
> > > > flush_tlb_all();
> > > > }
> > > >
> > > > There's a dsb in __pmd_populate, but it's _after_ the write to the pmd
> > > > entry, so the walker might start walking the newly-allocated pte table
> > > > before the zeroing is visible.
> > >
> > > Urgh. The reason this is a problem is because we're modifying the page
> > > tables live (which I know that you're fixing) without using
> > > break-before-make. Consequently, the usual ordering guarantees that we
> > > get from the tlb flush after installing the invalid entry do not apply
> > > and we end up with the issue you point out.
> >
> > My feeling was that in these paths we usually assume all prior page
> > table updates have been made visible to the page table walkers. Given
> > that, having the allocator guarantee the zeroing was already visible
> > felt like the natural thing to do.
> >
> > That said, having looked at mm/memory.c, we seem to follow the exact
> > same pattern when plumbing tables together dynamically, with only an
> > smp_wmb() between the zeroed allocation and plumbing a table entry in.
> >
> > e.g. in __pte_alloc we have the pattern:
> >
> > pgtable_t new = pte_alloc_one(mm, address);
> > smp_wmb();
> > if (pmd_none(*pmd))
> > pmd_populate(mm, pmd, new);
>
> I suspect this is potentially broken if somebody builds a CPU with a
> "cool feature" in the page table walker that allows it to walk out of
> order without respecting address dependencies.
>
> The easiest fix is adding dsb(ishst) to the page table alloc functions.
Sounds good to me.
Mark.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html