Message ID | 20220704063909.295546-1-aneesh.kumar@linux.ibm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2] powerpc/mm: Use is_vmalloc_addr to validate addr | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | fail | kernel (mpc885_ads_defconfig, korg-5.5.0) failed at step build. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit : > Instead of high_memory use is_vmalloc_addr to validate that the address is > not in the vmalloc range. Do we really need even more extra checks, and a function that is not inlined anymore ? virt_addr_valid() used to be pretty simple. Some extra tests were added by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit") in order to work around some corner cases, and the commit message say they are temporary. virt_addr_valid() is there to check that an address is a valid linear mapping, not that an address IS NOT a vmalloc address. What will happen with your check if you pass an address that is from an ioremap done prior to the start of the vmalloc system ? WIth the series I send last week to add KASAN to book3e/64, we now have VMALLOC above PAGE_OFFSET on all platforms so we should be able to come back to the original virt_addr_valid(), based exclusively on pfn_valid() for PPC64, and pfn_valid() && high_memory for PPC32 (Or maybe only for PPC32 having HIGHMEM) Christophe > > Cc: Kefeng Wang <wangkefeng.wang@huawei.com> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > - > --- > arch/powerpc/include/asm/page.h | 10 ++++------ > arch/powerpc/mm/mem.c | 11 +++++++++++ > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index e5f75c70eda8..977835570db3 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn) > #define virt_to_pfn(kaddr) (__pa(kaddr) >> PAGE_SHIFT) > #define virt_to_page(kaddr) pfn_to_page(virt_to_pfn(kaddr)) > #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) > - > -#define virt_addr_valid(vaddr) ({ \ > - unsigned long _addr = (unsigned long)vaddr; \ > - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ > - pfn_valid(virt_to_pfn(_addr)); \ > -}) > +#ifndef __ASSEMBLY__ > +extern bool __virt_addr_valid(unsigned long kaddr); > +#endif > +#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr)) > > /* > * On Book-E parts we need __va to parse the device tree and we can't > diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c > index 7b0d286bf9ba..622f8bac808b 100644 > --- a/arch/powerpc/mm/mem.c > +++ b/arch/powerpc/mm/mem.c > @@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn) > * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed. > */ > EXPORT_SYMBOL_GPL(walk_system_ram_range); > + > +bool __virt_addr_valid(unsigned long kaddr) > +{ > + if (kaddr < PAGE_OFFSET) > + return false; > + if (is_vmalloc_addr((void *) kaddr)) > + return false; > + return pfn_valid(virt_to_pfn(kaddr)); > +} > +EXPORT_SYMBOL(__virt_addr_valid); > +
On 7/4/22 12:43 PM, Christophe Leroy wrote: > > > Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit : >> Instead of high_memory use is_vmalloc_addr to validate that the address is >> not in the vmalloc range. > > > Do we really need even more extra checks, and a function that is not > inlined anymore ? > > virt_addr_valid() used to be pretty simple. Some extra tests were added > by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit > Book3E & 32-bit") in order to work around some corner cases, and the > commit message say they are temporary. > > virt_addr_valid() is there to check that an address is a valid linear > mapping, not that an address IS NOT a vmalloc address. What will happen > with your check if you pass an address that is from an ioremap done > prior to the start of the vmalloc system ? > I was expecting the io range to be handled by pfn_valid(). IS there a memory layout ascii diagram of book3e/64 like asm/book3s/64/radix.h:51 ? My goal with the change was to make it more explicit what is it being validated. > WIth the series I send last week to add KASAN to book3e/64, we now have > VMALLOC above PAGE_OFFSET on all platforms so we should be able to come > back to the original virt_addr_valid(), based exclusively on pfn_valid() > for PPC64, and pfn_valid() && high_memory for PPC32 (Or maybe only for > PPC32 having HIGHMEM) > > > Christophe > > >> >> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> >> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> - >> --- >> arch/powerpc/include/asm/page.h | 10 ++++------ >> arch/powerpc/mm/mem.c | 11 +++++++++++ >> 2 files changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >> index e5f75c70eda8..977835570db3 100644 >> --- a/arch/powerpc/include/asm/page.h >> +++ b/arch/powerpc/include/asm/page.h >> @@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn) >> #define virt_to_pfn(kaddr) (__pa(kaddr) >> PAGE_SHIFT) >> #define virt_to_page(kaddr) pfn_to_page(virt_to_pfn(kaddr)) >> #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) >> - >> -#define virt_addr_valid(vaddr) ({ \ >> - unsigned long _addr = (unsigned long)vaddr; \ >> - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ >> - pfn_valid(virt_to_pfn(_addr)); \ >> -}) >> +#ifndef __ASSEMBLY__ >> +extern bool __virt_addr_valid(unsigned long kaddr); >> +#endif >> +#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr)) >> >> /* >> * On Book-E parts we need __va to parse the device tree and we can't >> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c >> index 7b0d286bf9ba..622f8bac808b 100644 >> --- a/arch/powerpc/mm/mem.c >> +++ b/arch/powerpc/mm/mem.c >> @@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn) >> * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed. >> */ >> EXPORT_SYMBOL_GPL(walk_system_ram_range); >> + >> +bool __virt_addr_valid(unsigned long kaddr) >> +{ >> + if (kaddr < PAGE_OFFSET) >> + return false; >> + if (is_vmalloc_addr((void *) kaddr)) >> + return false; >> + return pfn_valid(virt_to_pfn(kaddr)); >> +} >> +EXPORT_SYMBOL(__virt_addr_valid); >> +
Le 04/07/2022 à 09:45, Aneesh Kumar K V a écrit : > On 7/4/22 12:43 PM, Christophe Leroy wrote: >> >> >> Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit : >>> Instead of high_memory use is_vmalloc_addr to validate that the address is >>> not in the vmalloc range. >> >> >> Do we really need even more extra checks, and a function that is not >> inlined anymore ? >> >> virt_addr_valid() used to be pretty simple. Some extra tests were added >> by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit >> Book3E & 32-bit") in order to work around some corner cases, and the >> commit message say they are temporary. >> >> virt_addr_valid() is there to check that an address is a valid linear >> mapping, not that an address IS NOT a vmalloc address. What will happen >> with your check if you pass an address that is from an ioremap done >> prior to the start of the vmalloc system ? >> > > I was expecting the io range to be handled by pfn_valid(). IS there a memory layout > ascii diagram of book3e/64 like asm/book3s/64/radix.h:51 ? My goal with the > change was to make it more explicit what is it being validated. Yes you are right it should be handled by pfn_valid(), just like the entire VMALLOC range indeed. But on PPC32 a valid pfn might hit above vmalloc space as well. You can find the new layout here : https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=c7b9ed7c34a9f5dbf8222d63e3e313cef9f3150b The only problem we have with pfn_valid() is for PPC32 because pfn_valid() also include highmem memory. That's the reason why we need to check that the address is below high_memory in addition. For everything else, pfn_valid() should be enough. For PPC64, we may want to add a verification that we are in the 0xc.... range, because of the way __pa/__va work. On PPC32 that's not needed. > >> WIth the series I send last week to add KASAN to book3e/64, we now have >> VMALLOC above PAGE_OFFSET on all platforms so we should be able to come >> back to the original virt_addr_valid(), based exclusively on pfn_valid() >> for PPC64, and pfn_valid() && high_memory for PPC32 (Or maybe only for >> PPC32 having HIGHMEM) >> >> >> Christophe >> >> >>> >>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> >>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>> - >>> --- >>> arch/powerpc/include/asm/page.h | 10 ++++------ >>> arch/powerpc/mm/mem.c | 11 +++++++++++ >>> 2 files changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >>> index e5f75c70eda8..977835570db3 100644 >>> --- a/arch/powerpc/include/asm/page.h >>> +++ b/arch/powerpc/include/asm/page.h >>> @@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn) >>> #define virt_to_pfn(kaddr) (__pa(kaddr) >> PAGE_SHIFT) >>> #define virt_to_page(kaddr) pfn_to_page(virt_to_pfn(kaddr)) >>> #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) >>> - >>> -#define virt_addr_valid(vaddr) ({ \ >>> - unsigned long _addr = (unsigned long)vaddr; \ >>> - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ >>> - pfn_valid(virt_to_pfn(_addr)); \ >>> -}) >>> +#ifndef __ASSEMBLY__ >>> +extern bool __virt_addr_valid(unsigned long kaddr); >>> +#endif >>> +#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr)) >>> >>> /* >>> * On Book-E parts we need __va to parse the device tree and we can't >>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c >>> index 7b0d286bf9ba..622f8bac808b 100644 >>> --- a/arch/powerpc/mm/mem.c >>> +++ b/arch/powerpc/mm/mem.c >>> @@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn) >>> * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed. >>> */ >>> EXPORT_SYMBOL_GPL(walk_system_ram_range); >>> + >>> +bool __virt_addr_valid(unsigned long kaddr) >>> +{ >>> + if (kaddr < PAGE_OFFSET) >>> + return false; >>> + if (is_vmalloc_addr((void *) kaddr)) >>> + return false; >>> + return pfn_valid(virt_to_pfn(kaddr)); >>> +} >>> +EXPORT_SYMBOL(__virt_addr_valid); >>> + >
Le 04/07/2022 à 09:55, Christophe Leroy a écrit : > > > Le 04/07/2022 à 09:45, Aneesh Kumar K V a écrit : >> On 7/4/22 12:43 PM, Christophe Leroy wrote: >>> >>> >>> Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit : >>>> Instead of high_memory use is_vmalloc_addr to validate that the >>>> address is >>>> not in the vmalloc range. >>> >>> >>> Do we really need even more extra checks, and a function that is not >>> inlined anymore ? >>> >>> virt_addr_valid() used to be pretty simple. Some extra tests were added >>> by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit >>> Book3E & 32-bit") in order to work around some corner cases, and the >>> commit message say they are temporary. >>> >>> virt_addr_valid() is there to check that an address is a valid linear >>> mapping, not that an address IS NOT a vmalloc address. What will happen >>> with your check if you pass an address that is from an ioremap done >>> prior to the start of the vmalloc system ? >>> >> >> I was expecting the io range to be handled by pfn_valid(). IS there a >> memory layout >> ascii diagram of book3e/64 like asm/book3s/64/radix.h:51 ? My goal >> with the >> change was to make it more explicit what is it being validated. > > > Yes you are right it should be handled by pfn_valid(), just like the > entire VMALLOC range indeed. But on PPC32 a valid pfn might hit above > vmalloc space as well. > > You can find the new layout here : > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=c7b9ed7c34a9f5dbf8222d63e3e313cef9f3150b > > > The only problem we have with pfn_valid() is for PPC32 because > pfn_valid() also include highmem memory. That's the reason why we need > to check that the address is below high_memory in addition. > > For everything else, pfn_valid() should be enough. > > For PPC64, we may want to add a verification that we are in the 0xc.... > range, because of the way __pa/__va work. On PPC32 that's not needed. > So, I would do something like that: static __always_inline bool __virt_addr_valid(unsigned long addr) { if (IS_ENABLED(CONFIG_PPC64) && lm_alias(addr) != addr) return false; if (IS_ENABLED(CONFIG_HIGHMEM) && addr >= (unsigned long)high_memory) return false; return pfn_valid(virt_to_pfn(addr)); } #define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr))
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h index e5f75c70eda8..977835570db3 100644 --- a/arch/powerpc/include/asm/page.h +++ b/arch/powerpc/include/asm/page.h @@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn) #define virt_to_pfn(kaddr) (__pa(kaddr) >> PAGE_SHIFT) #define virt_to_page(kaddr) pfn_to_page(virt_to_pfn(kaddr)) #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) - -#define virt_addr_valid(vaddr) ({ \ - unsigned long _addr = (unsigned long)vaddr; \ - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ - pfn_valid(virt_to_pfn(_addr)); \ -}) +#ifndef __ASSEMBLY__ +extern bool __virt_addr_valid(unsigned long kaddr); +#endif +#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr)) /* * On Book-E parts we need __va to parse the device tree and we can't diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c index 7b0d286bf9ba..622f8bac808b 100644 --- a/arch/powerpc/mm/mem.c +++ b/arch/powerpc/mm/mem.c @@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn) * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed. */ EXPORT_SYMBOL_GPL(walk_system_ram_range); + +bool __virt_addr_valid(unsigned long kaddr) +{ + if (kaddr < PAGE_OFFSET) + return false; + if (is_vmalloc_addr((void *) kaddr)) + return false; + return pfn_valid(virt_to_pfn(kaddr)); +} +EXPORT_SYMBOL(__virt_addr_valid); +
Instead of high_memory use is_vmalloc_addr to validate that the address is not in the vmalloc range. Cc: Kefeng Wang <wangkefeng.wang@huawei.com> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> - --- arch/powerpc/include/asm/page.h | 10 ++++------ arch/powerpc/mm/mem.c | 11 +++++++++++ 2 files changed, 15 insertions(+), 6 deletions(-)