| Message ID | 1384457866-16135-4-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 14, 2013 at 02:37:43PM -0500, Santosh Shilimkar wrote: > Slab allocator can allocate memory beyond the lowmem watermark > which can lead to false failure of virt_addr_valid(). > > So drop the check. The issue was seen with percpu_alloc() > in KVM code which was allocating memory beyond lowmem watermark. > > Am not completly sure whether this is the right fix and if it could > impact any other user of virt_addr_valid(). Without this fix as > pointed out the KVM init was failing in my testing. > > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > arch/arm/include/asm/memory.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > index 4dd2145..412da47 100644 > --- a/arch/arm/include/asm/memory.h > +++ b/arch/arm/include/asm/memory.h > @@ -343,8 +343,7 @@ static inline __deprecated void *bus_to_virt(unsigned long x) > #define ARCH_PFN_OFFSET PHYS_PFN_OFFSET > > #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT) > -#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET && (unsigned long)(kaddr) < (unsigned long)high_memory) > - > +#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET) > #endif > > #include <asm-generic/memory_model.h> > -- > 1.7.9.5 > This looks wrong to me. Check Documentation/arm/memory.txt, this would return true for the VMALLOC region, which would cause virt_to_phys to give you something invalid, which would be bad. We use the check in create_hyp_mappings to be sure that the physical address returned by virt_to_phys is valid and that if we're mapping more than one page that those pages are physically contiguous. So if you want to get rid of this check, you need to change the mapping functionality to obtain the physical address by walking the page table mappings for each page that you are mapping instead. Or limit each call to a single page in size and take the physical address as input and use per_cpu_ptr_to_phys at the caller side instead. Alternatively, we need to get rid of alloc_percpu and use regular kmallocs instead, unless anyone else knows of an even better way. -Christoffer
On Thursday 14 November 2013 07:22 PM, Christoffer Dall wrote: > On Thu, Nov 14, 2013 at 02:37:43PM -0500, Santosh Shilimkar wrote: >> Slab allocator can allocate memory beyond the lowmem watermark >> which can lead to false failure of virt_addr_valid(). >> >> So drop the check. The issue was seen with percpu_alloc() >> in KVM code which was allocating memory beyond lowmem watermark. >> >> Am not completly sure whether this is the right fix and if it could >> impact any other user of virt_addr_valid(). Without this fix as >> pointed out the KVM init was failing in my testing. >> >> Cc: Christoffer Dall <christoffer.dall@linaro.org> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> --- >> arch/arm/include/asm/memory.h | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h >> index 4dd2145..412da47 100644 >> --- a/arch/arm/include/asm/memory.h >> +++ b/arch/arm/include/asm/memory.h >> @@ -343,8 +343,7 @@ static inline __deprecated void *bus_to_virt(unsigned long x) >> #define ARCH_PFN_OFFSET PHYS_PFN_OFFSET >> >> #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT) >> -#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET && (unsigned long)(kaddr) < (unsigned long)high_memory) >> - >> +#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET) >> #endif >> >> #include <asm-generic/memory_model.h> >> -- >> 1.7.9.5 >> > > This looks wrong to me. Check Documentation/arm/memory.txt, this would > return true for the VMALLOC region, which would cause virt_to_phys to > give you something invalid, which would be bad. > I also thought it might not be right fix and hence added a disclaimer in the commit message ;-) Regards, Santosh
On Thu, Nov 14, 2013 at 07:31:36PM -0500, Santosh Shilimkar wrote: > On Thursday 14 November 2013 07:22 PM, Christoffer Dall wrote: > > On Thu, Nov 14, 2013 at 02:37:43PM -0500, Santosh Shilimkar wrote: > >> Slab allocator can allocate memory beyond the lowmem watermark > >> which can lead to false failure of virt_addr_valid(). > >> > >> So drop the check. The issue was seen with percpu_alloc() > >> in KVM code which was allocating memory beyond lowmem watermark. > >> > >> Am not completly sure whether this is the right fix and if it could > >> impact any other user of virt_addr_valid(). Without this fix as > >> pointed out the KVM init was failing in my testing. > >> > >> Cc: Christoffer Dall <christoffer.dall@linaro.org> > >> Cc: Marc Zyngier <marc.zyngier@arm.com> > >> Cc: Russell King <linux@arm.linux.org.uk> > >> Cc: Catalin Marinas <catalin.marinas@arm.com> > >> Cc: Will Deacon <will.deacon@arm.com> > >> > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >> --- > >> arch/arm/include/asm/memory.h | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > >> index 4dd2145..412da47 100644 > >> --- a/arch/arm/include/asm/memory.h > >> +++ b/arch/arm/include/asm/memory.h > >> @@ -343,8 +343,7 @@ static inline __deprecated void *bus_to_virt(unsigned long x) > >> #define ARCH_PFN_OFFSET PHYS_PFN_OFFSET > >> > >> #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT) > >> -#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET && (unsigned long)(kaddr) < (unsigned long)high_memory) > >> - > >> +#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET) > >> #endif > >> > >> #include <asm-generic/memory_model.h> > >> -- > >> 1.7.9.5 > >> > > > > This looks wrong to me. Check Documentation/arm/memory.txt, this would > > return true for the VMALLOC region, which would cause virt_to_phys to > > give you something invalid, which would be bad. > > > I also thought it might not be right fix and hence added a disclaimer > in the commit message ;-) > Yes I know, I'm not holding it against you ;) Not sure what the nicest fix is though. I think we can get away with the single page restriction per call for Hyp mappings for now. IIRC the Hyp code is still limited to be within a single page. Let's see what Marc has to say. -Christoffer
On 15/11/13 00:40, Christoffer Dall wrote: > On Thu, Nov 14, 2013 at 07:31:36PM -0500, Santosh Shilimkar wrote: >> On Thursday 14 November 2013 07:22 PM, Christoffer Dall wrote: >>> On Thu, Nov 14, 2013 at 02:37:43PM -0500, Santosh Shilimkar wrote: >>>> Slab allocator can allocate memory beyond the lowmem watermark >>>> which can lead to false failure of virt_addr_valid(). >>>> >>>> So drop the check. The issue was seen with percpu_alloc() >>>> in KVM code which was allocating memory beyond lowmem watermark. >>>> >>>> Am not completly sure whether this is the right fix and if it could >>>> impact any other user of virt_addr_valid(). Without this fix as >>>> pointed out the KVM init was failing in my testing. >>>> >>>> Cc: Christoffer Dall <christoffer.dall@linaro.org> >>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>> Cc: Russell King <linux@arm.linux.org.uk> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>> Cc: Will Deacon <will.deacon@arm.com> >>>> >>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>> --- >>>> arch/arm/include/asm/memory.h | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h >>>> index 4dd2145..412da47 100644 >>>> --- a/arch/arm/include/asm/memory.h >>>> +++ b/arch/arm/include/asm/memory.h >>>> @@ -343,8 +343,7 @@ static inline __deprecated void *bus_to_virt(unsigned long x) >>>> #define ARCH_PFN_OFFSET PHYS_PFN_OFFSET >>>> >>>> #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT) >>>> -#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET && (unsigned long)(kaddr) < (unsigned long)high_memory) >>>> - >>>> +#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET) >>>> #endif >>>> >>>> #include <asm-generic/memory_model.h> >>>> -- >>>> 1.7.9.5 >>>> >>> >>> This looks wrong to me. Check Documentation/arm/memory.txt, this would >>> return true for the VMALLOC region, which would cause virt_to_phys to >>> give you something invalid, which would be bad. >>> >> I also thought it might not be right fix and hence added a disclaimer >> in the commit message ;-) >> > Yes I know, I'm not holding it against you ;) > > Not sure what the nicest fix is though. I think we can get away with > the single page restriction per call for Hyp mappings for now. IIRC the > Hyp code is still limited to be within a single page. > > Let's see what Marc has to say. So that may change quite quickly. I already have additional code for GICv3 in HYP, and I'd like to avoid putting restrictions that we'd have a hard time removing later. I'd like to try the approach I just sent out in a separate email first, and if it can't be made to work, we can see how to either enforce a single-page restriction or move the percpu stuff to kmalloc instead. M.
On Thu, Nov 14, 2013 at 02:37:43PM -0500, Santosh Shilimkar wrote: > Slab allocator can allocate memory beyond the lowmem watermark > which can lead to false failure of virt_addr_valid(). This is definitely going to cause problems. > So drop the check. The issue was seen with percpu_alloc() > in KVM code which was allocating memory beyond lowmem watermark. > > Am not completly sure whether this is the right fix and if it could > impact any other user of virt_addr_valid(). Without this fix as > pointed out the KVM init was failing in my testing. virt_addr_valid() gets used in some places to check whether the virtual address is part of the lowmem mapping and explicitly not part of vmalloc() or DMA coherent space: drivers/mtd/nand/gpmi-nand/gpmi-nand.c drivers/net/wireless/ath/ath6kl/sdio.c It opens up the checks in include/linux/scatterlist.h to start accepting non-streaming DMA capable buffers as well. It also bypasses a check in the slab code to ensure that it's a potentially valid pointer that was handed out by slab. .. etc ..
On Friday 15 November 2013 09:20 AM, Russell King - ARM Linux wrote: > On Thu, Nov 14, 2013 at 02:37:43PM -0500, Santosh Shilimkar wrote: >> Slab allocator can allocate memory beyond the lowmem watermark >> which can lead to false failure of virt_addr_valid(). > > This is definitely going to cause problems. > >> So drop the check. The issue was seen with percpu_alloc() >> in KVM code which was allocating memory beyond lowmem watermark. >> >> Am not completly sure whether this is the right fix and if it could >> impact any other user of virt_addr_valid(). Without this fix as >> pointed out the KVM init was failing in my testing. > > virt_addr_valid() gets used in some places to check whether the virtual > address is part of the lowmem mapping and explicitly not part of vmalloc() > or DMA coherent space: > > drivers/mtd/nand/gpmi-nand/gpmi-nand.c > drivers/net/wireless/ath/ath6kl/sdio.c > > It opens up the checks in include/linux/scatterlist.h to start accepting > non-streaming DMA capable buffers as well. > > It also bypasses a check in the slab code to ensure that it's a potentially > valid pointer that was handed out by slab. > Thanks for the additional information Russell. We are going with an alternate approach from Marc so this patch will be dropped any ways. Regards, Santosh
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h index 4dd2145..412da47 100644 --- a/arch/arm/include/asm/memory.h +++ b/arch/arm/include/asm/memory.h @@ -343,8 +343,7 @@ static inline __deprecated void *bus_to_virt(unsigned long x) #define ARCH_PFN_OFFSET PHYS_PFN_OFFSET #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT) -#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET && (unsigned long)(kaddr) < (unsigned long)high_memory) - +#define virt_addr_valid(kaddr) ((unsigned long)(kaddr) >= PAGE_OFFSET) #endif #include <asm-generic/memory_model.h>
Slab allocator can allocate memory beyond the lowmem watermark which can lead to false failure of virt_addr_valid(). So drop the check. The issue was seen with percpu_alloc() in KVM code which was allocating memory beyond lowmem watermark. Am not completly sure whether this is the right fix and if it could impact any other user of virt_addr_valid(). Without this fix as pointed out the KVM init was failing in my testing. Cc: Christoffer Dall <christoffer.dall@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Russell King <linux@arm.linux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> --- arch/arm/include/asm/memory.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)