[PATCH 0/3] RISC-V: mm: correct mmap behavior in sv48 address space

[PATCH 0/3] RISC-V: mm: correct mmap behavior in sv48 address space

[Yangyu Chen]

Previous patch series [1] violates the principle of mmap syscall as it uses
hint address as the largest address space to use rather than where to
create the mapping, thus broke the possibility to mmap in sv48, sv57
address space without a MAP_FIXED flag. This patchset corrects the behavior
of mmap syscall and use the behavior of x86 5-stage-paging as a reference.

I first noticed this issue when I was trying to run box64 on a sv48 system
with commit previous than [2]. Then I reported this through private
communication, then a box64 contributor did some investigation and found
that trying to mmap in sv48 address space without MAP_FIXED flag will
always return a random address in sv39. I review the changelog with some
tests on qemu and found this issue was introduced from [1]. After reviewing
the code, tests and docs, I think the original author might misunderstand
the meaning of hint address in mmap syscall. Then I did some investigation
on other ISAs like x86 which has 5-stage-paging and found that it has
addressed the same issue if some userspace software assumes the pointer
size should smaller than 47 bits and also solved in kernel by limiting the
mmap in maximum 47 bits address space by default.

Finally I correct the behavior of mmap syscall as x86 5-stage-paging does,
and migreate the documentation from x86-64 kernel to riscv kernel.


[1]. https://lore.kernel.org/linux-riscv/20230809232218.849726-1-charlie@rivosinc.com/
[2]. https://github.com/ptitSeb/box64/commit/5b700cb6e6f397d2074c49659f7f9915f4a33c5f

Yangyu Chen (3):
  RISC-V: mm: fix mmap behavior in sv48 address space
  RISC-V: mm: only test mmap without hint
  Documentation: riscv: correct sv57 kernel behavior

 Documentation/arch/riscv/vm-layout.rst        | 48 +++++++++++--------
 arch/riscv/include/asm/processor.h            | 39 ++++-----------
 .../selftests/riscv/mm/mmap_bottomup.c        | 12 -----
 .../testing/selftests/riscv/mm/mmap_default.c | 12 -----
 tools/testing/selftests/riscv/mm/mmap_test.h  | 30 ------------
 5 files changed, 36 insertions(+), 105 deletions(-)

-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/3] RISC-V: mm: fix mmap behavior in sv48 address space

[Yangyu Chen]

A commit add2cc6b6515 ("RISC-V: mm: Restrict address space for
sv39,sv48,sv57") from patch[1] restricts regular mmap return address in the
sv48 space if the address hint is not above the sv48 userspace address.
However, this commit treats the address wrong which only use sv48 if the
hint address is above sv48 user address space. Actually, it should use sv48
if the address is above sv39 user address space. Moreover, the original
patch code looks very complex in logic, we can simplify it with min marco.

[1]. https://lore.kernel.org/r/20230809232218.849726-2-charlie@rivosinc.com

Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 arch/riscv/include/asm/processor.h | 39 ++++++------------------------
 1 file changed, 8 insertions(+), 31 deletions(-)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index e1944ff0757a..7ead6a3e1f12 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -9,6 +9,7 @@
 #include <linux/const.h>
 #include <linux/cache.h>
 #include <linux/prctl.h>
+#include <linux/minmax.h>
 
 #include <vdso/processor.h>
 
@@ -18,37 +19,13 @@
 #define DEFAULT_MAP_WINDOW	(UL(1) << (MMAP_VA_BITS - 1))
 #define STACK_TOP_MAX		TASK_SIZE
 
-#define arch_get_mmap_end(addr, len, flags)			\
-({								\
-	unsigned long mmap_end;					\
-	typeof(addr) _addr = (addr);				\
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
-		mmap_end = STACK_TOP_MAX;			\
-	else if ((_addr) >= VA_USER_SV57)			\
-		mmap_end = STACK_TOP_MAX;			\
-	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
-		mmap_end = VA_USER_SV48;			\
-	else							\
-		mmap_end = VA_USER_SV39;			\
-	mmap_end;						\
-})
-
-#define arch_get_mmap_base(addr, base)				\
-({								\
-	unsigned long mmap_base;				\
-	typeof(addr) _addr = (addr);				\
-	typeof(base) _base = (base);				\
-	unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);	\
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
-		mmap_base = (_base);				\
-	else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
-		mmap_base = VA_USER_SV57 - rnd_gap;		\
-	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
-		mmap_base = VA_USER_SV48 - rnd_gap;		\
-	else							\
-		mmap_base = VA_USER_SV39 - rnd_gap;		\
-	mmap_base;						\
-})
+#define arch_get_mmap_end(addr, len, flags) \
+	((addr) >= DEFAULT_MAP_WINDOW ? STACK_TOP_MAX :\
+	 min(DEFAULT_MAP_WINDOW, STACK_TOP_MAX))
+
+#define arch_get_mmap_base(addr, base) \
+	((addr) >= DEFAULT_MAP_WINDOW ? base :\
+	 min(base, DEFAULT_MAP_WINDOW))
 
 #else
 #define DEFAULT_MAP_WINDOW	TASK_SIZE
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/3] RISC-V: mm: only test mmap without hint

[Yangyu Chen]

The original test from a patch violates the principle of mmap which uses
mmap hint address as the largest address space to use rather than where to
create the mapping. The address space in this context is either sv39, sv48,
sv57.

After fixing the correct behavior, only keeping the test mmap without a
hint is enough.

Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 .../selftests/riscv/mm/mmap_bottomup.c        | 12 --------
 .../testing/selftests/riscv/mm/mmap_default.c | 12 --------
 tools/testing/selftests/riscv/mm/mmap_test.h  | 30 -------------------
 3 files changed, 54 deletions(-)

diff --git a/tools/testing/selftests/riscv/mm/mmap_bottomup.c b/tools/testing/selftests/riscv/mm/mmap_bottomup.c
index 1757d19ca89b..1ba703d3f552 100644
--- a/tools/testing/selftests/riscv/mm/mmap_bottomup.c
+++ b/tools/testing/selftests/riscv/mm/mmap_bottomup.c
@@ -15,20 +15,8 @@ TEST(infinite_rlimit)
 	do_mmaps(&mmap_addresses);
 
 	EXPECT_NE(MAP_FAILED, mmap_addresses.no_hint);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_37_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_38_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_46_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_47_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_55_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_56_addr);
 
 	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.no_hint);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_37_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_38_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_46_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_47_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_55_addr);
-	EXPECT_GT(1UL << 56, (unsigned long)mmap_addresses.on_56_addr);
 #endif
 }
 
diff --git a/tools/testing/selftests/riscv/mm/mmap_default.c b/tools/testing/selftests/riscv/mm/mmap_default.c
index c63c60b9397e..f1ac860dcf04 100644
--- a/tools/testing/selftests/riscv/mm/mmap_default.c
+++ b/tools/testing/selftests/riscv/mm/mmap_default.c
@@ -15,20 +15,8 @@ TEST(default_rlimit)
 	do_mmaps(&mmap_addresses);
 
 	EXPECT_NE(MAP_FAILED, mmap_addresses.no_hint);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_37_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_38_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_46_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_47_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_55_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_56_addr);
 
 	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.no_hint);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_37_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_38_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_46_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_47_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_55_addr);
-	EXPECT_GT(1UL << 56, (unsigned long)mmap_addresses.on_56_addr);
 #endif
 }
 
diff --git a/tools/testing/selftests/riscv/mm/mmap_test.h b/tools/testing/selftests/riscv/mm/mmap_test.h
index 2e0db9c5be6c..d2271426288f 100644
--- a/tools/testing/selftests/riscv/mm/mmap_test.h
+++ b/tools/testing/selftests/riscv/mm/mmap_test.h
@@ -10,47 +10,17 @@
 
 struct addresses {
 	int *no_hint;
-	int *on_37_addr;
-	int *on_38_addr;
-	int *on_46_addr;
-	int *on_47_addr;
-	int *on_55_addr;
-	int *on_56_addr;
 };
 
 // Only works on 64 bit
 #if __riscv_xlen == 64
 static inline void do_mmaps(struct addresses *mmap_addresses)
 {
-	/*
-	 * Place all of the hint addresses on the boundaries of mmap
-	 * sv39, sv48, sv57
-	 * User addresses end at 1<<38, 1<<47, 1<<56 respectively
-	 */
-	void *on_37_bits = (void *)(1UL << 37);
-	void *on_38_bits = (void *)(1UL << 38);
-	void *on_46_bits = (void *)(1UL << 46);
-	void *on_47_bits = (void *)(1UL << 47);
-	void *on_55_bits = (void *)(1UL << 55);
-	void *on_56_bits = (void *)(1UL << 56);
-
 	int prot = PROT_READ | PROT_WRITE;
 	int flags = MAP_PRIVATE | MAP_ANONYMOUS;
 
 	mmap_addresses->no_hint =
 		mmap(NULL, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_37_addr =
-		mmap(on_37_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_38_addr =
-		mmap(on_38_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_46_addr =
-		mmap(on_46_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_47_addr =
-		mmap(on_47_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_55_addr =
-		mmap(on_55_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_56_addr =
-		mmap(on_56_bits, 5 * sizeof(int), prot, flags, 0, 0);
 }
 #endif /* __riscv_xlen == 64 */
 
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/3] Documentation: riscv: correct sv57 kernel behavior

[Yangyu Chen]

The original documentation from a patch violates the principle of mmap.
Since the kernel behavior has been corrected from the previous patch, this
documentation should also be updated. This patch migrated the
5-level-paging documentation from x86_64 with minor modifications to align
with the current kernel's behavior on RISC-V.

Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 Documentation/arch/riscv/vm-layout.rst | 48 +++++++++++++++-----------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/Documentation/arch/riscv/vm-layout.rst b/Documentation/arch/riscv/vm-layout.rst
index 69ff6da1dbf8..30e879dad6a2 100644
--- a/Documentation/arch/riscv/vm-layout.rst
+++ b/Documentation/arch/riscv/vm-layout.rst
@@ -135,23 +135,31 @@ RISC-V Linux Kernel SV57
   __________________|____________|__________________|_________|____________________________________________________________
 
 
-Userspace VAs
---------------------
-To maintain compatibility with software that relies on the VA space with a
-maximum of 48 bits the kernel will, by default, return virtual addresses to
-userspace from a 48-bit range (sv48). This default behavior is achieved by
-passing 0 into the hint address parameter of mmap. On CPUs with an address space
-smaller than sv48, the CPU maximum supported address space will be the default.
-
-Software can "opt-in" to receiving VAs from another VA space by providing
-a hint address to mmap. A hint address passed to mmap will cause the largest
-address space that fits entirely into the hint to be used, unless there is no
-space left in the address space. If there is no space available in the requested
-address space, an address in the next smallest available address space will be
-returned.
-
-For example, in order to obtain 48-bit VA space, a hint address greater than
-:code:`1 << 47` must be provided. Note that this is 47 due to sv48 userspace
-ending at :code:`1 << 47` and the addresses beyond this are reserved for the
-kernel. Similarly, to obtain 57-bit VA space addresses, a hint address greater
-than or equal to :code:`1 << 56` must be provided.
+User-space and large virtual address space
+==========================================
+On RISC-V, Sv57 paging enables 56-bit userspace virtual address space.
+Not all user space is ready to handle wide addresses. It's known that
+at least some JIT compilers use higher bits in pointers to encode their
+information. It collides with valid pointers with Sv57 paging and
+leads to crashes.
+
+To mitigate this, we are not going to allocate virtual address space
+above 47-bit by default.
+
+But userspace can ask for allocation from full address space by
+specifying hint address (with or without MAP_FIXED) above 47-bits.
+
+If hint address set above 47-bit, but MAP_FIXED is not specified, we try
+to look for unmapped area by specified address. If it's already occupied,
+this mmap will fail.
+
+A high hint address would only affect the allocation in question, but not
+any future mmap()s.
+
+Specifying high hint address without MAP_FIXED on older kernel or on
+machine without Sv57 paging support is safe. The hint will be ignored and
+kernel will fall back to allocation from the supported address space.
+
+This approach helps to easily make application's memory allocator aware
+about large address space without manually tracking allocated virtual
+address space.
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/3] RISC-V: mm: correct mmap behavior in sv48 address space

[Yangyu Chen]

Friendly ping. There are already 5 days since this patch was submitted 
but there are no comments now. The previous buggy patch has already been 
in the mainline kernel since the v6.6 release and breaks some userspace 
software that relies on mmap to create mapping on the hint address 
without MAP_FIXED set. I think this fix should go to the kernel ASAP.

Thanks,
Yangyu Chen

On 1/15/24 03:55, Yangyu Chen wrote:
> Previous patch series [1] violates the principle of mmap syscall as it uses
> hint address as the largest address space to use rather than where to
> create the mapping, thus broke the possibility to mmap in sv48, sv57
> address space without a MAP_FIXED flag. This patchset corrects the behavior
> of mmap syscall and use the behavior of x86 5-stage-paging as a reference.
> 
> I first noticed this issue when I was trying to run box64 on a sv48 system
> with commit previous than [2]. Then I reported this through private
> communication, then a box64 contributor did some investigation and found
> that trying to mmap in sv48 address space without MAP_FIXED flag will
> always return a random address in sv39. I review the changelog with some
> tests on qemu and found this issue was introduced from [1]. After reviewing
> the code, tests and docs, I think the original author might misunderstand
> the meaning of hint address in mmap syscall. Then I did some investigation
> on other ISAs like x86 which has 5-stage-paging and found that it has
> addressed the same issue if some userspace software assumes the pointer
> size should smaller than 47 bits and also solved in kernel by limiting the
> mmap in maximum 47 bits address space by default.
> 
> Finally I correct the behavior of mmap syscall as x86 5-stage-paging does,
> and migreate the documentation from x86-64 kernel to riscv kernel.
> 
> 
> [1]. https://lore.kernel.org/linux-riscv/20230809232218.849726-1-charlie@rivosinc.com/
> [2]. https://github.com/ptitSeb/box64/commit/5b700cb6e6f397d2074c49659f7f9915f4a33c5f
> 
> Yangyu Chen (3):
>    RISC-V: mm: fix mmap behavior in sv48 address space
>    RISC-V: mm: only test mmap without hint
>    Documentation: riscv: correct sv57 kernel behavior
> 
>   Documentation/arch/riscv/vm-layout.rst        | 48 +++++++++++--------
>   arch/riscv/include/asm/processor.h            | 39 ++++-----------
>   .../selftests/riscv/mm/mmap_bottomup.c        | 12 -----
>   .../testing/selftests/riscv/mm/mmap_default.c | 12 -----
>   tools/testing/selftests/riscv/mm/mmap_test.h  | 30 ------------
>   5 files changed, 36 insertions(+), 105 deletions(-)
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] RISC-V: mm: fix mmap behavior in sv48 address space

[Charlie Jenkins]

On Mon, Jan 15, 2024 at 03:58:30AM +0800, Yangyu Chen wrote:
> A commit add2cc6b6515 ("RISC-V: mm: Restrict address space for
> sv39,sv48,sv57") from patch[1] restricts regular mmap return address in the
> sv48 space if the address hint is not above the sv48 userspace address.
> However, this commit treats the address wrong which only use sv48 if the
> hint address is above sv48 user address space. Actually, it should use sv48
> if the address is above sv39 user address space. Moreover, the original

It is not valid to use any address in sv48 in this case. It is designed
to provide an address that is in sv39 to guarantee that the application
is able to handle any address that is returned by mmap. If, for example,
the application asks for the address 1<<42, it expects that all of the
bits above 42 are not used. The address provided by mmap must not
contain those bits, so it uses the sv39 address space.

- Charlie

> patch code looks very complex in logic, we can simplify it with min marco.
> 
> [1]. https://lore.kernel.org/r/20230809232218.849726-2-charlie@rivosinc.com
> 
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> ---
>  arch/riscv/include/asm/processor.h | 39 ++++++------------------------
>  1 file changed, 8 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index e1944ff0757a..7ead6a3e1f12 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -9,6 +9,7 @@
>  #include <linux/const.h>
>  #include <linux/cache.h>
>  #include <linux/prctl.h>
> +#include <linux/minmax.h>
>  
>  #include <vdso/processor.h>
>  
> @@ -18,37 +19,13 @@
>  #define DEFAULT_MAP_WINDOW	(UL(1) << (MMAP_VA_BITS - 1))
>  #define STACK_TOP_MAX		TASK_SIZE
>  
> -#define arch_get_mmap_end(addr, len, flags)			\
> -({								\
> -	unsigned long mmap_end;					\
> -	typeof(addr) _addr = (addr);				\
> -	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> -		mmap_end = STACK_TOP_MAX;			\
> -	else if ((_addr) >= VA_USER_SV57)			\
> -		mmap_end = STACK_TOP_MAX;			\
> -	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> -		mmap_end = VA_USER_SV48;			\
> -	else							\
> -		mmap_end = VA_USER_SV39;			\
> -	mmap_end;						\
> -})
> -
> -#define arch_get_mmap_base(addr, base)				\
> -({								\
> -	unsigned long mmap_base;				\
> -	typeof(addr) _addr = (addr);				\
> -	typeof(base) _base = (base);				\
> -	unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);	\
> -	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> -		mmap_base = (_base);				\
> -	else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
> -		mmap_base = VA_USER_SV57 - rnd_gap;		\
> -	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> -		mmap_base = VA_USER_SV48 - rnd_gap;		\
> -	else							\
> -		mmap_base = VA_USER_SV39 - rnd_gap;		\
> -	mmap_base;						\
> -})
> +#define arch_get_mmap_end(addr, len, flags) \
> +	((addr) >= DEFAULT_MAP_WINDOW ? STACK_TOP_MAX :\
> +	 min(DEFAULT_MAP_WINDOW, STACK_TOP_MAX))
> +
> +#define arch_get_mmap_base(addr, base) \
> +	((addr) >= DEFAULT_MAP_WINDOW ? base :\
> +	 min(base, DEFAULT_MAP_WINDOW))
>  
>  #else
>  #define DEFAULT_MAP_WINDOW	TASK_SIZE
> -- 
> 2.43.0
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv