linux-kernel - Re: [PATCH 1/2] arm64/mm/hotplug: Drop redundant [pgd|p4d]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1c35a8c5-324b-4c60-9bf1-a5414481e5ad@arm.com>
Date: Wed, 19 Mar 2025 11:32:25 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Mark Rutland <mark.rutland@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Ard Biesheuvel <ardb@...nel.org>, Ryan Roberts <ryan.roberts@....com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] arm64/mm/hotplug: Drop redundant [pgd|p4d]_present()

On 3/18/25 16:46, Mark Rutland wrote:
> On Fri, Feb 21, 2025 at 03:14:48PM +0530, Anshuman Khandual wrote:
>> [pgd|p4d]_present() are inverse to their corresponding [pgd|p4d]_none().
> 
> Maybe their implementations happen to be inverse today, but the semantic
> of pXX_present() is not the inverse of the semantic of pXX_none(). In
> general, !pXX_none() does not imply pXX_present().

Agreed.

> 
>> So [pgd|p4d]_present() test right after corresponding [pgd|p4d]_none()
>> inverse test does not make sense. Hence just drop these redundant
>> checks.
> 
> I think the checks make sense in abstract, even if they're redundant
> today

Okay.

> 
> Is there any reason to remove these specific case?

Just for optimization.

> 
> Surely the compiler optimizes these out when they're redundant?

Agreed. Although wondering if p4d_present() is the right thing to test.
Should it be replaced with a p4d_bad() check instead because subsequent
calls into either unmap_hotplug_pud_range() or free_empty_pud_table() 
requires given entry to be a table to proceed further successfully.

> 
> Mark.
> 
>>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Ard Biesheuvel <ardb@...nel.org>
>> Cc: Ryan Roberts <ryan.roberts@....com>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>>  arch/arm64/mm/mmu.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index b4df5bc5b1b8..66906c45c7f6 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -952,7 +952,6 @@ static void unmap_hotplug_p4d_range(pgd_t *pgdp, unsigned long addr,
>>  		if (p4d_none(p4d))
>>  			continue;
>>  
>> -		WARN_ON(!p4d_present(p4d));
>>  		unmap_hotplug_pud_range(p4dp, addr, next, free_mapped, altmap);
>>  	} while (addr = next, addr < end);
>>  }
>> @@ -978,7 +977,6 @@ static void unmap_hotplug_range(unsigned long addr, unsigned long end,
>>  		if (pgd_none(pgd))
>>  			continue;
>>  
>> -		WARN_ON(!pgd_present(pgd));
>>  		unmap_hotplug_p4d_range(pgdp, addr, next, free_mapped, altmap);
>>  	} while (addr = next, addr < end);
>>  }
>> @@ -1114,7 +1112,6 @@ static void free_empty_p4d_table(pgd_t *pgdp, unsigned long addr,
>>  		if (p4d_none(p4d))
>>  			continue;
>>  
>> -		WARN_ON(!p4d_present(p4d));
>>  		free_empty_pud_table(p4dp, addr, next, floor, ceiling);
>>  	} while (addr = next, addr < end);
>>  
>> @@ -1153,7 +1150,6 @@ static void free_empty_tables(unsigned long addr, unsigned long end,
>>  		if (pgd_none(pgd))
>>  			continue;
>>  
>> -		WARN_ON(!pgd_present(pgd));
>>  		free_empty_p4d_table(pgdp, addr, next, floor, ceiling);
>>  	} while (addr = next, addr < end);
>>  }
>> -- 
>> 2.30.2
>>