linux-kernel - Re: [PATCH v14 21/22] arm64: mm: Convert mm/dump.c to use walk_page

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <40956d62-241c-6685-72f1-bfc01183141e@arm.com>
Date:   Thu, 31 Oct 2019 13:32:34 +0000
From:   Steven Price <steven.price@....com>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     Mark Rutland <Mark.Rutland@....com>, x86@...nel.org,
        Arnd Bergmann <arnd@...db.de>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Jérôme Glisse <jglisse@...hat.com>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Andy Lutomirski <luto@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        James Morse <james.morse@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-arm-kernel@...ts.infradead.org,
        "Liang, Kan" <kan.liang@...ux.intel.com>
Subject: Re: [PATCH v14 21/22] arm64: mm: Convert mm/dump.c to use
 walk_page_range()

On 30/10/2019 16:45, Catalin Marinas wrote:
> On Mon, Oct 28, 2019 at 01:59:09PM +0000, Steven Price wrote:
>> diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
>> index 93f9f77582ae..9d9b740a86d2 100644
>> --- a/arch/arm64/mm/dump.c
>> +++ b/arch/arm64/mm/dump.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/io.h>
>>  #include <linux/init.h>
>>  #include <linux/mm.h>
>> +#include <linux/ptdump.h>
>>  #include <linux/sched.h>
>>  #include <linux/seq_file.h>
>>  
>> @@ -75,10 +76,11 @@ static struct addr_marker address_markers[] = {
>>   * dumps out a description of the range.
>>   */
>>  struct pg_state {
>> +	struct ptdump_state ptdump;
>>  	struct seq_file *seq;
>>  	const struct addr_marker *marker;
>>  	unsigned long start_address;
>> -	unsigned level;
>> +	int level;
>>  	u64 current_prot;
>>  	bool check_wx;
>>  	unsigned long wx_pages;
>> @@ -178,6 +180,10 @@ static struct pg_level pg_level[] = {
>>  		.name	= "PGD",
>>  		.bits	= pte_bits,
>>  		.num	= ARRAY_SIZE(pte_bits),
>> +	}, { /* p4d */
>> +		.name	= "P4D",
>> +		.bits	= pte_bits,
>> +		.num	= ARRAY_SIZE(pte_bits),
>>  	}, { /* pud */
>>  		.name	= (CONFIG_PGTABLE_LEVELS > 3) ? "PUD" : "PGD",
>>  		.bits	= pte_bits,
> 
> We could use "PGD" for the p4d entry since we don't have five levels.
> This patches the "PGD" name used for pud/pmd when these levels are
> folded.

Good point, although I'd actually be more tempted to do the opposite -
remove the special casing for PUD/PMD as the generic code should now
never provide those levels if they are folded. What do you think?

>> @@ -240,11 +246,15 @@ static void note_prot_wx(struct pg_state *st, unsigned long addr)
>>  	st->wx_pages += (addr - st->start_address) / PAGE_SIZE;
>>  }
>>  
>> -static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
>> -				u64 val)
>> +static void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>> +		      unsigned long val)
>>  {
>> +	struct pg_state *st = container_of(pt_st, struct pg_state, ptdump);
>>  	static const char units[] = "KMGTPE";
>> -	u64 prot = val & pg_level[level].mask;
>> +	u64 prot = 0;
>> +
>> +	if (level >= 0)
>> +		prot = val & pg_level[level].mask;
> 
> I think this test is not needed as we never have level < 0. The only
> call with a level 0 is from ptdump_hole() where the level passed is
> depth+1 while depth is -1 or higher.

Yes, sorry - that was needed in a previous version of the series, but I
apparently forgot to remove it.

> Anyway, we can keep this test _if_ we shift the levels down. I find it
> quite confusing that ptdump_hole() takes a 'depth' argument where 0 is
> PGD and 4 is PTE while for note_page() 1 is PGD and 5 PTE.
> 

Yes - I'll send a follow up patch which shifts the levels down. I
originally picked the levels to match the existing dump implementations
on x86/arm64. But it appears that x86 has had its levels artificially
inflated in the past, so actually moving it back down and matching
'depth' should make the code slightly more simple.

Steve