[1/8] arm64: memory: Fix virt_addr_valid() using __is_lm_address()
diff mbox series

Message ID 20190813170149.26037-2-will@kernel.org
State New
Headers show
Series
  • Fix issues with 52-bit kernel virtual addressing
Related show

Commit Message

Will Deacon Aug. 13, 2019, 5:01 p.m. UTC
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(-)

Comments

Ard Biesheuvel Aug. 13, 2019, 6:09 p.m. UTC | #1
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
Steve Capper Aug. 13, 2019, 6:53 p.m. UTC | #2
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
>
Steve Capper Aug. 13, 2019, 7:11 p.m. UTC | #3
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,
Ard Biesheuvel Aug. 13, 2019, 7:25 p.m. UTC | #4
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.
Steve Capper Aug. 13, 2019, 8:34 p.m. UTC | #5
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,
Will Deacon Aug. 14, 2019, 8:32 a.m. UTC | #6
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
Catalin Marinas Aug. 14, 2019, 9:19 a.m. UTC | #7
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?
Will Deacon Aug. 14, 2019, 9:48 a.m. UTC | #8
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
Catalin Marinas Aug. 14, 2019, 10:40 a.m. UTC | #9
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>
Will Deacon Aug. 14, 2019, 12:02 p.m. UTC | #10
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
Ard Biesheuvel Aug. 14, 2019, 3:17 p.m. UTC | #11
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 :-)]

Patch
diff mbox series

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