linux-kernel - Re: [PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq

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: <ab17be2e-202e-e34e-21f8-c865aff4024f@kernel.org>
Date: Sat, 25 Oct 2025 01:15:12 -0600 (MDT)
From: Paul Walmsley <pjw@...nel.org>
To: Josephine Pfeiffer <hi@...ie.lol>
cc: pjw@...nel.org, paul.walmsley@...ive.com, palmer@...belt.com, 
    aou@...s.berkeley.edu, alex@...ti.fr, linux-riscv@...ts.infradead.org, 
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] riscv: ptdump: use seq_puts() in pt_dump_seq_puts()
 macro

On Sun, 19 Oct 2025, Josephine Pfeiffer wrote:

> On Sat, 18 Oct 2025, Paul Walmsley wrote:
> 
> > Hard to accept that it's a performance issue.  But I think you're right
> > that generating a newline should be done with seq_puts().
> 
> Fair point. I'll drop that from the commit message.
> 
> > A better fix would seem to be to just get rid of pt_dump_seq_puts().  It's
> > only used once in arch/riscv.
> >
> > Taking a broader view, both pt_dump_seq_puts() and pt_dump_seq_printf()
> > look completely pointless.  Is there any argument for keeping them?
> 
> Good question. I investigated the git history and current usage:
> 
> The macros were introduced in commit ae5d1cf358a5 ("arm64: dump: Make the
> page table dumping seq_file optional") to support passing NULL for the
> seq_file parameter. This is used by ptdump_check_wx() for CONFIG_DEBUG_WX,
> where the kernel walks page tables to check for writable+executable pages
> without outputting anything to userspace.
> 
> All four architectures use this pattern in ptdump_check_wx():
> 
>   arch/arm64/mm/ptdump.c:341:         .seq = NULL,
>   arch/arm/mm/dump.c:456:             .seq = NULL,
>   arch/riscv/mm/ptdump.c:378:         .seq = NULL,
>   arch/s390/mm/dump_pagetables.c:197: .seq = NULL,
> 
> However, you're right that the utility of these macros varies:
> 
> Usage of pt_dump_seq_puts():
> - arm64: 1 use
> - ARM: 0 uses
> - riscv: 1 use
> - s390: 3 uses
> 
> Note: ARM defines pt_dump_seq_puts() but never uses it - that macro
> could be removed entirely.
> 
> Usage of pt_dump_seq_printf():
> - arm64: 6 uses
> - ARM: 7 uses
> - riscv: 6 uses
> - s390: 5 uses
> 
> For RISC-V specifically, I agree the single use of pt_dump_seq_puts()
> could be replaced with an inline conditional. For pt_dump_seq_printf(),
> the macro does save some repetition (6 uses vs 1 macro definition).
> 
> pt_dump_seq_printf() could also be questioned - removing it means 20+
> inline conditionals across all architectures. I focused on the minimal
> fix, but happy to tackle the larger refactor if preferred.
> 
> Would you prefer:
> 
> Option A) Remove pt_dump_seq_puts() entirely from riscv and replace the
> single use with:
>   if (st->seq)
>     seq_puts(st->seq, "\n");
> 
> Option B) Keep the macro for consistency with other architectures, but
> fix the bug
> 
> I'm happy to send a v2 with either approach. If Option A, I could also
> propose similar cleanups for arm64 (1 use) as a follow-up.

Thanks for investigating further.  I just queued your original patch; I 
think that's the simplest way forward.


- Paul