linux-kernel - Re: [PATCH V2] mm/ptdump: Drop GENERIC

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: <d5a2b423-2e13-48a1-b3b7-8e1c638bf716@arm.com>
Date: Thu, 6 Feb 2025 09:51:45 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Mark Rutland <mark.rutland@....com>
Cc: linux-mm@...ck.org, steven.price@....com, christophe.leroy@...roup.eu,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Jonathan Corbet <corbet@....net>, Marc Zyngier <maz@...nel.org>,
 Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Heiko Carstens <hca@...ux.ibm.com>,
 Vasily Gorbik <gor@...ux.ibm.com>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>,
 linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev,
 linuxppc-dev@...ts.ozlabs.org, linux-riscv@...ts.infradead.org,
 linux-s390@...r.kernel.org
Subject: Re: [PATCH V2] mm/ptdump: Drop GENERIC_PTDUMP



On 2/5/25 15:31, Mark Rutland wrote:
> On Wed, Feb 05, 2025 at 10:30:39AM +0530, Anshuman Khandual wrote:
>> GENERIC_PTDUMP does not guard any code but instead just used for platform's
>> subscription into core ptdump defined under PTDUMP_CORE, which is selected.
> 
> Selected by what?

I guess that sentence was incomplete. PTDUMP_CORE gets selected by all
the real users which requires core PTDUMP to be built and enabled.

arch/arm64/kvm/Kconfig: select PTDUMP_CORE	/* config PTDUMP_STAGE2_DEBUGFS */
arch/x86/Kconfig.debug: select PTDUMP_CORE	/* config EFI_PGT_DUMP */

mm/Kconfig.debug:       select PTDUMP_CORE	/* config PTDUMP_DEBUGFS */
mm/Kconfig.debug:       select PTDUMP_CORE	/* config DEBUG_WX */

> 
>> Instead use PTDUMP_CORE for platform subscription and drop GENERIC_PTDUMP.
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Jonathan Corbet <corbet@....net>
>> Cc: Marc Zyngier <maz@...nel.org>
>> Cc: Michael Ellerman <mpe@...erman.id.au>
>> Cc: Nicholas Piggin <npiggin@...il.com>
>> Cc: Paul Walmsley <paul.walmsley@...ive.com>
>> Cc: Palmer Dabbelt <palmer@...belt.com>
>> Cc: Heiko Carstens <hca@...ux.ibm.com>
>> Cc: Vasily Gorbik <gor@...ux.ibm.com>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-doc@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: kvmarm@...ts.linux.dev
>> Cc: linuxppc-dev@...ts.ozlabs.org
>> Cc: linux-riscv@...ts.infradead.org
>> Cc: linux-s390@...r.kernel.org
>> Cc: linux-mm@...ck.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>> This patch applies on v6.14-rc1
>>
>> Changes in V2:
>>
>> - Keep arch/powerpc/Kconfig alphabetically sorted per Christophe
>>
>> Changes in V1:
>>
>> https://lore.kernel.org/all/20241217034807.2541349-1-anshuman.khandual@arm.com/
>>
>>  Documentation/arch/arm64/ptdump.rst       | 1 -
>>  arch/arm64/Kconfig                        | 2 +-
>>  arch/arm64/kvm/Kconfig                    | 3 +--
>>  arch/powerpc/Kconfig                      | 2 +-
>>  arch/powerpc/configs/mpc885_ads_defconfig | 1 -
>>  arch/riscv/Kconfig                        | 2 +-
>>  arch/s390/Kconfig                         | 2 +-
>>  arch/x86/Kconfig                          | 2 +-
>>  arch/x86/Kconfig.debug                    | 2 +-
>>  kernel/configs/debug.config               | 1 -
>>  mm/Kconfig.debug                          | 8 ++------
>>  11 files changed, 9 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/arch/arm64/ptdump.rst b/Documentation/arch/arm64/ptdump.rst
>> index 5dcfc5d7cddf..61ca040a885b 100644
>> --- a/Documentation/arch/arm64/ptdump.rst
>> +++ b/Documentation/arch/arm64/ptdump.rst
>> @@ -22,7 +22,6 @@ offlining of memory being accessed by the ptdump code.
>>  In order to dump the kernel page tables, enable the following
>>  configurations and mount debugfs::
>>  
>> - CONFIG_GENERIC_PTDUMP=y
>>   CONFIG_PTDUMP_CORE=y
>>   CONFIG_PTDUMP_DEBUGFS=y
>>  
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index fcdd0ed3eca8..1f516bed81dd 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -157,7 +157,7 @@ config ARM64
>>  	select GENERIC_IRQ_SHOW_LEVEL
>>  	select GENERIC_LIB_DEVMEM_IS_ALLOWED
>>  	select GENERIC_PCI_IOMAP
>> -	select GENERIC_PTDUMP
>> +	select PTDUMP_CORE
>>  	select GENERIC_SCHED_CLOCK
>>  	select GENERIC_SMP_IDLE_THREAD
>>  	select GENERIC_TIME_VSYSCALL
> 
> This change means that the ptdump core code will be built regardless of
> whether any users are selected:
> 
>   [mark@...rids:~/src/linux]% git grep CONFIG_PTDUMP_CORE
>   Documentation/arch/arm64/ptdump.rst: CONFIG_PTDUMP_CORE=y
>   arch/arm64/include/asm/ptdump.h:#ifdef CONFIG_PTDUMP_CORE
>   arch/arm64/include/asm/ptdump.h:#endif /* CONFIG_PTDUMP_CORE */
>   arch/arm64/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE)        += ptdump.o
>   arch/powerpc/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE)      += ptdump/
>   arch/riscv/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE) += ptdump.o
>   arch/s390/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE) += dump_pagetables.o
>   arch/x86/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE)  += dump_pagetables.o
>   mm/Makefile:obj-$(CONFIG_PTDUMP_CORE) += ptdump.o
> 
> GENERIC_PTDUMP means "this architecture uses generic ptdump code for
> ptdump", i.e. the architecture implements all the necessary bits for
> that to work *IF* it is built.
> 
> PTDUMP_CORE means "actually build the core ptdump code".
> 
> If everyone uses the generic ptdump code now, maybe it's worth renaming
> GENERIC_PTDUMP to ARCH_HAS_PTDUMP or something like that, but I don't
> think this change makes sense as-is.

Seems like a combination of ARCH_HAS_CORE_PTDUMP for subscription and
CORE_PTDUMP for actual enablement will make things clear. _CORE_ here
would still let platforms define their own CONFIG_PTDUMP if preferred
and not subscribe the generic CORE_DUMP.

currently GENERIC_PTDUMP and CORE_PTDUMP just seems disjoint, because
GENERIC_PTDUMP does not not appear to be the platform opt in required
for CORE_PTDUMP.

There is another problem.

mm/Kconfig.debug

config DEBUG_WX
        bool "Warn on W+X mappings at boot"
        depends on ARCH_HAS_DEBUG_WX
        depends on MMU
        select PTDUMP_CORE
        help

Before selecting PTDUMP_CORE it does not ensure platform has opted in
via GENERIC_PTDUMP or not. Instead it should be

config DEBUG_WX
        bool "Warn on W+X mappings at boot"
        depends on ARCH_HAS_DEBUG_WX
	depends on GENERIC_PTDUMP
	depends on 
        depends on MMU
        select PTDUMP_CORE
        help

PTDUMP_STAGE2_DEBUGFS on arm64 does that where as EFI_PGT_DUMP on x86
does not. Although it will be ideal but that's not a problem if the
platform knows for sure that it opts in GENERIC_PTDUMP.

> 
> [...]
> 
>> diff --git a/kernel/configs/debug.config b/kernel/configs/debug.config
>> index 20552f163930..8aafd050b754 100644
>> --- a/kernel/configs/debug.config
>> +++ b/kernel/configs/debug.config
>> @@ -73,7 +73,6 @@ CONFIG_DEBUG_VM=y
>>  CONFIG_DEBUG_VM_PGFLAGS=y
>>  CONFIG_DEBUG_VM_RB=y
>>  CONFIG_DEBUG_VM_VMACACHE=y
>> -CONFIG_GENERIC_PTDUMP=y
>>  CONFIG_KASAN=y
>>  CONFIG_KASAN_GENERIC=y
>>  CONFIG_KASAN_INLINE=y
> 
> I think this is wrong today, and removing it is the right thing to do.
> 
> Architectures with support will select this themselves, and on
> architectures without support this either does nothing or causes a build
> failure.

Alright, will separate this change out first.