From: Mark Rutland <mark.rutland@arm.com> To: Will Deacon <will.deacon@arm.com>, catalin.marinas@arm.com Cc: linux-arm-kernel@lists.infradead.org, stable@vger.kernel.org Subject: Re: [PATCH] arm64: mm: ensure that the zero page is visible to the page table walker Date: Fri, 11 Dec 2015 18:19:52 +0000 [thread overview] Message-ID: <20151211181952.GB23638@leverpostej> (raw) In-Reply-To: <20151211175849.GM18828@arm.com> > > > + /* 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); > > Either we need a barrier after every alloc, or we fold the barrier into > > the two allocation functions. > > Could you roll this into your patch that drops the size parameter from > the alloc functions please? Then we can name them {early,late}_alloc_pgtable > and have them do the dsb in there. Maybe we can drop it again when we're > doing proper break-before-make. Sure, will do. Thanks, Mark.
WARNING: multiple messages have this Message-ID (diff)
From: mark.rutland@arm.com (Mark Rutland) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] arm64: mm: ensure that the zero page is visible to the page table walker Date: Fri, 11 Dec 2015 18:19:52 +0000 [thread overview] Message-ID: <20151211181952.GB23638@leverpostej> (raw) In-Reply-To: <20151211175849.GM18828@arm.com> > > > + /* 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); > > Either we need a barrier after every alloc, or we fold the barrier into > > the two allocation functions. > > Could you roll this into your patch that drops the size parameter from > the alloc functions please? Then we can name them {early,late}_alloc_pgtable > and have them do the dsb in there. Maybe we can drop it again when we're > doing proper break-before-make. Sure, will do. Thanks, Mark.
next prev parent reply other threads:[~2015-12-11 18:20 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-10 17:39 [PATCH] arm64: mm: ensure that the zero page is visible to the page table walker Will Deacon 2015-12-10 17:39 ` Will Deacon 2015-12-10 18:14 ` Mark Rutland 2015-12-10 18:14 ` Mark Rutland 2015-12-11 17:58 ` Will Deacon 2015-12-11 17:58 ` Will Deacon 2015-12-11 18:19 ` Mark Rutland [this message] 2015-12-11 18:19 ` Mark Rutland 2015-12-11 19:10 ` Will Deacon 2015-12-11 19:10 ` Will Deacon 2015-12-11 19:16 ` Mark Rutland 2015-12-11 19:16 ` Mark Rutland 2015-12-14 11:40 ` [PATCH 1/2] arm64: mm: specialise pagetable allocators Mark Rutland 2015-12-14 11:40 ` [PATCH 2/2] arm64: mm: ensure visbility of page table zeroing Mark Rutland
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151211181952.GB23638@leverpostej \ --to=mark.rutland@arm.com \ --cc=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=stable@vger.kernel.org \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.