| Message ID | 20240826-riscv_mmap-v1-3-cd8962afe47f@rivosinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2116988d5372aec51f8c4fb85bf8e305ecda47a0 |
| Headers | show |
| Series | riscv: mm: Do not restrict mmap address based on hint | expand |
> 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 >
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
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 >>
> 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
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
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(-)