[PATCH 0/3] riscv: mm: Do not restrict mmap address based on hint

[PATCH 0/3] riscv: mm: Do not restrict mmap address based on hint

[Charlie Jenkins]

There have been a couple of reports that using the hint address to
restrict the address returned by mmap hint address has caused issues in
applications. A different solution for restricting addresses returned by
mmap is necessary to avoid breakages.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Charlie Jenkins (3):
      Revert "RISC-V: mm: Document mmap changes"
      riscv: selftests: Remove mmap hint address checks
      riscv: mm: Do not restrict mmap address based on hint

 Documentation/arch/riscv/vm-layout.rst           | 16 ------
 arch/riscv/include/asm/processor.h               | 26 +--------
 tools/testing/selftests/riscv/mm/mmap_bottomup.c |  2 -
 tools/testing/selftests/riscv/mm/mmap_default.c  |  2 -
 tools/testing/selftests/riscv/mm/mmap_test.h     | 67 ------------------------
 5 files changed, 2 insertions(+), 111 deletions(-)
---
base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
change-id: 20240820-riscv_mmap-055efd23f19c
-- 
- Charlie

[PATCH 1/3] Revert "RISC-V: mm: Document mmap changes"

[Charlie Jenkins]

This mmap behavior caused unintended breakages so the behavior has been
changed.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 Documentation/arch/riscv/vm-layout.rst | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/Documentation/arch/riscv/vm-layout.rst b/Documentation/arch/riscv/vm-layout.rst
index 077b968dcc81..eabec99b5852 100644
--- a/Documentation/arch/riscv/vm-layout.rst
+++ b/Documentation/arch/riscv/vm-layout.rst
@@ -134,19 +134,3 @@ RISC-V Linux Kernel SV57
    ffffffff00000000 |  -4     GB | ffffffff7fffffff |    2 GB | modules, BPF
    ffffffff80000000 |  -2     GB | ffffffffffffffff |    2 GB | kernel
   __________________|____________|__________________|_________|____________________________________________________________
-
-
-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. When a hint address is passed to mmap, the returned
-address will never use more bits than the hint address. For example, if a hint
-address of `1 << 40` is passed to mmap, a valid returned address will never use
-bits 41 through 63. If no mappable addresses are available in that range, mmap
-will return `MAP_FAILED`.

-- 
2.45.0

[PATCH 2/3] riscv: selftests: Remove mmap hint address checks

[Charlie Jenkins]

The mmap behavior that restricts the addresses returned by mmap caused
unexpected behavior, so get rid of the test cases that check that
behavior.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
Fixes: 73d05262a2ca ("selftests: riscv: Generalize mm selftests")
---
 tools/testing/selftests/riscv/mm/mmap_bottomup.c |  2 -
 tools/testing/selftests/riscv/mm/mmap_default.c  |  2 -
 tools/testing/selftests/riscv/mm/mmap_test.h     | 67 ------------------------
 3 files changed, 71 deletions(-)

diff --git a/tools/testing/selftests/riscv/mm/mmap_bottomup.c b/tools/testing/selftests/riscv/mm/mmap_bottomup.c
index 7f7d3eb8b9c9..f9ccae50349b 100644
--- a/tools/testing/selftests/riscv/mm/mmap_bottomup.c
+++ b/tools/testing/selftests/riscv/mm/mmap_bottomup.c
@@ -7,8 +7,6 @@
 TEST(infinite_rlimit)
 {
 	EXPECT_EQ(BOTTOM_UP, memory_layout());
-
-	TEST_MMAPS;
 }
 
 TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/riscv/mm/mmap_default.c b/tools/testing/selftests/riscv/mm/mmap_default.c
index 2ba3ec990006..3f53b6ecc326 100644
--- a/tools/testing/selftests/riscv/mm/mmap_default.c
+++ b/tools/testing/selftests/riscv/mm/mmap_default.c
@@ -7,8 +7,6 @@
 TEST(default_rlimit)
 {
 	EXPECT_EQ(TOP_DOWN, memory_layout());
-
-	TEST_MMAPS;
 }
 
 TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/riscv/mm/mmap_test.h b/tools/testing/selftests/riscv/mm/mmap_test.h
index 3b29ca3bb3d4..75918d15919f 100644
--- a/tools/testing/selftests/riscv/mm/mmap_test.h
+++ b/tools/testing/selftests/riscv/mm/mmap_test.h
@@ -10,76 +10,9 @@
 #define TOP_DOWN 0
 #define BOTTOM_UP 1
 
-#if __riscv_xlen == 64
-uint64_t random_addresses[] = {
-	0x19764f0d73b3a9f0, 0x016049584cecef59, 0x3580bdd3562f4acd,
-	0x1164219f20b17da0, 0x07d97fcb40ff2373, 0x76ec528921272ee7,
-	0x4dd48c38a3de3f70, 0x2e11415055f6997d, 0x14b43334ac476c02,
-	0x375a60795aff19f6, 0x47f3051725b8ee1a, 0x4e697cf240494a9f,
-	0x456b59b5c2f9e9d1, 0x101724379d63cb96, 0x7fe9ad31619528c1,
-	0x2f417247c495c2ea, 0x329a5a5b82943a5e, 0x06d7a9d6adcd3827,
-	0x327b0b9ee37f62d5, 0x17c7b1851dfd9b76, 0x006ebb6456ec2cd9,
-	0x00836cd14146a134, 0x00e5c4dcde7126db, 0x004c29feadf75753,
-	0x00d8b20149ed930c, 0x00d71574c269387a, 0x0006ebe4a82acb7a,
-	0x0016135df51f471b, 0x00758bdb55455160, 0x00d0bdd949b13b32,
-	0x00ecea01e7c5f54b, 0x00e37b071b9948b1, 0x0011fdd00ff57ab3,
-	0x00e407294b52f5ea, 0x00567748c200ed20, 0x000d073084651046,
-	0x00ac896f4365463c, 0x00eb0d49a0b26216, 0x0066a2564a982a31,
-	0x002e0d20237784ae, 0x0000554ff8a77a76, 0x00006ce07a54c012,
-	0x000009570516d799, 0x00000954ca15b84d, 0x0000684f0d453379,
-	0x00002ae5816302b5, 0x0000042403fb54bf, 0x00004bad7392bf30,
-	0x00003e73bfa4b5e3, 0x00005442c29978e0, 0x00002803f11286b6,
-	0x000073875d745fc6, 0x00007cede9cb8240, 0x000027df84cc6a4f,
-	0x00006d7e0e74242a, 0x00004afd0b836e02, 0x000047d0e837cd82,
-	0x00003b42405efeda, 0x00001531bafa4c95, 0x00007172cae34ac4,
-};
-#else
-uint32_t random_addresses[] = {
-	0x8dc302e0, 0x929ab1e0, 0xb47683ba, 0xea519c73, 0xa19f1c90, 0xc49ba213,
-	0x8f57c625, 0xadfe5137, 0x874d4d95, 0xaa20f09d, 0xcf21ebfc, 0xda7737f1,
-	0xcedf392a, 0x83026c14, 0xccedca52, 0xc6ccf826, 0xe0cd9415, 0x997472ca,
-	0xa21a44c1, 0xe82196f5, 0xa23fd66b, 0xc28d5590, 0xd009cdce, 0xcf0be646,
-	0x8fc8c7ff, 0xe2a85984, 0xa3d3236b, 0x89a0619d, 0xc03db924, 0xb5d4cc1b,
-	0xb96ee04c, 0xd191da48, 0xb432a000, 0xaa2bebbc, 0xa2fcb289, 0xb0cca89b,
-	0xb0c18d6a, 0x88f58deb, 0xa4d42d1c, 0xe4d74e86, 0x99902b09, 0x8f786d31,
-	0xbec5e381, 0x9a727e65, 0xa9a65040, 0xa880d789, 0x8f1b335e, 0xfc821c1e,
-	0x97e34be4, 0xbbef84ed, 0xf447d197, 0xfd7ceee2, 0xe632348d, 0xee4590f4,
-	0x958992a5, 0xd57e05d6, 0xfd240970, 0xc5b0dcff, 0xd96da2c2, 0xa7ae041d,
-};
-#endif
-
-// Only works on 64 bit
-#if __riscv_xlen == 64
 #define PROT (PROT_READ | PROT_WRITE)
 #define FLAGS (MAP_PRIVATE | MAP_ANONYMOUS)
 
-/* mmap must return a value that doesn't use more bits than the hint address. */
-static inline unsigned long get_max_value(unsigned long input)
-{
-	unsigned long max_bit = (1UL << (((sizeof(unsigned long) * 8) - 1 -
-					  __builtin_clzl(input))));
-
-	return max_bit + (max_bit - 1);
-}
-
-#define TEST_MMAPS                                                            \
-	({                                                                    \
-		void *mmap_addr;                                              \
-		for (int i = 0; i < ARRAY_SIZE(random_addresses); i++) {      \
-			mmap_addr = mmap((void *)random_addresses[i],         \
-					 5 * sizeof(int), PROT, FLAGS, 0, 0); \
-			EXPECT_NE(MAP_FAILED, mmap_addr);                     \
-			EXPECT_GE((void *)get_max_value(random_addresses[i]), \
-				  mmap_addr);                                 \
-			mmap_addr = mmap((void *)random_addresses[i],         \
-					 5 * sizeof(int), PROT, FLAGS, 0, 0); \
-			EXPECT_NE(MAP_FAILED, mmap_addr);                     \
-			EXPECT_GE((void *)get_max_value(random_addresses[i]), \
-				  mmap_addr);                                 \
-		}                                                             \
-	})
-#endif /* __riscv_xlen == 64 */
-
 static inline int memory_layout(void)
 {
 	void *value1 = mmap(NULL, sizeof(int), PROT, FLAGS, 0, 0);

-- 
2.45.0

[PATCH 3/3] riscv: mm: Do not restrict mmap address based on hint

[Charlie Jenkins]

The hint address should not forcefully restrict the addresses returned
by mmap as this causes mmap to report ENOMEM when there is memory still
available.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
Fixes: b5b4287accd7 ("riscv: mm: Use hint address in mmap if available")
Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57")
Closes: https://lore.kernel.org/linux-kernel/ZbxTNjQPFKBatMq+@ghost/T/#mccb1890466bf5a488c9ce7441e57e42271895765
---
 arch/riscv/include/asm/processor.h | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 8702b8721a27..efa1b3519b23 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -14,36 +14,14 @@
 
 #include <asm/ptrace.h>
 
-/*
- * addr is a hint to the maximum userspace address that mmap should provide, so
- * this macro needs to return the largest address space available so that
- * mmap_end < addr, being mmap_end the top of that address space.
- * See Documentation/arch/riscv/vm-layout.rst for more details.
- */
 #define arch_get_mmap_end(addr, len, flags)			\
 ({								\
-	unsigned long mmap_end;					\
-	typeof(addr) _addr = (addr);				\
-	if ((_addr) == 0 || is_compat_task() ||			\
-	    ((_addr + len) > BIT(VA_BITS - 1)))			\
-		mmap_end = STACK_TOP_MAX;			\
-	else							\
-		mmap_end = (_addr + len);			\
-	mmap_end;						\
+	STACK_TOP_MAX;						\
 })
 
 #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_compat_task() || 		\
-	    ((_addr + len) > BIT(VA_BITS - 1)))			\
-		mmap_base = (_base);				\
-	else							\
-		mmap_base = (_addr + len) - rnd_gap;		\
-	mmap_base;						\
+	base;							\
 })
 
 #ifdef CONFIG_64BIT

-- 
2.45.0

Re: [PATCH 3/3] riscv: mm: Do not restrict mmap address based on hint

[Yangyu Chen]

> On Aug 27, 2024, at 00:36, Charlie Jenkins <charlie@rivosinc.com> wrote:
> 
> The hint address should not forcefully restrict the addresses returned
> by mmap as this causes mmap to report ENOMEM when there is memory still
> available.
> 

Fixing in this way will break userspace on Sv57 machines as some
issues mentioned in the patch [1].

I suggest restricting to BIT(47) by default, like patch [2], to
align with kernel behavior on x86 and aarch64, and this does exist
on x86 and aarch64 for quite a long time. In that way, we will also
solve the problem mentioned in the first patch [1], as QEMU enables
Sv57 by default now and will not break userspace.

[1] https://lore.kernel.org/linux-riscv/20230809232218.849726-1-charlie@rivosinc.com/
[2] https://lore.kernel.org/linux-riscv/tencent_B2D0435BC011135736262764B511994F4805@qq.com/

Thanks,
Yangyu Chen

> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> Fixes: b5b4287accd7 ("riscv: mm: Use hint address in mmap if available")
> Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57")
> Closes: https://lore.kernel.org/linux-kernel/ZbxTNjQPFKBatMq+@ghost/T/#mccb1890466bf5a488c9ce7441e57e42271895765
> ---
> arch/riscv/include/asm/processor.h | 26 ++------------------------
> 1 file changed, 2 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index 8702b8721a27..efa1b3519b23 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -14,36 +14,14 @@
> 
> #include <asm/ptrace.h>
> 
> -/*
> - * addr is a hint to the maximum userspace address that mmap should provide, so
> - * this macro needs to return the largest address space available so that
> - * mmap_end < addr, being mmap_end the top of that address space.
> - * See Documentation/arch/riscv/vm-layout.rst for more details.
> - */
> #define arch_get_mmap_end(addr, len, flags) \
> ({ \
> - unsigned long mmap_end; \
> - typeof(addr) _addr = (addr); \
> - if ((_addr) == 0 || is_compat_task() || \
> -    ((_addr + len) > BIT(VA_BITS - 1))) \
> - mmap_end = STACK_TOP_MAX; \
> - else \
> - mmap_end = (_addr + len); \
> - mmap_end; \
> + STACK_TOP_MAX; \
> })
> 
> #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_compat_task() || \
> -    ((_addr + len) > BIT(VA_BITS - 1))) \
> - mmap_base = (_base); \
> - else \
> - mmap_base = (_addr + len) - rnd_gap; \
> - mmap_base; \
> + base; \
> })
> 
> #ifdef CONFIG_64BIT
> 
> -- 
> 2.45.0
> 

Re: [PATCH 3/3] riscv: mm: Do not restrict mmap address based on hint

[Levi Zim]

On 2024-08-27 00:36, Charlie Jenkins wrote:
> The hint address should not forcefully restrict the addresses returned
> by mmap as this causes mmap to report ENOMEM when there is memory still
> available.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> Fixes: b5b4287accd7 ("riscv: mm: Use hint address in mmap if available")
> Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57")
> Closes: https://lore.kernel.org/linux-kernel/ZbxTNjQPFKBatMq+@ghost/T/#mccb1890466bf5a488c9ce7441e57e42271895765
> ---
>   arch/riscv/include/asm/processor.h | 26 ++------------------------
>   1 file changed, 2 insertions(+), 24 deletions(-)
>
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index 8702b8721a27..efa1b3519b23 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -14,36 +14,14 @@
>   
>   #include <asm/ptrace.h>
>   
> -/*
> - * addr is a hint to the maximum userspace address that mmap should provide, so
> - * this macro needs to return the largest address space available so that
> - * mmap_end < addr, being mmap_end the top of that address space.
> - * See Documentation/arch/riscv/vm-layout.rst for more details.
> - */
>   #define arch_get_mmap_end(addr, len, flags)			\
>   ({								\
> -	unsigned long mmap_end;					\
> -	typeof(addr) _addr = (addr);				\
> -	if ((_addr) == 0 || is_compat_task() ||			\
> -	    ((_addr + len) > BIT(VA_BITS - 1)))			\
> -		mmap_end = STACK_TOP_MAX;			\
> -	else							\
> -		mmap_end = (_addr + len);			\
> -	mmap_end;						\
> +	STACK_TOP_MAX;						\
>   })
>   
>   #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_compat_task() || 		\
> -	    ((_addr + len) > BIT(VA_BITS - 1)))			\
> -		mmap_base = (_base);				\
> -	else							\
> -		mmap_base = (_addr + len) - rnd_gap;		\
> -	mmap_base;						\
> +	base;							\
>   })
>   
>   #ifdef CONFIG_64BIT
>
I tested this patch on 6.10.2 kernel and could confirm that it fixes the 
crash of chromium. But I think I prefer Yangyu Chen's approach because 
that would avoid breaking some applications on sv57.

Tested-by: Levi Zim <rsworktech@outlook.com> # Chromium, sv39

Re: [PATCH 3/3] riscv: mm: Do not restrict mmap address based on hint

[Palmer Dabbelt]

On Mon, 26 Aug 2024 19:24:38 PDT (-0700), cyy@cyyself.name wrote:
>
>
>> On Aug 27, 2024, at 00:36, Charlie Jenkins <charlie@rivosinc.com> wrote:
>> 
>> The hint address should not forcefully restrict the addresses returned
>> by mmap as this causes mmap to report ENOMEM when there is memory still
>> available.
>> 
>
> Fixing in this way will break userspace on Sv57 machines as some
> issues mentioned in the patch [1].
>
> I suggest restricting to BIT(47) by default, like patch [2], to
> align with kernel behavior on x86 and aarch64, and this does exist
> on x86 and aarch64 for quite a long time. In that way, we will also
> solve the problem mentioned in the first patch [1], as QEMU enables
> Sv57 by default now and will not break userspace.
>
> [1] https://lore.kernel.org/linux-riscv/20230809232218.849726-1-charlie@rivosinc.com/
> [2] https://lore.kernel.org/linux-riscv/tencent_B2D0435BC011135736262764B511994F4805@qq.com/

I'm going to pick this up as it's a revert and a bug fix, so we can 
backport it.  If the right answer is to just forget about the sv39 
userspace and only worry about sv48 userspace then your patches are 
likely the way to go, but there's a handful of discussions around that 
which might take a bit.

>
> Thanks,
> Yangyu Chen
>
>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>> Fixes: b5b4287accd7 ("riscv: mm: Use hint address in mmap if available")
>> Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57")
>> Closes: https://lore.kernel.org/linux-kernel/ZbxTNjQPFKBatMq+@ghost/T/#mccb1890466bf5a488c9ce7441e57e42271895765
>> ---
>> arch/riscv/include/asm/processor.h | 26 ++------------------------
>> 1 file changed, 2 insertions(+), 24 deletions(-)
>> 
>> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
>> index 8702b8721a27..efa1b3519b23 100644
>> --- a/arch/riscv/include/asm/processor.h
>> +++ b/arch/riscv/include/asm/processor.h
>> @@ -14,36 +14,14 @@
>> 
>> #include <asm/ptrace.h>
>> 
>> -/*
>> - * addr is a hint to the maximum userspace address that mmap should provide, so
>> - * this macro needs to return the largest address space available so that
>> - * mmap_end < addr, being mmap_end the top of that address space.
>> - * See Documentation/arch/riscv/vm-layout.rst for more details.
>> - */
>> #define arch_get_mmap_end(addr, len, flags) \
>> ({ \
>> - unsigned long mmap_end; \
>> - typeof(addr) _addr = (addr); \
>> - if ((_addr) == 0 || is_compat_task() || \
>> -    ((_addr + len) > BIT(VA_BITS - 1))) \
>> - mmap_end = STACK_TOP_MAX; \
>> - else \
>> - mmap_end = (_addr + len); \
>> - mmap_end; \
>> + STACK_TOP_MAX; \
>> })
>> 
>> #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_compat_task() || \
>> -    ((_addr + len) > BIT(VA_BITS - 1))) \
>> - mmap_base = (_base); \
>> - else \
>> - mmap_base = (_addr + len) - rnd_gap; \
>> - mmap_base; \
>> + base; \
>> })
>> 
>> #ifdef CONFIG_64BIT
>> 
>> -- 
>> 2.45.0
>> 

Re: [PATCH 0/3] riscv: mm: Do not restrict mmap address based on hint

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Mon, 26 Aug 2024 09:36:44 -0700 you wrote:
> There have been a couple of reports that using the hint address to
> restrict the address returned by mmap hint address has caused issues in
> applications. A different solution for restricting addresses returned by
> mmap is necessary to avoid breakages.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> 
> [...]

Here is the summary with links:
  - [1/3] Revert "RISC-V: mm: Document mmap changes"
    https://git.kernel.org/riscv/c/954260ff5a46
  - [2/3] riscv: selftests: Remove mmap hint address checks
    https://git.kernel.org/riscv/c/83dae72ac038
  - [3/3] riscv: mm: Do not restrict mmap address based on hint
    https://git.kernel.org/riscv/c/2116988d5372

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 3/3] riscv: mm: Do not restrict mmap address based on hint

[Yangyu Chen]

> On Sep 3, 2024, at 22:27, Palmer Dabbelt <palmer@rivosinc.com> wrote:
> 
> On Mon, 26 Aug 2024 19:24:38 PDT (-0700), cyy@cyyself.name wrote:
>> 
>> 
>>> On Aug 27, 2024, at 00:36, Charlie Jenkins <charlie@rivosinc.com> wrote:
>>> The hint address should not forcefully restrict the addresses returned
>>> by mmap as this causes mmap to report ENOMEM when there is memory still
>>> available.
>> 
>> Fixing in this way will break userspace on Sv57 machines as some
>> issues mentioned in the patch [1].
>> 
>> I suggest restricting to BIT(47) by default, like patch [2], to
>> align with kernel behavior on x86 and aarch64, and this does exist
>> on x86 and aarch64 for quite a long time. In that way, we will also
>> solve the problem mentioned in the first patch [1], as QEMU enables
>> Sv57 by default now and will not break userspace.
>> 
>> [1] https://lore.kernel.org/linux-riscv/20230809232218.849726-1-charlie@rivosinc.com/
>> [2] https://lore.kernel.org/linux-riscv/tencent_B2D0435BC011135736262764B511994F4805@qq.com/
> 
> I'm going to pick this up as it's a revert and a bug fix, so we can backport it.  If the right answer is to just forget about the sv39 userspace and only worry about sv48 userspace then your patches are likely the way to go,

I think we don't need to care about restricting to sv39 now since
the ASAN bug has been fixed. We should care about what to do to not
break userspace on sv57 machines as QEMU enables sv57 by default,
which is widely used.

> but there's a handful of discussions around that which might take a bit.
> 
>> 
>> Thanks,
>> Yangyu Chen
>> 
>>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>>> Fixes: b5b4287accd7 ("riscv: mm: Use hint address in mmap if available")
>>> Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57")
>>> Closes: https://lore.kernel.org/linux-kernel/ZbxTNjQPFKBatMq+@ghost/T/#mccb1890466bf5a488c9ce7441e57e42271895765
>>> ---
>>> arch/riscv/include/asm/processor.h | 26 ++------------------------
>>> 1 file changed, 2 insertions(+), 24 deletions(-)
>>> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
>>> index 8702b8721a27..efa1b3519b23 100644
>>> --- a/arch/riscv/include/asm/processor.h
>>> +++ b/arch/riscv/include/asm/processor.h
>>> @@ -14,36 +14,14 @@
>>> #include <asm/ptrace.h>
>>> -/*
>>> - * addr is a hint to the maximum userspace address that mmap should provide, so
>>> - * this macro needs to return the largest address space available so that
>>> - * mmap_end < addr, being mmap_end the top of that address space.
>>> - * See Documentation/arch/riscv/vm-layout.rst for more details.
>>> - */
>>> #define arch_get_mmap_end(addr, len, flags) \
>>> ({ \
>>> - unsigned long mmap_end; \
>>> - typeof(addr) _addr = (addr); \
>>> - if ((_addr) == 0 || is_compat_task() || \
>>> -    ((_addr + len) > BIT(VA_BITS - 1))) \
>>> - mmap_end = STACK_TOP_MAX; \
>>> - else \
>>> - mmap_end = (_addr + len); \
>>> - mmap_end; \
>>> + STACK_TOP_MAX; \
>>> })
>>> #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_compat_task() || \
>>> -    ((_addr + len) > BIT(VA_BITS - 1))) \
>>> - mmap_base = (_base); \
>>> - else \
>>> - mmap_base = (_addr + len) - rnd_gap; \
>>> - mmap_base; \
>>> + base; \
>>> })
>>> #ifdef CONFIG_64BIT
>>> -- 
>>> 2.45.0