| Message ID | 1556595839-9282-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page | expand |
On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote: > @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr) > return 1; > } > > - table = pmd_offset(pudp, addr); > - pmdp = table; > + VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE), > + "%s: unaligned address 0x%016llx\n", __func__, addr); > + table = pmd_offset(pudp, 0UL); > + pmdp = pmd_offset(pudp, addr); Why does pmdp need to use addr? We are freeing the whole pmd page, so I don't think pmdp should be different from table here.
On 04/30/2019 09:47 PM, Catalin Marinas wrote: > On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote: >> @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr) >> return 1; >> } >> >> - table = pmd_offset(pudp, addr); >> - pmdp = table; >> + VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE), >> + "%s: unaligned address 0x%016llx\n", __func__, addr); >> + table = pmd_offset(pudp, 0UL); >> + pmdp = pmd_offset(pudp, addr); > > Why does pmdp need to use addr? We are freeing the whole pmd page, so I > don't think pmdp should be different from table here. pmd_offset(pudp, addr) and pmd_offset(pudp, 0UL) would evaluate the same if the input addr is PMD_SIZE aligned. The problem just arises when it is not. The Idea is that the wrong input should be allowed to have adverse affect all the way without any corrections. Now because intermediate 'next' and 'end' are derived from wrong input 'addr' in the first place, all 'pmdp' start and intermediate values should just follow without any corrections starting at pmd_offset(pudp, addr). The new warning here just informs about the fact that the iteration range is going to be wrong (as well as everything else probably) because the input address is not aligned. 'table = pmd_offset(pudp, 0UL)' will prevent it from hitting unaligned BUG_ON() in pmd_free(). Using 'pmdp = pmd_offset(pudp, 0UL)' for iterations will be sort of fixing or aligning the wrong input 'addr' which we always wanted to avoid.
On Wed, May 01, 2019 at 10:15:49AM +0530, Anshuman Khandual wrote: > On 04/30/2019 09:47 PM, Catalin Marinas wrote: > > On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote: > >> @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr) > >> return 1; > >> } > >> > >> - table = pmd_offset(pudp, addr); > >> - pmdp = table; > >> + VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE), > >> + "%s: unaligned address 0x%016llx\n", __func__, addr); > >> + table = pmd_offset(pudp, 0UL); > >> + pmdp = pmd_offset(pudp, addr); > > > > Why does pmdp need to use addr? We are freeing the whole pmd page, so I > > don't think pmdp should be different from table here. > > pmd_offset(pudp, addr) and pmd_offset(pudp, 0UL) would evaluate the same if > the input addr is PMD_SIZE aligned. The problem just arises when it is not. > > The Idea is that the wrong input should be allowed to have adverse affect > all the way without any corrections. Now because intermediate 'next' and > 'end' are derived from wrong input 'addr' in the first place, all 'pmdp' > start and intermediate values should just follow without any corrections > starting at pmd_offset(pudp, addr). > > The new warning here just informs about the fact that the iteration range is > going to be wrong (as well as everything else probably) because the input > address is not aligned. 'table = pmd_offset(pudp, 0UL)' will prevent it from > hitting unaligned BUG_ON() in pmd_free(). > > Using 'pmdp = pmd_offset(pudp, 0UL)' for iterations will be sort of fixing > or aligning the wrong input 'addr' which we always wanted to avoid. So you want to hide the BUG_ON in pmd_free() by changing this to a warning in the caller (pud_free_pmd_page()) and sanitising the value passed to pmd_free(). I don't see how this is different from just turning the BUG_ON into a warning in pmd_free() directly (which I don't think we should, see below). It looks to me like we should either fix the callers in ioremap.c (e.g. ioremap_try_huge_pud() returning 0 if addr is not PUD_SIZE aligned, not just phys_addr) or return 0 in pud_free_pmd_page() with a similar check. I'd go for changing ioremap.c since x86 doesn't have such check either. IIUC currently if we pass a PUD_SIZE range to ioremap_page_range() where phys_addr is PUD_SIZE aligned but the virtual addr is not, we'd end up randomly freeing the pmd page that potentially still contains valid mappings. Your patch just hides the problem by turning the BUG_ON into a warning but doesn't solve it.
On 05/01/2019 04:44 PM, Catalin Marinas wrote: > On Wed, May 01, 2019 at 10:15:49AM +0530, Anshuman Khandual wrote: >> On 04/30/2019 09:47 PM, Catalin Marinas wrote: >>> On Tue, Apr 30, 2019 at 09:13:59AM +0530, Anshuman Khandual wrote: >>>> @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr) >>>> return 1; >>>> } >>>> >>>> - table = pmd_offset(pudp, addr); >>>> - pmdp = table; >>>> + VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE), >>>> + "%s: unaligned address 0x%016llx\n", __func__, addr); >>>> + table = pmd_offset(pudp, 0UL); >>>> + pmdp = pmd_offset(pudp, addr); >>> >>> Why does pmdp need to use addr? We are freeing the whole pmd page, so I >>> don't think pmdp should be different from table here. >> >> pmd_offset(pudp, addr) and pmd_offset(pudp, 0UL) would evaluate the same if >> the input addr is PMD_SIZE aligned. The problem just arises when it is not. >> >> The Idea is that the wrong input should be allowed to have adverse affect >> all the way without any corrections. Now because intermediate 'next' and >> 'end' are derived from wrong input 'addr' in the first place, all 'pmdp' >> start and intermediate values should just follow without any corrections >> starting at pmd_offset(pudp, addr). >> >> The new warning here just informs about the fact that the iteration range is >> going to be wrong (as well as everything else probably) because the input >> address is not aligned. 'table = pmd_offset(pudp, 0UL)' will prevent it from >> hitting unaligned BUG_ON() in pmd_free(). >> >> Using 'pmdp = pmd_offset(pudp, 0UL)' for iterations will be sort of fixing >> or aligning the wrong input 'addr' which we always wanted to avoid. > > So you want to hide the BUG_ON in pmd_free() by changing this to a > warning in the caller (pud_free_pmd_page()) and sanitising the value > passed to pmd_free(). I don't see how this is different from just > turning the BUG_ON into a warning in pmd_free() directly (which I don't > think we should, see below). > > It looks to me like we should either fix the callers in ioremap.c (e.g. > ioremap_try_huge_pud() returning 0 if addr is not PUD_SIZE aligned, not > just phys_addr) or return 0 in pud_free_pmd_page() with a similar check. > I'd go for changing ioremap.c since x86 doesn't have such check either. Sounds good. Will change ioremap_try_huge_[pud|pmd] to include alignment checks for 'addr' along with existing 'phys_addr'. But still bit inclined towards having XXX_offset(XXX, 0UL) for fetching the pgtable page address which is different than XXX_offset(XXX, addr) for iteration purpose unless if you have concerns. > > IIUC currently if we pass a PUD_SIZE range to ioremap_page_range() where > phys_addr is PUD_SIZE aligned but the virtual addr is not, we'd end up > randomly freeing the pmd page that potentially still contains valid > mappings. Your patch just hides the problem by turning the BUG_ON into a > warning but doesn't solve it. Right. This patch tried to use XXX_offset(XXX, 0UL) which is a better way fetching the pgtable page address but that does not solve the problem related to unaligned input virtual address. VM_WARN_ONCE() just shifts the problem bit earlier without solving it as you have pointed out.
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index e97f018ff740..9dc1c7e90ef4 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -1005,7 +1005,9 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr) return 1; } - table = pte_offset_kernel(pmdp, addr); + VM_WARN_ONCE(!IS_ALIGNED(addr, PMD_SIZE), + "%s: unaligned address: 0x%016llx\n", __func__, addr); + table = pte_offset_kernel(pmdp, 0UL); pmd_clear(pmdp); __flush_tlb_kernel_pgtable(addr); pte_free_kernel(NULL, table); @@ -1026,8 +1028,10 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr) return 1; } - table = pmd_offset(pudp, addr); - pmdp = table; + VM_WARN_ONCE(!IS_ALIGNED(addr, PUD_SIZE), + "%s: unaligned address 0x%016llx\n", __func__, addr); + table = pmd_offset(pudp, 0UL); + pmdp = pmd_offset(pudp, addr); next = addr; end = addr + PUD_SIZE; do {
The pgtable page address can be fetched with [pmd|pte]_offset_[kernel] if the input address is either PMD_SIZE or PTE_SIZE aligned. Though incoming input addresses tend to be aligned to the required size (PMD_SIZE|PTE_SIZE) which is the reason why there were no reported problems earlier. But it is not correct. However 0UL as input address will guarantee that the fetched pgtable page address is always correct without any possible skid. While at this just warn once when the addresses are not aligned. Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- Changes in V2: - Replaced WARN_ON_ONCE() with VM_WARN_ONCE() as per Catalin arch/arm64/mm/mmu.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)