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: <20251019140711.63664-1-hi@josie.lol>
Date: Sun, 19 Oct 2025 16:07:11 +0200
From: Josephine Pfeiffer <hi@...ie.lol>
To: pjw@...nel.org
Cc: 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 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 the review!