Re: [PATCH v2 1/3] riscv: ptdump: Create ptdump.h and move declarations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The refactoring to make ptdump reusable by other components (like KVM) is incomplete. The newly added fields `bits` and `num` in `struct ptdump_pg_level` are dead code, and `note_page()` hardcodes access to file-local static arrays.
- [Low] The function `note_page` is exported globally with a highly generic name, polluting the global namespace.
--

--- Patch [1]: [PATCH v2 1/3] riscv: ptdump: Create ptdump.h and move declarations ---
commit 7ec3e13f790f5980858ebc9e23a4405735f7e6cf
Author: Dylan.Wu <fredwudi0305@gmail.com>

riscv: ptdump: Create ptdump.h and move declarations

Create a new arch/riscv/include/asm/ptdump.h header file and move the
pagetable walking state structures and level definitions there. This
allows other parts of the kernel (like KVM) to reuse the ptdump data
structures.

Also export the note_page() symbol so it can be used by other kernel
components.

> diff --git a/arch/riscv/include/asm/ptdump.h b/arch/riscv/include/asm/ptdump.h
> new file mode 100644
> index 0000000000000..eb9d11dab6098
> --- /dev/null
> +++ b/arch/riscv/include/asm/ptdump.h
> @@ -0,0 +1,41 @@
[ ... ]
> +struct ptdump_pg_level {
> +	const struct ptdump_prot_bits *bits;
> +	const char *name;
> +	u64 mask;
> +	int num;
> +};

[Severity: Medium]
Are the newly added fields bits and num essentially dead code?

The commit message indicates this refactoring allows other parts of the kernel
(like KVM) to reuse the ptdump data structures. However, these new fields
appear to be unused.

For instance, dump_prot() in arch/riscv/mm/ptdump.c still unconditionally
iterates over the file-local pte_bits array rather than utilizing the
caller-provided bits. Additionally, note_page() continues to hardcode access
to the file-local pg_level array.

Does this incomplete refactoring prevent external components from dumping
page tables with custom layouts?

[ ... ]
> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> index f4b4a9fcbbd88..63655a9c8df3e 100644
> --- a/arch/riscv/mm/ptdump.c
> +++ b/arch/riscv/mm/ptdump.c
[ ... ]
> -static void note_page(struct ptdump_state *pt_st, unsigned long addr,
> -		      int level, u64 val)
> +void note_page(struct ptdump_state *pt_st, unsigned long addr,
> +	       int level, u64 val)
>  {
[ ... ]
> @@ -317,6 +280,7 @@ static void note_page(struct ptdump_state *pt_st, unsigned long addr,
>  		st->last_pa = pa;
>  	}
>  }
> +EXPORT_SYMBOL_GPL(note_page);

[Severity: Low]
Does exporting this generically named function pollute the global kernel symbol
table?

Using a generic name like note_page() (instead of something specific like
riscv_ptdump_note_page) could increase the risk of linker collisions if another
subsystem uses the same name.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727123013.118984-1-fredwudi0305@gmail.com?part=1