arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page
diff mbox series

Message ID 1556509914-21138-1-git-send-email-anshuman.khandual@arm.com
State New
Headers show
Series
  • arm64/mm: Fix pgtable page offset address in [pud|pmd]_free_[pmd|pte]_page
Related show

Commit Message

Anshuman Khandual April 29, 2019, 3:51 a.m. UTC
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>
---
 arch/arm64/mm/mmu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Catalin Marinas April 29, 2019, 11:23 a.m. UTC | #1
On Mon, Apr 29, 2019 at 09:21:54AM +0530, Anshuman Khandual wrote:
> 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.

I'm fine with using 0UL but did you actually hit any problem to be worth
adding a WARN_ON?

> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/mm/mmu.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e97f018ff740..17af49585fb2 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1005,7 +1005,8 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>  		return 1;
>  	}
>  
> -	table = pte_offset_kernel(pmdp, addr);
> +	WARN_ON_ONCE(!IS_ALIGNED(addr, PMD_SIZE));

I'd use VM_WARN_ON_ONCE() as we do, for example, in set_pte_at().
Anshuman Khandual April 29, 2019, 2:44 p.m. UTC | #2
On 04/29/2019 04:53 PM, Catalin Marinas wrote:
> On Mon, Apr 29, 2019 at 09:21:54AM +0530, Anshuman Khandual wrote:
>> 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.
> 
> I'm fine with using 0UL but did you actually hit any problem to be worth
> adding a WARN_ON?

No I did not hit any problem. But the higher level function (ioremap_page_range)
from which this gets called does not check the alignment for incoming address
range. As it can be called from outside of core MM like drivers we cannot even
correct the alignment without changing the range which would risk potential
undefined behavior. Hence just warn once and then move on to do what ever has
been asked by the caller.

> 
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/mm/mmu.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index e97f018ff740..17af49585fb2 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1005,7 +1005,8 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>>  		return 1;
>>  	}
>>  
>> -	table = pte_offset_kernel(pmdp, addr);
>> +	WARN_ON_ONCE(!IS_ALIGNED(addr, PMD_SIZE));
> 
> I'd use VM_WARN_ON_ONCE() as we do, for example, in set_pte_at().

Did you mean VM_WARN_ONCE() instead which is in set_pte_at(). Sure will change it.

Patch
diff mbox series

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index e97f018ff740..17af49585fb2 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1005,7 +1005,8 @@  int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
 		return 1;
 	}
 
-	table = pte_offset_kernel(pmdp, addr);
+	WARN_ON_ONCE(!IS_ALIGNED(addr, PMD_SIZE));
+	table = pte_offset_kernel(pmdp, 0UL);
 	pmd_clear(pmdp);
 	__flush_tlb_kernel_pgtable(addr);
 	pte_free_kernel(NULL, table);
@@ -1026,8 +1027,9 @@  int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
 		return 1;
 	}
 
-	table = pmd_offset(pudp, addr);
-	pmdp = table;
+	WARN_ON_ONCE(!IS_ALIGNED(addr, PUD_SIZE));
+	table = pmd_offset(pudp, 0UL);
+	pmdp = pmd_offset(pudp, addr);
 	next = addr;
 	end = addr + PUD_SIZE;
 	do {