linux-kernel - Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to

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: <ZbrfcTaiuu2gaa2A@yzhao56-desk.sh.intel.com>
Date: Thu, 1 Feb 2024 08:01:53 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Arnd Bergmann <arnd@...db.de>
CC: Linus Walleij <linus.walleij@...aro.org>, guoren <guoren@...nel.org>,
	Brian Cain <bcain@...cinc.com>, Jonas Bonn <jonas@...thpole.se>, "Stefan
 Kristiansson" <stefan.kristiansson@...nalahti.fi>, Stafford Horne
	<shorne@...il.com>, Linux-Arch <linux-arch@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, "linux-csky@...r.kernel.org"
	<linux-csky@...r.kernel.org>, <linux-hexagon@...r.kernel.org>,
	"linux-openrisc@...r.kernel.org" <linux-openrisc@...r.kernel.org>
Subject: Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt

On Wed, Jan 31, 2024 at 12:48:38PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote:
> > This is a tiny fix to pfn_to_virt() for some platforms.
> >
> > The original implementaion of pfn_to_virt() takes PFN instead of PA as the
> > input to macro __va, with PAGE_SHIFT applying to the converted VA, which
> > is not right under most conditions, especially when there's an offset in
> > __va.
> >
> >
> > Yan Zhao (4):
> >   asm-generic/page.h: apply page shift to PFN instead of VA in
> >     pfn_to_virt
> >   csky: apply page shift to PFN instead of VA in pfn_to_virt
> >   Hexagon: apply page shift to PFN instead of VA in pfn_to_virt
> >   openrisc: apply page shift to PFN instead of VA in pfn_to_virt
> 
> Nice catch, this is clearly a correct fix, and I can take
> the series through the asm-generic tree if we want to take
> this approach.
> 
> I made a couple of interesting observations looking at this patch
> though:
> 
> - this function is only used in architecture specific code on
>   m68k, riscv and s390, though a couple of other architectures
>   have the same definition.
> 
> - There is another function that does the same thing called
>   pfn_to_kaddr(), which is defined on arm, arm64, csky,
>   loongarch, mips, nios2, powerpc, s390, sh, sparc and x86,
>   as well as yet another pfn_va() on parisc.
> 
> - the asm-generic/page.h file used to be included by h8300, c6x
>   and blackfin, all of which are now gone. It has no users left
>   and can just as well get removed, unless we find a new use
>   for it.
> 
> Since it looks like the four broken functions you fix
> don't have a single caller, maybe it would be better to
> just remove them all?
> 
> How exactly did you notice the function being wrong,
> did you try to add a user somewhere, or just read through
> the code?
I came across them when I was debugging an unexpected kernel page fault
on x86, and I was not sure whether page_to_virt() was compiled in
asm-generic/page.h or linux/mm.h.
Though finally, it turned out that the one in linux/mm.h was used, which
yielded the right result and the unexpected kernel page fault in my case
was not related to page_to_virt(), it did lead me to noticing that the
pfn_to_virt() in asm-generic/page.h and other 3 archs did not look right.

Yes, unlike virt_to_pfn() which still has a caller in openrisc (among
csky, Hexagon, openrisc), pfn_to_virt() now does not have a caller in
the 3 archs. Though both virt_to_pfn() and pfn_to_virt() are referenced
in asm-generic/page.h, I also not sure if we need to remove the
asm-generic/page.h which may serve as a template to future archs ?

So, either way looks good to me :)