| Message ID | 1566010813-27219-1-git-send-email-huangzhaoyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arch : arm : add a criteria for pfn_valid | expand |
On Sat, Aug 17, 2019 at 11:00:13AM +0800, Zhaoyang Huang wrote: > #ifdef CONFIG_HAVE_ARCH_PFN_VALID > int pfn_valid(unsigned long pfn) > { > - return memblock_is_map_memory(__pfn_to_phys(pfn)); > + return (pfn > max_pfn) ? > + false : memblock_is_map_memory(__pfn_to_phys(pfn)); > } This is a really awkward way to use the ternary operator. It's easier to read if you just: + if (pfn > max_pfn) + return 0; return memblock_is_map_memory(__pfn_to_phys(pfn)); (if you really wanted to be clever ... er, obscure, you'd've written: return (pfn <= max_pfn) && memblock_is_map_memory(__pfn_to_phys(pfn)); ... but don't do that) Also, why is this diverged between arm and arm64?
On Sat, Aug 17, 2019 at 11:00:13AM +0800, Zhaoyang Huang wrote: > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > pfn_valid can be wrong while the MSB of physical address be trimed as pfn > larger than the max_pfn. How the overflow of __pfn_to_phys() is related to max_pfn? Where is the guarantee that __pfn_to_phys(max_pfn) won't overflow? > Signed-off-by: Zhaoyang Huang <huangzhaoyang@gmail.com> > --- > arch/arm/mm/init.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c > index c2daabb..9c4d938 100644 > --- a/arch/arm/mm/init.c > +++ b/arch/arm/mm/init.c > @@ -177,7 +177,8 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max_low, > #ifdef CONFIG_HAVE_ARCH_PFN_VALID > int pfn_valid(unsigned long pfn) > { > - return memblock_is_map_memory(__pfn_to_phys(pfn)); > + return (pfn > max_pfn) ? > + false : memblock_is_map_memory(__pfn_to_phys(pfn)); > } > EXPORT_SYMBOL(pfn_valid); > #endif > -- > 1.9.1 >
On Sat, Aug 17, 2019 at 5:00 PM Mike Rapoport <rppt@linux.ibm.com> wrote: > > On Sat, Aug 17, 2019 at 11:00:13AM +0800, Zhaoyang Huang wrote: > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > pfn_valid can be wrong while the MSB of physical address be trimed as pfn > > larger than the max_pfn. > > How the overflow of __pfn_to_phys() is related to max_pfn? > Where is the guarantee that __pfn_to_phys(max_pfn) won't overflow? eg, the invalid pfn value as 0x1bffc0 will pass pfn_valid if there is a memory block while the max_pfn is 0xbffc0. In ARM64, bellowing condition check will help to > > > Signed-off-by: Zhaoyang Huang <huangzhaoyang@gmail.com> > > --- > > arch/arm/mm/init.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c > > index c2daabb..9c4d938 100644 > > --- a/arch/arm/mm/init.c > > +++ b/arch/arm/mm/init.c > > @@ -177,7 +177,8 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max_low, > > #ifdef CONFIG_HAVE_ARCH_PFN_VALID > > int pfn_valid(unsigned long pfn) > > { > > - return memblock_is_map_memory(__pfn_to_phys(pfn)); > > + return (pfn > max_pfn) ? > > + false : memblock_is_map_memory(__pfn_to_phys(pfn)); > > } > > EXPORT_SYMBOL(pfn_valid); > > #endif > > -- > > 1.9.1 > > > > -- > Sincerely yours, > Mike. >
On Sat, Aug 17, 2019 at 11:00:13AM +0800, Zhaoyang Huang wrote: > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > pfn_valid can be wrong while the MSB of physical address be trimed as pfn > larger than the max_pfn. What scenario are you addressing here? At a guess, you're addressing the non-LPAE case with PFNs that correspond with >= 4GiB of memory? > > Signed-off-by: Zhaoyang Huang <huangzhaoyang@gmail.com> > --- > arch/arm/mm/init.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c > index c2daabb..9c4d938 100644 > --- a/arch/arm/mm/init.c > +++ b/arch/arm/mm/init.c > @@ -177,7 +177,8 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max_low, > #ifdef CONFIG_HAVE_ARCH_PFN_VALID > int pfn_valid(unsigned long pfn) > { > - return memblock_is_map_memory(__pfn_to_phys(pfn)); > + return (pfn > max_pfn) ? > + false : memblock_is_map_memory(__pfn_to_phys(pfn)); > } > EXPORT_SYMBOL(pfn_valid); > #endif > -- > 1.9.1 > >
On Sun, Aug 18, 2019 at 2:32 AM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Sat, Aug 17, 2019 at 11:00:13AM +0800, Zhaoyang Huang wrote: > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > pfn_valid can be wrong while the MSB of physical address be trimed as pfn > > larger than the max_pfn. > > What scenario are you addressing here? At a guess, you're addressing > the non-LPAE case with PFNs that correspond with >= 4GiB of memory? Please find bellowing for the callstack caused by this defect. The original reason is a invalid PFN passed from userspace which will introduce a invalid page within stable_page_flags and then kernel panic. [46886.723249] c7 [<c031ff98>] (stable_page_flags) from [<c03203f8>] (kpageflags_read+0x90/0x11c) [46886.723256] c7 r9:c101ce04 r8:c2d0bf70 r7:c2d0bf70 r6:1fbb10fb r5:a8686f08 r4:a8686f08 [46886.723264] c7 [<c0320368>] (kpageflags_read) from [<c0312030>] (proc_reg_read+0x80/0x94) [46886.723270] c7 r10:000000b4 r9:00000008 r8:c2d0bf70 r7:00000000 r6:00000001 r5:ed8e7240 [46886.723272] c7 r4:00000000 [46886.723280] c7 [<c0311fb0>] (proc_reg_read) from [<c02a6e6c>] (__vfs_read+0x48/0x150) [46886.723284] c7 r7:c2d0bf70 r6:c0f09208 r5:c0a4f940 r4:c40326c0 [46886.723290] c7 [<c02a6e24>] (__vfs_read) from [<c02a7018>] (vfs_read+0xa4/0x158) [46886.723296] c7 r9:a8686f08 r8:00000008 r7:c2d0bf70 r6:a8686f08 r5:c40326c0 r4:00000008 [46886.723301] c7 [<c02a6f74>] (vfs_read) from [<c02a778c>] (SyS_pread64+0x80/0xb8) [46886.723306] c7 r8:00000008 r7:c0f09208 r6:c40326c0 r5:c40326c0 r4:fdd887d8 [46886.723315] c7 [<c02a770c>] (SyS_pread64) from [<c0108620>] (ret_fast_syscall+0x0/0x28) > > > > > Signed-off-by: Zhaoyang Huang <huangzhaoyang@gmail.com> > > --- > > arch/arm/mm/init.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c > > index c2daabb..9c4d938 100644 > > --- a/arch/arm/mm/init.c > > +++ b/arch/arm/mm/init.c > > @@ -177,7 +177,8 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max_low, > > #ifdef CONFIG_HAVE_ARCH_PFN_VALID > > int pfn_valid(unsigned long pfn) > > { > > - return memblock_is_map_memory(__pfn_to_phys(pfn)); > > + return (pfn > max_pfn) ? > > + false : memblock_is_map_memory(__pfn_to_phys(pfn)); > > } > > EXPORT_SYMBOL(pfn_valid); > > #endif > > -- > > 1.9.1 > > > > > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up
On Sun, Aug 18, 2019 at 03:46:51PM +0800, Zhaoyang Huang wrote: > On Sun, Aug 18, 2019 at 2:32 AM Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Sat, Aug 17, 2019 at 11:00:13AM +0800, Zhaoyang Huang wrote: > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > > > pfn_valid can be wrong while the MSB of physical address be trimed as pfn > > > larger than the max_pfn. > > > > What scenario are you addressing here? At a guess, you're addressing > > the non-LPAE case with PFNs that correspond with >= 4GiB of memory? > Please find bellowing for the callstack caused by this defect. The > original reason is a invalid PFN passed from userspace which will > introduce a invalid page within stable_page_flags and then kernel > panic. Yeah, arm64 hit this issue a while ago and it was fixed with commit 5ad356eabc47 ("arm64: mm: check for upper PAGE_SHIFT bits in pfn_valid()"). IMHO, the check if ((addr >> PAGE_SHIFT) != pfn) is more robust than comparing pfn to max_pfn. > [46886.723249] c7 [<c031ff98>] (stable_page_flags) from [<c03203f8>] > (kpageflags_read+0x90/0x11c) > [46886.723256] c7 r9:c101ce04 r8:c2d0bf70 r7:c2d0bf70 r6:1fbb10fb > r5:a8686f08 r4:a8686f08 > [46886.723264] c7 [<c0320368>] (kpageflags_read) from [<c0312030>] > (proc_reg_read+0x80/0x94) > [46886.723270] c7 r10:000000b4 r9:00000008 r8:c2d0bf70 r7:00000000 > r6:00000001 r5:ed8e7240 > [46886.723272] c7 r4:00000000 > [46886.723280] c7 [<c0311fb0>] (proc_reg_read) from [<c02a6e6c>] > (__vfs_read+0x48/0x150) > [46886.723284] c7 r7:c2d0bf70 r6:c0f09208 r5:c0a4f940 r4:c40326c0 > [46886.723290] c7 [<c02a6e24>] (__vfs_read) from [<c02a7018>] > (vfs_read+0xa4/0x158) > [46886.723296] c7 r9:a8686f08 r8:00000008 r7:c2d0bf70 r6:a8686f08 > r5:c40326c0 r4:00000008 > [46886.723301] c7 [<c02a6f74>] (vfs_read) from [<c02a778c>] > (SyS_pread64+0x80/0xb8) > [46886.723306] c7 r8:00000008 r7:c0f09208 r6:c40326c0 r5:c40326c0 r4:fdd887d8 > [46886.723315] c7 [<c02a770c>] (SyS_pread64) from [<c0108620>] > (ret_fast_syscall+0x0/0x28) > > > > > > > > > Signed-off-by: Zhaoyang Huang <huangzhaoyang@gmail.com> > > > --- > > > arch/arm/mm/init.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c > > > index c2daabb..9c4d938 100644 > > > --- a/arch/arm/mm/init.c > > > +++ b/arch/arm/mm/init.c > > > @@ -177,7 +177,8 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max_low, > > > #ifdef CONFIG_HAVE_ARCH_PFN_VALID > > > int pfn_valid(unsigned long pfn) > > > { > > > - return memblock_is_map_memory(__pfn_to_phys(pfn)); > > > + return (pfn > max_pfn) ? > > > + false : memblock_is_map_memory(__pfn_to_phys(pfn)); > > > } > > > EXPORT_SYMBOL(pfn_valid); > > > #endif > > > -- > > > 1.9.1 > > > > > > > > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > > According to speedtest.net: 11.9Mbps down 500kbps up >
On Sun, Aug 18, 2019 at 11:20:35AM +0300, Mike Rapoport wrote: > On Sun, Aug 18, 2019 at 03:46:51PM +0800, Zhaoyang Huang wrote: > > On Sun, Aug 18, 2019 at 2:32 AM Russell King - ARM Linux admin > > <linux@armlinux.org.uk> wrote: > > > > > > On Sat, Aug 17, 2019 at 11:00:13AM +0800, Zhaoyang Huang wrote: > > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > > > > > > > pfn_valid can be wrong while the MSB of physical address be trimed as pfn > > > > larger than the max_pfn. > > > > > > What scenario are you addressing here? At a guess, you're addressing > > > the non-LPAE case with PFNs that correspond with >= 4GiB of memory? > > Please find bellowing for the callstack caused by this defect. The > > original reason is a invalid PFN passed from userspace which will > > introduce a invalid page within stable_page_flags and then kernel > > panic. Thanks. > Yeah, arm64 hit this issue a while ago and it was fixed with commit > 5ad356eabc47 ("arm64: mm: check for upper PAGE_SHIFT bits in pfn_valid()"). > > IMHO, the check > > if ((addr >> PAGE_SHIFT) != pfn) > > is more robust than comparing pfn to max_pfn. Yep, I'd prefer to see: phys_addr_t addr = __pfn_to_phys(pfn); if (__pfn_to_phys(addr) != pfn) return 0; return memblock_is_map_memory(addr);
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index c2daabb..9c4d938 100644 --- a/arch/arm/mm/init.c +++ b/arch/arm/mm/init.c @@ -177,7 +177,8 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max_low, #ifdef CONFIG_HAVE_ARCH_PFN_VALID int pfn_valid(unsigned long pfn) { - return memblock_is_map_memory(__pfn_to_phys(pfn)); + return (pfn > max_pfn) ? + false : memblock_is_map_memory(__pfn_to_phys(pfn)); } EXPORT_SYMBOL(pfn_valid); #endif