Re: [PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page

From: Catalin Marinas <catalin.marinas@arm.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: mark.rutland@arm.com, will.deacon@arm.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH V2] arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page
Date: Wed, 1 May 2019 12:14:16 +0100	[thread overview]
Message-ID: <20190501111416.GD58847@arrakis.emea.arm.com> (raw)
In-Reply-To: <4f2d0ad4-7453-1851-4e12-f83a9226a6ef@arm.com>

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.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel