linux-kernel - Re: [PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

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: <20160930001339.GA4369@remoulade>
Date:   Fri, 30 Sep 2016 01:13:40 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Laura Abbott <labbott@...hat.com>
Cc:     AKASHI Takahiro <takahiro.akashi@...aro.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Will Deacon <will.deacon@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

Hi,

On Thu, Sep 29, 2016 at 02:32:55PM -0700, Laura Abbott wrote:
> ptdump_register currently initializes a set of page table information and
> registers debugfs. There are uses for the ptdump option without wanting the
> debugfs options. Split this out to make it a separate option.
> 
> Signed-off-by: Laura Abbott <labbott@...hat.com>
> ---
>  arch/arm64/Kconfig.debug        |  6 +++++-
>  arch/arm64/include/asm/ptdump.h | 15 +++++++++++++--
>  arch/arm64/mm/Makefile          |  3 ++-
>  arch/arm64/mm/dump.c            | 30 +++++++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c  | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+), 25 deletions(-)
>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c

As a heads-up, Ard has new ARM64_PTUMP user under drivers/firmware/efi queued
up in the EFI tree, which will also need fixing up. See commit d80448ac92b72051
("efi/arm64: Add debugfs node to dump UEFI runtime page tables") [1].

[...]

> +#include <linux/seq_file.h>
>  #include <linux/mm_types.h>

Nit: please keep headers in alphabetical order.

> -static void walk_pgd(struct pg_state *st, struct mm_struct *mm,
> +static void __walk_pgd(struct pg_state *st, struct mm_struct *mm,

Can we leave this name as-is? We didn't change walk_{pud,pmd,pte}, so this is
inconsistent, and we haven't reused the name.

[...]

> +int ptdump_register(struct ptdump_info *info, const char *name)
> +{
> +	ptdump_initialize(info);
> +	return ptdump_debugfs_create(info, name);
>  }

It feels like a layering violation to have the core ptdump code call the
debugfs ptdump code. Is there some reason this has to live here?

Other than the above points, this looks good to me.

Thanks,
Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=next&id=9d80448ac92b720512c415265597d349d8b5c3e8