| Message ID | 20220424172044.22220-1-rppt@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm[64]/memremap: don't abuse pfn_valid() to ensure presence of linear map | expand |
Hi Mike,
I love your patch! Yet something to improve:
[auto build test ERROR on b2d229d4ddb17db541098b83524d901257e93845]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Rapoport/arm-64-memremap-don-t-abuse-pfn_valid-to-ensure-presence-of-linear-map/20220425-012242
base: b2d229d4ddb17db541098b83524d901257e93845
config: arm-randconfig-r015-20220424 (https://download.01.org/0day-ci/archive/20220425/202204250429.nq0alVBK-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/635763878be30ab45f350cdcffba3d8e71089942
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mike-Rapoport/arm-64-memremap-don-t-abuse-pfn_valid-to-ensure-presence-of-linear-map/20220425-012242
git checkout 635763878be30ab45f350cdcffba3d8e71089942
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: kernel/iomem.o: in function `memremap':
>> iomem.c:(.text+0x36): undefined reference to `arch_memremap_can_ram_remap'
arm-linux-gnueabi-ld: drivers/gpu/drm/drm_gem_shmem_helper.o: in function `drm_gem_shmem_fault':
drm_gem_shmem_helper.c:(.text+0x54): undefined reference to `vmf_insert_pfn'
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for DRM_GEM_SHMEM_HELPER
Depends on HAS_IOMEM && DRM && MMU
Selected by
- DRM_SSD130X && HAS_IOMEM && DRM
Hi Mike,
I love your patch! Yet something to improve:
[auto build test ERROR on b2d229d4ddb17db541098b83524d901257e93845]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Rapoport/arm-64-memremap-don-t-abuse-pfn_valid-to-ensure-presence-of-linear-map/20220425-012242
base: b2d229d4ddb17db541098b83524d901257e93845
config: arm-randconfig-c002-20220424 (https://download.01.org/0day-ci/archive/20220425/202204250425.OjOiS0ZK-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/635763878be30ab45f350cdcffba3d8e71089942
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mike-Rapoport/arm-64-memremap-don-t-abuse-pfn_valid-to-ensure-presence-of-linear-map/20220425-012242
git checkout 635763878be30ab45f350cdcffba3d8e71089942
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: kernel/iomem.o: in function `try_ram_remap':
>> kernel/iomem.c:37: undefined reference to `arch_memremap_can_ram_remap'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_i2c_remove':
drivers/gpu/drm/bridge/ite-it6505.c:3317: undefined reference to `drm_dp_aux_unregister'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_get_dpcd':
drivers/gpu/drm/bridge/ite-it6505.c:602: undefined reference to `drm_dp_dpcd_read'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_bridge_attach':
drivers/gpu/drm/bridge/ite-it6505.c:2858: undefined reference to `drm_dp_aux_register'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_writeb':
include/drm/dp/drm_dp_helper.h:2088: undefined reference to `drm_dp_dpcd_write'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_readb':
include/drm/dp/drm_dp_helper.h:2073: undefined reference to `drm_dp_dpcd_read'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_hdcp_work':
drivers/gpu/drm/bridge/ite-it6505.c:2084: undefined reference to `drm_dp_dpcd_read_link_status'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:2088: undefined reference to `drm_dp_channel_eq_ok'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_step_cr_train':
drivers/gpu/drm/bridge/ite-it6505.c:1688: undefined reference to `drm_dp_link_train_clock_recovery_delay'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1689: undefined reference to `drm_dp_dpcd_read_link_status'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1691: undefined reference to `drm_dp_clock_recovery_ok'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1704: undefined reference to `drm_dp_get_adjust_request_voltage'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1708: undefined reference to `drm_dp_get_adjust_request_pre_emphasis'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_step_eq_train':
drivers/gpu/drm/bridge/ite-it6505.c:1760: undefined reference to `drm_dp_link_train_channel_eq_delay'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1761: undefined reference to `drm_dp_dpcd_read_link_status'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1763: undefined reference to `drm_dp_clock_recovery_ok'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1766: undefined reference to `drm_dp_channel_eq_ok'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1777: undefined reference to `drm_dp_get_adjust_request_voltage'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:1781: undefined reference to `drm_dp_get_adjust_request_pre_emphasis'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_drm_dp_link_configure':
drivers/gpu/drm/bridge/ite-it6505.c:1603: undefined reference to `drm_dp_dpcd_write'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_parse_link_capabilities':
drivers/gpu/drm/bridge/ite-it6505.c:1457: undefined reference to `drm_dp_link_rate_to_bw_code'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_drm_dp_link_probe':
drivers/gpu/drm/bridge/ite-it6505.c:726: undefined reference to `drm_dp_dpcd_read'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:731: undefined reference to `drm_dp_bw_code_to_link_rate'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_readb':
include/drm/dp/drm_dp_helper.h:2073: undefined reference to `drm_dp_dpcd_read'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `drm_dp_dpcd_writeb':
include/drm/dp/drm_dp_helper.h:2088: undefined reference to `drm_dp_dpcd_write'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.o: in function `it6505_process_hpd_irq':
drivers/gpu/drm/bridge/ite-it6505.c:2293: undefined reference to `drm_dp_dpcd_read_link_status'
arm-linux-gnueabi-ld: drivers/gpu/drm/bridge/ite-it6505.c:2302: undefined reference to `drm_dp_channel_eq_ok'
vim +37 kernel/iomem.c
5981690ddb8f72 Dan Williams 2018-03-29 29
5981690ddb8f72 Dan Williams 2018-03-29 30 static void *try_ram_remap(resource_size_t offset, size_t size,
5981690ddb8f72 Dan Williams 2018-03-29 31 unsigned long flags)
5981690ddb8f72 Dan Williams 2018-03-29 32 {
5981690ddb8f72 Dan Williams 2018-03-29 33 unsigned long pfn = PHYS_PFN(offset);
5981690ddb8f72 Dan Williams 2018-03-29 34
5981690ddb8f72 Dan Williams 2018-03-29 35 /* In the simple case just return the existing linear address */
635763878be30a Mike Rapoport 2022-04-24 36 if (!PageHighMem(pfn_to_page(pfn)) &&
5981690ddb8f72 Dan Williams 2018-03-29 @37 arch_memremap_can_ram_remap(offset, size, flags))
5981690ddb8f72 Dan Williams 2018-03-29 38 return __va(offset);
5981690ddb8f72 Dan Williams 2018-03-29 39
5981690ddb8f72 Dan Williams 2018-03-29 40 return NULL; /* fallback to arch_memremap_wb */
5981690ddb8f72 Dan Williams 2018-03-29 41 }
5981690ddb8f72 Dan Williams 2018-03-29 42
On Sun, 24 Apr 2022 at 19:22, Mike Rapoport <rppt@kernel.org> wrote: > > From: Mike Rapoport <rppt@linux.ibm.com> > > The semantics of pfn_valid() is to check presence of the memory map for a > PFN and not whether a PFN is covered by the linear map. The memory map may > be present for NOMAP memory regions, but they won't be mapped in the linear > mapping. Accessing such regions via __va() when they are memremap()'ed > will cause a crash. > > On v5.4.y the crash happens on qemu-arm with UEFI [1]: > > <1>[ 0.084476] 8<--- cut here --- > <1>[ 0.084595] Unable to handle kernel paging request at virtual address dfb76000 > <1>[ 0.084938] pgd = (ptrval) > <1>[ 0.085038] [dfb76000] *pgd=5f7fe801, *pte=00000000, *ppte=00000000 > > ... > > <4>[ 0.093923] [<c0ed6ce8>] (memcpy) from [<c16a06f8>] (dmi_setup+0x60/0x418) > <4>[ 0.094204] [<c16a06f8>] (dmi_setup) from [<c16a38d4>] (arm_dmi_init+0x8/0x10) > <4>[ 0.094408] [<c16a38d4>] (arm_dmi_init) from [<c0302e9c>] (do_one_initcall+0x50/0x228) > <4>[ 0.094619] [<c0302e9c>] (do_one_initcall) from [<c16011e4>] (kernel_init_freeable+0x15c/0x1f8) > <4>[ 0.094841] [<c16011e4>] (kernel_init_freeable) from [<c0f028cc>] (kernel_init+0x8/0x10c) > <4>[ 0.095057] [<c0f028cc>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c) > > On kernels v5.10.y and newer the same crash won't reproduce on ARM because > commit b10d6bca8720 ("arch, drivers: replace for_each_membock() with > for_each_mem_range()") changed the way memory regions are registered in the > resource tree, but that merely covers up the problem. > > On ARM64 memory resources registered in yet another way and there the > issue of wrong usage of pfn_valid() to ensure availability of the linear > map is also covered. > > Implement arch_memremap_can_ram_remap() on ARM and ARM64 to prevent access > to NOMAP regions via the linear mapping in memremap(). > > Link: https://lore.kernel.org/all/Yl65zxGgFzF1Okac@sirena.org.uk > Reported-by: "kernelci.org bot" <bot@kernelci.org> > Tested-by: Mark Brown <broonie@kernel.org> > Cc: stable@vger.kernel.org # 5.4+ > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > --- > arch/arm/include/asm/io.h | 4 ++++ > arch/arm/mm/ioremap.c | 9 ++++++++- > arch/arm64/include/asm/io.h | 4 ++++ > arch/arm64/mm/ioremap.c | 8 ++++++++ > kernel/iomem.c | 2 +- > 5 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > index 0c70eb688a00..fbb2eeea7285 100644 > --- a/arch/arm/include/asm/io.h > +++ b/arch/arm/include/asm/io.h > @@ -145,6 +145,10 @@ extern void __iomem * (*arch_ioremap_caller)(phys_addr_t, size_t, > unsigned int, void *); > extern void (*arch_iounmap)(volatile void __iomem *); > > +extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, > + unsigned long flags); > +#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap > + > /* > * Bad read/write accesses... > */ > diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c > index aa08bcb72db9..6eb1ad24544d 100644 > --- a/arch/arm/mm/ioremap.c > +++ b/arch/arm/mm/ioremap.c > @@ -43,7 +43,6 @@ > #include <asm/mach/pci.h> > #include "mm.h" > > - > LIST_HEAD(static_vmlist); > > static struct static_vm *find_static_vm_paddr(phys_addr_t paddr, > @@ -493,3 +492,11 @@ void __init early_ioremap_init(void) > { > early_ioremap_setup(); > } > + > +bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, > + unsigned long flags) > +{ > + unsigned long pfn = PHYS_PFN(offset); > + > + return memblock_is_map_memory(pfn); > +} > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index 7fd836bea7eb..3995652daf81 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -192,4 +192,8 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size); > extern int valid_phys_addr_range(phys_addr_t addr, size_t size); > extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size); > > +extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, > + unsigned long flags); > +#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap > + > #endif /* __ASM_IO_H */ > diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c > index b7c81dacabf0..b21f91cd830d 100644 > --- a/arch/arm64/mm/ioremap.c > +++ b/arch/arm64/mm/ioremap.c > @@ -99,3 +99,11 @@ void __init early_ioremap_init(void) > { > early_ioremap_setup(); > } > + > +bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, > + unsigned long flags) > +{ > + unsigned long pfn = PHYS_PFN(offset); > + > + return pfn_is_map_memory(pfn); > +} > diff --git a/kernel/iomem.c b/kernel/iomem.c > index 62c92e43aa0d..e85bed24c0a9 100644 > --- a/kernel/iomem.c > +++ b/kernel/iomem.c > @@ -33,7 +33,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, > unsigned long pfn = PHYS_PFN(offset); > > /* In the simple case just return the existing linear address */ > - if (pfn_valid(pfn) && !PageHighMem(pfn_to_page(pfn)) && > + if (!PageHighMem(pfn_to_page(pfn)) && This looks wrong to me. Calling any of the PageXxx() accessors is only safe if the PFN is valid, since otherwise, we don't know if the associated struct page exists. > arch_memremap_can_ram_remap(offset, size, flags)) > return __va(offset); > > > base-commit: b2d229d4ddb17db541098b83524d901257e93845 > -- > 2.28.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Apr 24, 2022 at 11:19:05PM +0200, Ard Biesheuvel wrote: > On Sun, 24 Apr 2022 at 19:22, Mike Rapoport <rppt@kernel.org> wrote: > > > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > The semantics of pfn_valid() is to check presence of the memory map for a > > PFN and not whether a PFN is covered by the linear map. The memory map may > > be present for NOMAP memory regions, but they won't be mapped in the linear > > mapping. Accessing such regions via __va() when they are memremap()'ed > > will cause a crash. ... > > diff --git a/kernel/iomem.c b/kernel/iomem.c > > index 62c92e43aa0d..e85bed24c0a9 100644 > > --- a/kernel/iomem.c > > +++ b/kernel/iomem.c > > @@ -33,7 +33,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, > > unsigned long pfn = PHYS_PFN(offset); > > > > /* In the simple case just return the existing linear address */ > > - if (pfn_valid(pfn) && !PageHighMem(pfn_to_page(pfn)) && > > + if (!PageHighMem(pfn_to_page(pfn)) && > > This looks wrong to me. Calling any of the PageXxx() accessors is only > safe if the PFN is valid, since otherwise, we don't know if the > associated struct page exists. Yeah, you are right, was over-enthusiastic here... > > arch_memremap_can_ram_remap(offset, size, flags)) > > return __va(offset); > > > > > > base-commit: b2d229d4ddb17db541098b83524d901257e93845 > > -- > > 2.28.0 > >
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 0c70eb688a00..fbb2eeea7285 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -145,6 +145,10 @@ extern void __iomem * (*arch_ioremap_caller)(phys_addr_t, size_t, unsigned int, void *); extern void (*arch_iounmap)(volatile void __iomem *); +extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, + unsigned long flags); +#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap + /* * Bad read/write accesses... */ diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c index aa08bcb72db9..6eb1ad24544d 100644 --- a/arch/arm/mm/ioremap.c +++ b/arch/arm/mm/ioremap.c @@ -43,7 +43,6 @@ #include <asm/mach/pci.h> #include "mm.h" - LIST_HEAD(static_vmlist); static struct static_vm *find_static_vm_paddr(phys_addr_t paddr, @@ -493,3 +492,11 @@ void __init early_ioremap_init(void) { early_ioremap_setup(); } + +bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, + unsigned long flags) +{ + unsigned long pfn = PHYS_PFN(offset); + + return memblock_is_map_memory(pfn); +} diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index 7fd836bea7eb..3995652daf81 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -192,4 +192,8 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size); extern int valid_phys_addr_range(phys_addr_t addr, size_t size); extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size); +extern bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, + unsigned long flags); +#define arch_memremap_can_ram_remap arch_memremap_can_ram_remap + #endif /* __ASM_IO_H */ diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c index b7c81dacabf0..b21f91cd830d 100644 --- a/arch/arm64/mm/ioremap.c +++ b/arch/arm64/mm/ioremap.c @@ -99,3 +99,11 @@ void __init early_ioremap_init(void) { early_ioremap_setup(); } + +bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, + unsigned long flags) +{ + unsigned long pfn = PHYS_PFN(offset); + + return pfn_is_map_memory(pfn); +} diff --git a/kernel/iomem.c b/kernel/iomem.c index 62c92e43aa0d..e85bed24c0a9 100644 --- a/kernel/iomem.c +++ b/kernel/iomem.c @@ -33,7 +33,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, unsigned long pfn = PHYS_PFN(offset); /* In the simple case just return the existing linear address */ - if (pfn_valid(pfn) && !PageHighMem(pfn_to_page(pfn)) && + if (!PageHighMem(pfn_to_page(pfn)) && arch_memremap_can_ram_remap(offset, size, flags)) return __va(offset);