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: <e17189c7-54e0-f0a1-2b16-36f486111c4c@redhat.com>
Date:   Thu, 29 Sep 2016 17:31:09 -0700
From:   Laura Abbott <labbott@...hat.com>
To:     Mark Rutland <mark.rutland@....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

On 09/29/2016 05:13 PM, Mark Rutland wrote:
> 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].
>
> [...]
>

I'll take a look at that, thanks for the pointer!

>> +#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.
>

Yes, I think this is a relic of earlier refactoring attempts.

> [...]
>
>> +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?
>

Which 'this' are you referring to here? Are you suggesting moving
the ptdump_register elsewhere or moving the debugfs create elsewhere?

> 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
>