| Message ID | 20190813170149.26037-2-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix issues with 52-bit kernel virtual addressing | expand |
On Tue, 13 Aug 2019 at 20:02, Will Deacon <will@kernel.org> wrote: > > virt_addr_valid() is intended to test whether or not the passed address > is a valid linear map address. Unfortunately, it relies on > _virt_addr_is_linear() which is broken because it assumes the linear > map is at the top of the address space, which it no longer is. > > Reimplement virt_addr_valid() using __is_lm_address() and remove > _virt_addr_is_linear() entirely. At the same time, ensure we evaluate > the macro parameter only once and move it within the __ASSEMBLY__ block. > > Reported-by: Qian Cai <cai@lca.pw> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") > Signed-off-by: Will Deacon <will@kernel.org> > --- > arch/arm64/include/asm/memory.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > index afaf512c0e1b..442ab861cab8 100644 > --- a/arch/arm64/include/asm/memory.h > +++ b/arch/arm64/include/asm/memory.h > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > /* > * The linear kernel range starts in the middle of the virtual adddress > * space. This is no longer true either. > Testing the top bit for the start of the region is a > - * sufficient check. > + * sufficient check and avoids having to worry about the tag. > */ > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > ... and this assumes that the VA space is split evenly between linear and vmalloc/vmemmap/etc, which is no longer true when running with 52-bit VAs > #define __lm_to_phys(addr) (((addr) + physvirt_offset)) > #define __kimg_to_phys(addr) ((addr) - kimage_voffset) > @@ -326,13 +326,13 @@ static inline void *phys_to_virt(phys_addr_t x) > > #define virt_to_page(vaddr) ((struct page *)((__virt_to_pgoff(vaddr)) + VMEMMAP_START)) > #endif > -#endif > > -#define _virt_addr_is_linear(kaddr) \ > - (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET) > +#define virt_addr_valid(addr) ({ \ > + __typeof__(addr) __addr = addr; \ > + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > +}) > > -#define virt_addr_valid(kaddr) \ > - (_virt_addr_is_linear(kaddr) && pfn_valid(virt_to_pfn(kaddr))) > +#endif > > /* > * Given that the GIC architecture permits ITS implementations that can only be > -- > 2.11.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 13, 2019 at 06:01:42PM +0100, Will Deacon wrote: > virt_addr_valid() is intended to test whether or not the passed address > is a valid linear map address. Unfortunately, it relies on > _virt_addr_is_linear() which is broken because it assumes the linear > map is at the top of the address space, which it no longer is. > > Reimplement virt_addr_valid() using __is_lm_address() and remove > _virt_addr_is_linear() entirely. At the same time, ensure we evaluate > the macro parameter only once and move it within the __ASSEMBLY__ block. > > Reported-by: Qian Cai <cai@lca.pw> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") > Signed-off-by: Will Deacon <will@kernel.org> Reviewed-by: Steve Capper <steve.capper@arm.com> > --- > arch/arm64/include/asm/memory.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > index afaf512c0e1b..442ab861cab8 100644 > --- a/arch/arm64/include/asm/memory.h > +++ b/arch/arm64/include/asm/memory.h > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > /* > * The linear kernel range starts in the middle of the virtual adddress > * space. Testing the top bit for the start of the region is a > - * sufficient check. > + * sufficient check and avoids having to worry about the tag. > */ > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > #define __lm_to_phys(addr) (((addr) + physvirt_offset)) > #define __kimg_to_phys(addr) ((addr) - kimage_voffset) > @@ -326,13 +326,13 @@ static inline void *phys_to_virt(phys_addr_t x) > > #define virt_to_page(vaddr) ((struct page *)((__virt_to_pgoff(vaddr)) + VMEMMAP_START)) > #endif > -#endif > > -#define _virt_addr_is_linear(kaddr) \ > - (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET) > +#define virt_addr_valid(addr) ({ \ > + __typeof__(addr) __addr = addr; \ > + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > +}) > > -#define virt_addr_valid(kaddr) \ > - (_virt_addr_is_linear(kaddr) && pfn_valid(virt_to_pfn(kaddr))) > +#endif > > /* > * Given that the GIC architecture permits ITS implementations that can only be > -- > 2.11.0 >
Hi Ard, On Tue, Aug 13, 2019 at 09:09:16PM +0300, Ard Biesheuvel wrote: > On Tue, 13 Aug 2019 at 20:02, Will Deacon <will@kernel.org> wrote: > > > > virt_addr_valid() is intended to test whether or not the passed address > > is a valid linear map address. Unfortunately, it relies on > > _virt_addr_is_linear() which is broken because it assumes the linear > > map is at the top of the address space, which it no longer is. > > > > Reimplement virt_addr_valid() using __is_lm_address() and remove > > _virt_addr_is_linear() entirely. At the same time, ensure we evaluate > > the macro parameter only once and move it within the __ASSEMBLY__ block. > > > > Reported-by: Qian Cai <cai@lca.pw> > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") > > Signed-off-by: Will Deacon <will@kernel.org> > > --- > > arch/arm64/include/asm/memory.h | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > index afaf512c0e1b..442ab861cab8 100644 > > --- a/arch/arm64/include/asm/memory.h > > +++ b/arch/arm64/include/asm/memory.h > > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > /* > > * The linear kernel range starts in the middle of the virtual adddress > > * space. > > This is no longer true either. > Whoops agreed. > > Testing the top bit for the start of the region is a > > - * sufficient check. > > + * sufficient check and avoids having to worry about the tag. > > */ > > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > > > ... and this assumes that the VA space is split evenly between linear > and vmalloc/vmemmap/etc, which is no longer true when running with > 52-bit VAs > For 52-bit VAs we have two possibilities: Start End Size Use ----------------------------------------------------------------------- 0000000000000000 000fffffffffffff 4PB user fff0000000000000 fff7ffffffffffff 2PB kernel logical memory map fff8000000000000 fffd9fffffffffff 1440TB [gap] fffda00000000000 ffff9fffffffffff 512TB kasan shadow region and Start End Size Use ----------------------------------------------------------------------- 0000000000000000 0000ffffffffffff 256TB user ffff000000000000 ffff7fffffffffff 128TB kernel logical memory map ffff800000000000 ffff9fffffffffff 32TB kasan shadow region ffffa00000000000 ffffa00007ffffff 128MB bpf jit region IIUC the definition for __is_lm_address is correct for these cases? (it's based off vabits_actual). Cheers,
On Tue, 13 Aug 2019 at 22:11, Steve Capper <Steve.Capper@arm.com> wrote: > > Hi Ard, > > On Tue, Aug 13, 2019 at 09:09:16PM +0300, Ard Biesheuvel wrote: > > On Tue, 13 Aug 2019 at 20:02, Will Deacon <will@kernel.org> wrote: > > > > > > virt_addr_valid() is intended to test whether or not the passed address > > > is a valid linear map address. Unfortunately, it relies on > > > _virt_addr_is_linear() which is broken because it assumes the linear > > > map is at the top of the address space, which it no longer is. > > > > > > Reimplement virt_addr_valid() using __is_lm_address() and remove > > > _virt_addr_is_linear() entirely. At the same time, ensure we evaluate > > > the macro parameter only once and move it within the __ASSEMBLY__ block. > > > > > > Reported-by: Qian Cai <cai@lca.pw> > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") > > > Signed-off-by: Will Deacon <will@kernel.org> > > > --- > > > arch/arm64/include/asm/memory.h | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > > index afaf512c0e1b..442ab861cab8 100644 > > > --- a/arch/arm64/include/asm/memory.h > > > +++ b/arch/arm64/include/asm/memory.h > > > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > > /* > > > * The linear kernel range starts in the middle of the virtual adddress > > > * space. > > > > This is no longer true either. > > > > Whoops agreed. > > > > Testing the top bit for the start of the region is a > > > - * sufficient check. > > > + * sufficient check and avoids having to worry about the tag. > > > */ > > > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > > > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > > > > > > ... and this assumes that the VA space is split evenly between linear > > and vmalloc/vmemmap/etc, which is no longer true when running with > > 52-bit VAs > > > > For 52-bit VAs we have two possibilities: > Start End Size Use > ----------------------------------------------------------------------- > 0000000000000000 000fffffffffffff 4PB user > fff0000000000000 fff7ffffffffffff 2PB kernel logical memory map > fff8000000000000 fffd9fffffffffff 1440TB [gap] Right. I missed the part where we throw away 1/3 of the VA space: IIRC, the idea was that keeping the size of the upper half of the 48-bit VA space fixed for 52-bit not only allowed compile time constant addresses to be used for many of the things that populate it, it also makes a lot more VA space available to the linear region, which is where we need it the most. > fffda00000000000 ffff9fffffffffff 512TB kasan shadow region > > and > Start End Size Use > ----------------------------------------------------------------------- > 0000000000000000 0000ffffffffffff 256TB user > ffff000000000000 ffff7fffffffffff 128TB kernel logical memory map > ffff800000000000 ffff9fffffffffff 32TB kasan shadow region > ffffa00000000000 ffffa00007ffffff 128MB bpf jit region > > IIUC the definition for __is_lm_address is correct for these cases? > (it's based off vabits_actual). > With the gap taken into account, it is correct. But throwing away 1440 TB of address space seems suboptimal to me.
On Tue, Aug 13, 2019 at 10:25:14PM +0300, Ard Biesheuvel wrote: > On Tue, 13 Aug 2019 at 22:11, Steve Capper <Steve.Capper@arm.com> wrote: > > > > Hi Ard, > > > > On Tue, Aug 13, 2019 at 09:09:16PM +0300, Ard Biesheuvel wrote: > > > On Tue, 13 Aug 2019 at 20:02, Will Deacon <will@kernel.org> wrote: > > > > > > > > virt_addr_valid() is intended to test whether or not the passed address > > > > is a valid linear map address. Unfortunately, it relies on > > > > _virt_addr_is_linear() which is broken because it assumes the linear > > > > map is at the top of the address space, which it no longer is. > > > > > > > > Reimplement virt_addr_valid() using __is_lm_address() and remove > > > > _virt_addr_is_linear() entirely. At the same time, ensure we evaluate > > > > the macro parameter only once and move it within the __ASSEMBLY__ block. > > > > > > > > Reported-by: Qian Cai <cai@lca.pw> > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") > > > > Signed-off-by: Will Deacon <will@kernel.org> > > > > --- > > > > arch/arm64/include/asm/memory.h | 14 +++++++------- > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > > > index afaf512c0e1b..442ab861cab8 100644 > > > > --- a/arch/arm64/include/asm/memory.h > > > > +++ b/arch/arm64/include/asm/memory.h > > > > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > > > /* > > > > * The linear kernel range starts in the middle of the virtual adddress > > > > * space. > > > > > > This is no longer true either. > > > > > > > Whoops agreed. > > > > > > Testing the top bit for the start of the region is a > > > > - * sufficient check. > > > > + * sufficient check and avoids having to worry about the tag. > > > > */ > > > > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > > > > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > > > > > > > > > ... and this assumes that the VA space is split evenly between linear > > > and vmalloc/vmemmap/etc, which is no longer true when running with > > > 52-bit VAs > > > > > > > For 52-bit VAs we have two possibilities: > > Start End Size Use > > ----------------------------------------------------------------------- > > 0000000000000000 000fffffffffffff 4PB user > > fff0000000000000 fff7ffffffffffff 2PB kernel logical memory map > > fff8000000000000 fffd9fffffffffff 1440TB [gap] > > Right. I missed the part where we throw away 1/3 of the VA space: > IIRC, the idea was that keeping the size of the upper half of the > 48-bit VA space fixed for 52-bit not only allowed compile time > constant addresses to be used for many of the things that populate it, > it also makes a lot more VA space available to the linear region, > which is where we need it the most. > > > fffda00000000000 ffff9fffffffffff 512TB kasan shadow region > > > > and > > Start End Size Use > > ----------------------------------------------------------------------- > > 0000000000000000 0000ffffffffffff 256TB user > > ffff000000000000 ffff7fffffffffff 128TB kernel logical memory map > > ffff800000000000 ffff9fffffffffff 32TB kasan shadow region > > ffffa00000000000 ffffa00007ffffff 128MB bpf jit region > > > > IIUC the definition for __is_lm_address is correct for these cases? > > (it's based off vabits_actual). > > > > With the gap taken into account, it is correct. But throwing away 1440 > TB of address space seems suboptimal to me. When getting the 52-bit kernel VA support ready, I was trying to achieve functional and performant support in as few steps as possible to avoid risk of breaking things (unfortunately I missed a couple of things between rebases with the SW KASAN). The big gain from that series is support for a much larger linear mapping. The best way I can think of to get rid of the gap is to use it for vmalloc space which means changes to VMALLOC_START and VMALLOC_END. I think it would be better to make this change incrementally and I'm more than happy to get hacking on a patch. Or maybe there's a better use for the gap in other areas... Cheers,
On Tue, Aug 13, 2019 at 07:11:26PM +0000, Steve Capper wrote: > On Tue, Aug 13, 2019 at 09:09:16PM +0300, Ard Biesheuvel wrote: > > On Tue, 13 Aug 2019 at 20:02, Will Deacon <will@kernel.org> wrote: > > > > > > virt_addr_valid() is intended to test whether or not the passed address > > > is a valid linear map address. Unfortunately, it relies on > > > _virt_addr_is_linear() which is broken because it assumes the linear > > > map is at the top of the address space, which it no longer is. > > > > > > Reimplement virt_addr_valid() using __is_lm_address() and remove > > > _virt_addr_is_linear() entirely. At the same time, ensure we evaluate > > > the macro parameter only once and move it within the __ASSEMBLY__ block. > > > > > > Reported-by: Qian Cai <cai@lca.pw> > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") > > > Signed-off-by: Will Deacon <will@kernel.org> > > > --- > > > arch/arm64/include/asm/memory.h | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > > index afaf512c0e1b..442ab861cab8 100644 > > > --- a/arch/arm64/include/asm/memory.h > > > +++ b/arch/arm64/include/asm/memory.h > > > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > > /* > > > * The linear kernel range starts in the middle of the virtual adddress > > > * space. > > > > This is no longer true either. > > > > Whoops agreed. Bah, stupid comment. Dunno how I missed that when I was editing it. I'll change "starts in the middle" to be "starts at the bottom". We can look at the wasted VA space issue as part of a seperate series, since I'd rather not hold the current patches up on that. Will
On Tue, Aug 13, 2019 at 06:01:42PM +0100, Will Deacon wrote: > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > index afaf512c0e1b..442ab861cab8 100644 > --- a/arch/arm64/include/asm/memory.h > +++ b/arch/arm64/include/asm/memory.h > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > /* > * The linear kernel range starts in the middle of the virtual adddress > * space. Testing the top bit for the start of the region is a > - * sufficient check. > + * sufficient check and avoids having to worry about the tag. > */ > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > #define __lm_to_phys(addr) (((addr) + physvirt_offset)) > #define __kimg_to_phys(addr) ((addr) - kimage_voffset) > @@ -326,13 +326,13 @@ static inline void *phys_to_virt(phys_addr_t x) > > #define virt_to_page(vaddr) ((struct page *)((__virt_to_pgoff(vaddr)) + VMEMMAP_START)) > #endif > -#endif > > -#define _virt_addr_is_linear(kaddr) \ > - (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET) > +#define virt_addr_valid(addr) ({ \ > + __typeof__(addr) __addr = addr; \ > + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > +}) There is a slight change of semantics here but I don't think it's an issue currently. __is_lm_address() is true even for a user address, so at least the first part of virt_addr_valid() now accepts such addresses. The pfn would be wrong eventually because of the virt-to-phys offsetting and pfn_valid() false but we rely on this rather than checking it's a kernel address. Slight concern as this macro is called from drivers. Should we keep the PAGE_OFFSET check as well?
On Wed, Aug 14, 2019 at 10:19:42AM +0100, Catalin Marinas wrote: > On Tue, Aug 13, 2019 at 06:01:42PM +0100, Will Deacon wrote: > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > index afaf512c0e1b..442ab861cab8 100644 > > --- a/arch/arm64/include/asm/memory.h > > +++ b/arch/arm64/include/asm/memory.h > > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > /* > > * The linear kernel range starts in the middle of the virtual adddress > > * space. Testing the top bit for the start of the region is a > > - * sufficient check. > > + * sufficient check and avoids having to worry about the tag. > > */ > > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > > > #define __lm_to_phys(addr) (((addr) + physvirt_offset)) > > #define __kimg_to_phys(addr) ((addr) - kimage_voffset) > > @@ -326,13 +326,13 @@ static inline void *phys_to_virt(phys_addr_t x) > > > > #define virt_to_page(vaddr) ((struct page *)((__virt_to_pgoff(vaddr)) + VMEMMAP_START)) > > #endif > > -#endif > > > > -#define _virt_addr_is_linear(kaddr) \ > > - (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET) > > +#define virt_addr_valid(addr) ({ \ > > + __typeof__(addr) __addr = addr; \ > > + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > > +}) > > There is a slight change of semantics here but I don't think it's an > issue currently. __is_lm_address() is true even for a user address, so > at least the first part of virt_addr_valid() now accepts such addresses. > The pfn would be wrong eventually because of the virt-to-phys offsetting > and pfn_valid() false but we rely on this rather than checking it's a > kernel address. Slight concern as this macro is called from drivers. > > Should we keep the PAGE_OFFSET check as well? In virt_addr_valid() or __is_lm_address()? To be honest with you, I'm not even sure what virt_addr_valid() is supposed to do with non-linear kernel addresses: look at powerpc and riscv, who appear to convert the address straight to a pfn. Many callers check against is_vmalloc_addr() first, but not all of them. I think passing in a *user* address would be a huge bug in the caller, just like it would be if you called virt_to_phys() on a user address. If we care about that, then I think __is_lm_address() should be the one doing the check against PAGE_OFFSET. Thoughts? I'd be inclined to leave this patch as it is. Will
On Wed, Aug 14, 2019 at 10:48:20AM +0100, Will Deacon wrote: > On Wed, Aug 14, 2019 at 10:19:42AM +0100, Catalin Marinas wrote: > > On Tue, Aug 13, 2019 at 06:01:42PM +0100, Will Deacon wrote: > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > > index afaf512c0e1b..442ab861cab8 100644 > > > --- a/arch/arm64/include/asm/memory.h > > > +++ b/arch/arm64/include/asm/memory.h > > > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > > /* > > > * The linear kernel range starts in the middle of the virtual adddress > > > * space. Testing the top bit for the start of the region is a > > > - * sufficient check. > > > + * sufficient check and avoids having to worry about the tag. > > > */ > > > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > > > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > > > > > #define __lm_to_phys(addr) (((addr) + physvirt_offset)) > > > #define __kimg_to_phys(addr) ((addr) - kimage_voffset) > > > @@ -326,13 +326,13 @@ static inline void *phys_to_virt(phys_addr_t x) > > > > > > #define virt_to_page(vaddr) ((struct page *)((__virt_to_pgoff(vaddr)) + VMEMMAP_START)) > > > #endif > > > -#endif > > > > > > -#define _virt_addr_is_linear(kaddr) \ > > > - (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET) > > > +#define virt_addr_valid(addr) ({ \ > > > + __typeof__(addr) __addr = addr; \ > > > + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > > > +}) > > > > There is a slight change of semantics here but I don't think it's an > > issue currently. __is_lm_address() is true even for a user address, so > > at least the first part of virt_addr_valid() now accepts such addresses. > > The pfn would be wrong eventually because of the virt-to-phys offsetting > > and pfn_valid() false but we rely on this rather than checking it's a > > kernel address. Slight concern as this macro is called from drivers. > > > > Should we keep the PAGE_OFFSET check as well? > > In virt_addr_valid() or __is_lm_address()? > > To be honest with you, I'm not even sure what virt_addr_valid() is supposed > to do with non-linear kernel addresses: look at powerpc and riscv, who > appear to convert the address straight to a pfn. Many callers check against > is_vmalloc_addr() first, but not all of them. Even if they call is_vmalloc_addr(), it would return false for user address. Anyway, at a quick look, I couldn't find any virt_addr_valid() where it would be an issue. > I think passing in a *user* address would be a huge bug in the caller, > just like it would be if you called virt_to_phys() on a user address. > If we care about that, then I think __is_lm_address() should be the one > doing the check against PAGE_OFFSET. > > Thoughts? I'd be inclined to leave this patch as it is. Leave it as it is. The way pfn_valid() is written it wouldn't return true for a user address due to the fact that virt_to_phys() cannot return the same physical address for a user and linear map one. For this patch: Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
On Wed, Aug 14, 2019 at 11:40:23AM +0100, Catalin Marinas wrote: > On Wed, Aug 14, 2019 at 10:48:20AM +0100, Will Deacon wrote: > > On Wed, Aug 14, 2019 at 10:19:42AM +0100, Catalin Marinas wrote: > > > On Tue, Aug 13, 2019 at 06:01:42PM +0100, Will Deacon wrote: > > > > -#define _virt_addr_is_linear(kaddr) \ > > > > - (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET) > > > > +#define virt_addr_valid(addr) ({ \ > > > > + __typeof__(addr) __addr = addr; \ > > > > + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ > > > > +}) > > > > > > There is a slight change of semantics here but I don't think it's an > > > issue currently. __is_lm_address() is true even for a user address, so > > > at least the first part of virt_addr_valid() now accepts such addresses. > > > The pfn would be wrong eventually because of the virt-to-phys offsetting > > > and pfn_valid() false but we rely on this rather than checking it's a > > > kernel address. Slight concern as this macro is called from drivers. > > > > > > Should we keep the PAGE_OFFSET check as well? > > > > In virt_addr_valid() or __is_lm_address()? > > > > To be honest with you, I'm not even sure what virt_addr_valid() is supposed > > to do with non-linear kernel addresses: look at powerpc and riscv, who > > appear to convert the address straight to a pfn. Many callers check against > > is_vmalloc_addr() first, but not all of them. > > Even if they call is_vmalloc_addr(), it would return false for user > address. Anyway, at a quick look, I couldn't find any virt_addr_valid() > where it would be an issue. Sure, but my point was more that it would be crazy to have a macro that accepted user and linear addresses, but not vmalloc addresses! > > I think passing in a *user* address would be a huge bug in the caller, > > just like it would be if you called virt_to_phys() on a user address. > > If we care about that, then I think __is_lm_address() should be the one > > doing the check against PAGE_OFFSET. > > > > Thoughts? I'd be inclined to leave this patch as it is. > > Leave it as it is. The way pfn_valid() is written it wouldn't return > true for a user address due to the fact that virt_to_phys() cannot > return the same physical address for a user and linear map one. > > For this patch: > > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> Ta, Will
On Tue, 13 Aug 2019 at 23:34, Steve Capper <Steve.Capper@arm.com> wrote: > > On Tue, Aug 13, 2019 at 10:25:14PM +0300, Ard Biesheuvel wrote: > > On Tue, 13 Aug 2019 at 22:11, Steve Capper <Steve.Capper@arm.com> wrote: > > > > > > Hi Ard, > > > > > > On Tue, Aug 13, 2019 at 09:09:16PM +0300, Ard Biesheuvel wrote: > > > > On Tue, 13 Aug 2019 at 20:02, Will Deacon <will@kernel.org> wrote: > > > > > > > > > > virt_addr_valid() is intended to test whether or not the passed address > > > > > is a valid linear map address. Unfortunately, it relies on > > > > > _virt_addr_is_linear() which is broken because it assumes the linear > > > > > map is at the top of the address space, which it no longer is. > > > > > > > > > > Reimplement virt_addr_valid() using __is_lm_address() and remove > > > > > _virt_addr_is_linear() entirely. At the same time, ensure we evaluate > > > > > the macro parameter only once and move it within the __ASSEMBLY__ block. > > > > > > > > > > Reported-by: Qian Cai <cai@lca.pw> > > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") > > > > > Signed-off-by: Will Deacon <will@kernel.org> > > > > > --- > > > > > arch/arm64/include/asm/memory.h | 14 +++++++------- > > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > > > > > index afaf512c0e1b..442ab861cab8 100644 > > > > > --- a/arch/arm64/include/asm/memory.h > > > > > +++ b/arch/arm64/include/asm/memory.h > > > > > @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > > > > /* > > > > > * The linear kernel range starts in the middle of the virtual adddress > > > > > * space. > > > > > > > > This is no longer true either. > > > > > > > > > > Whoops agreed. > > > > > > > > Testing the top bit for the start of the region is a > > > > > - * sufficient check. > > > > > + * sufficient check and avoids having to worry about the tag. > > > > > */ > > > > > -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) > > > > > +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) > > > > > > > > > > > > > ... and this assumes that the VA space is split evenly between linear > > > > and vmalloc/vmemmap/etc, which is no longer true when running with > > > > 52-bit VAs > > > > > > > > > > For 52-bit VAs we have two possibilities: > > > Start End Size Use > > > ----------------------------------------------------------------------- > > > 0000000000000000 000fffffffffffff 4PB user > > > fff0000000000000 fff7ffffffffffff 2PB kernel logical memory map > > > fff8000000000000 fffd9fffffffffff 1440TB [gap] > > > > Right. I missed the part where we throw away 1/3 of the VA space: > > IIRC, the idea was that keeping the size of the upper half of the > > 48-bit VA space fixed for 52-bit not only allowed compile time > > constant addresses to be used for many of the things that populate it, > > it also makes a lot more VA space available to the linear region, > > which is where we need it the most. > > > > > fffda00000000000 ffff9fffffffffff 512TB kasan shadow region > > > > > > and > > > Start End Size Use > > > ----------------------------------------------------------------------- > > > 0000000000000000 0000ffffffffffff 256TB user > > > ffff000000000000 ffff7fffffffffff 128TB kernel logical memory map > > > ffff800000000000 ffff9fffffffffff 32TB kasan shadow region > > > ffffa00000000000 ffffa00007ffffff 128MB bpf jit region > > > > > > IIUC the definition for __is_lm_address is correct for these cases? > > > (it's based off vabits_actual). > > > > > > > With the gap taken into account, it is correct. But throwing away 1440 > > TB of address space seems suboptimal to me. > > When getting the 52-bit kernel VA support ready, I was trying to achieve > functional and performant support in as few steps as possible to avoid risk of > breaking things (unfortunately I missed a couple of things between > rebases with the SW KASAN). The big gain from that series is support for > a much larger linear mapping. > Sure. > The best way I can think of to get rid of the gap is to use it for > vmalloc space which means changes to VMALLOC_START and VMALLOC_END. I > think it would be better to make this change incrementally and I'm more > than happy to get hacking on a patch. Or maybe there's a better use for > the gap in other areas... > Given that it is only the MMU hardware that is preventing us from running a 52-bit VA kernel on 48-bit VA hardware, I still don't follow 100% why we have all these moving parts. If we start the address space at 0xfff0_... and start the vmalloc/vmemmap/etc space at 0xffff_8.... (and make sure the vmemmap space is sized so it can cover the entire span), all we should need to do is make sure we don't map any memory below 0xffff_0... if the hardware does not support that. I don't think there is any need to make the start of physical memory coincide with the lowest kernel VA supported by the hardware, or have other variable quantities that assume different values based on the actual h/w config. [Apologies for not bringing this up earlier. I am not actually working atm :-)]
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index afaf512c0e1b..442ab861cab8 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -244,9 +244,9 @@ static inline const void *__tag_set(const void *addr, u8 tag) /* * The linear kernel range starts in the middle of the virtual adddress * space. Testing the top bit for the start of the region is a - * sufficient check. + * sufficient check and avoids having to worry about the tag. */ -#define __is_lm_address(addr) (!((addr) & BIT(vabits_actual - 1))) +#define __is_lm_address(addr) (!(((u64)addr) & BIT(vabits_actual - 1))) #define __lm_to_phys(addr) (((addr) + physvirt_offset)) #define __kimg_to_phys(addr) ((addr) - kimage_voffset) @@ -326,13 +326,13 @@ static inline void *phys_to_virt(phys_addr_t x) #define virt_to_page(vaddr) ((struct page *)((__virt_to_pgoff(vaddr)) + VMEMMAP_START)) #endif -#endif -#define _virt_addr_is_linear(kaddr) \ - (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET) +#define virt_addr_valid(addr) ({ \ + __typeof__(addr) __addr = addr; \ + __is_lm_address(__addr) && pfn_valid(virt_to_pfn(__addr)); \ +}) -#define virt_addr_valid(kaddr) \ - (_virt_addr_is_linear(kaddr) && pfn_valid(virt_to_pfn(kaddr))) +#endif /* * Given that the GIC architecture permits ITS implementations that can only be
virt_addr_valid() is intended to test whether or not the passed address is a valid linear map address. Unfortunately, it relies on _virt_addr_is_linear() which is broken because it assumes the linear map is at the top of the address space, which it no longer is. Reimplement virt_addr_valid() using __is_lm_address() and remove _virt_addr_is_linear() entirely. At the same time, ensure we evaluate the macro parameter only once and move it within the __ASSEMBLY__ block. Reported-by: Qian Cai <cai@lca.pw> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Fixes: 14c127c957c1 ("arm64: mm: Flip kernel VA space") Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/include/asm/memory.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)