| Message ID | ab847b6edb197bffdfe189e70fb4ac76bfe79e0d.1650033747.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v10] mm, hugetlbfs: Allow for "high" userspace addresses | expand |
On Fri, 15 Apr 2022 16:45:13 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > This is a fix for commit f6795053dac8 ("mm: mmap: Allow for "high" > userspace addresses") for hugetlb. > > This patch adds support for "high" userspace addresses that are > optionally supported on the system and have to be requested via a hint > mechanism ("high" addr parameter to mmap). > > Architectures such as powerpc and x86 achieve this by making changes to > their architectural versions of hugetlb_get_unmapped_area() function. > However, arm64 uses the generic version of that function. > > So take into account arch_get_mmap_base() and arch_get_mmap_end() in > hugetlb_get_unmapped_area(). To allow that, move those two macros > out of mm/mmap.c into include/linux/sched/mm.h > > If these macros are not defined in architectural code then they default > to (TASK_SIZE) and (base) so should not introduce any behavioural > changes to architectures that do not define them. > > For the time being, only ARM64 is affected by this change. > > >From Catalin (ARM64): > We should have fixed hugetlb_get_unmapped_area() as well when we > added support for 52-bit VA. The reason for commit f6795053dac8 was to > prevent normal mmap() from returning addresses above 48-bit by default > as some user-space had hard assumptions about this. > > It's a slight ABI change if you do this for hugetlb_get_unmapped_area() > but I doubt anyone would notice. It's more likely that the current > behaviour would cause issues, so I'd rather have them consistent. I'm struggling to understand the need for a -stable backport from the above text. Could we please get a simple statement of the end-user visible effects of the shortcoming? Target audience is -stable tree maintainers, and people who we've never heard of who will be wondering whether they should add this to their organization's older kernel. > fs/hugetlbfs/inode.c | 9 +++++---- > include/linux/sched/mm.h | 8 ++++++++ > mm/mmap.c | 8 -------- > 3 files changed, 13 insertions(+), 12 deletions(-) I'm a bit surprised that this has reached version 10! Was it really that tricky?
On Fri, 15 Apr 2022 16:45:13 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > This is a fix for commit f6795053dac8 ("mm: mmap: Allow for "high" > userspace addresses") for hugetlb. So the "hugetlbfs" in the Subject: is a tpyo?
Hi Catalin, Le 16/04/2022 à 00:09, Andrew Morton a écrit : > On Fri, 15 Apr 2022 16:45:13 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > >> This is a fix for commit f6795053dac8 ("mm: mmap: Allow for "high" >> userspace addresses") for hugetlb. >> >> This patch adds support for "high" userspace addresses that are >> optionally supported on the system and have to be requested via a hint >> mechanism ("high" addr parameter to mmap). >> >> Architectures such as powerpc and x86 achieve this by making changes to >> their architectural versions of hugetlb_get_unmapped_area() function. >> However, arm64 uses the generic version of that function. >> >> So take into account arch_get_mmap_base() and arch_get_mmap_end() in >> hugetlb_get_unmapped_area(). To allow that, move those two macros >> out of mm/mmap.c into include/linux/sched/mm.h >> >> If these macros are not defined in architectural code then they default >> to (TASK_SIZE) and (base) so should not introduce any behavioural >> changes to architectures that do not define them. >> >> For the time being, only ARM64 is affected by this change. >> >> >From Catalin (ARM64): >> We should have fixed hugetlb_get_unmapped_area() as well when we >> added support for 52-bit VA. The reason for commit f6795053dac8 was to >> prevent normal mmap() from returning addresses above 48-bit by default >> as some user-space had hard assumptions about this. >> >> It's a slight ABI change if you do this for hugetlb_get_unmapped_area() >> but I doubt anyone would notice. It's more likely that the current >> behaviour would cause issues, so I'd rather have them consistent. > > I'm struggling to understand the need for a -stable backport from the > above text. > > Could we please get a simple statement of the end-user visible effects > of the shortcoming? Target audience is -stable tree maintainers, and > people who we've never heard of who will be wondering whether they should > add this to their organization's older kernel. Catalin, can you help answering this question ? It was your recommendation to tag this patch for stable in https://patchwork.ozlabs.org/project/linuxppc-dev/patch/db238c1ca2d46e33c57328f8d450f2563e92f8c2.1639736449.git.christophe.leroy@csgroup.eu/ > >> fs/hugetlbfs/inode.c | 9 +++++---- >> include/linux/sched/mm.h | 8 ++++++++ >> mm/mmap.c | 8 -------- >> 3 files changed, 13 insertions(+), 12 deletions(-) > > I'm a bit surprised that this has reached version 10! Was it really > that tricky? > Well, that's the series it was part of that has reached v10. This patch was introduced in the series in v6 v6: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/db238c1ca2d46e33c57328f8d450f2563e92f8c2.1639736449.git.christophe.leroy@csgroup.eu/ v7: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/6c95091eab9f58cee58da3762a4dc4c56ab700e7.1642752946.git.christophe.leroy@csgroup.eu/ v8: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/c234ceaf81ff37447fec5c9813d4ba5fc472a355.1646847562.git.christophe.leroy@csgroup.eu/ v9: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/3bb944642140841c065f1cd6eae73f084fc026d2.1649401201.git.christophe.leroy@csgroup.eu/ Thanks Christophe
Le 16/04/2022 à 00:12, Andrew Morton a écrit : > On Fri, 15 Apr 2022 16:45:13 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > >> This is a fix for commit f6795053dac8 ("mm: mmap: Allow for "high" >> userspace addresses") for hugetlb. > > So the "hugetlbfs" in the Subject: is a tpyo? This patch modifies fs/hugetlbfs/inode.c , hence the "hugetlbfs" in the Subject. Am I doing it wrong ? Christophe
On Sat, 16 Apr 2022 06:29:04 +0000 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > Le 16/04/2022 à 00:12, Andrew Morton a écrit : > > On Fri, 15 Apr 2022 16:45:13 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > >> This is a fix for commit f6795053dac8 ("mm: mmap: Allow for "high" > >> userspace addresses") for hugetlb. > > > > So the "hugetlbfs" in the Subject: is a tpyo? > > This patch modifies fs/hugetlbfs/inode.c , hence the "hugetlbfs" in the > Subject. Sorry, obviously I was too sober at the time.
On Sat, Apr 16, 2022 at 06:26:42AM +0000, Christophe Leroy wrote: > Le 16/04/2022 à 00:09, Andrew Morton a écrit : > > On Fri, 15 Apr 2022 16:45:13 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > >> This is a fix for commit f6795053dac8 ("mm: mmap: Allow for "high" > >> userspace addresses") for hugetlb. > >> > >> This patch adds support for "high" userspace addresses that are > >> optionally supported on the system and have to be requested via a hint > >> mechanism ("high" addr parameter to mmap). > >> > >> Architectures such as powerpc and x86 achieve this by making changes to > >> their architectural versions of hugetlb_get_unmapped_area() function. > >> However, arm64 uses the generic version of that function. > >> > >> So take into account arch_get_mmap_base() and arch_get_mmap_end() in > >> hugetlb_get_unmapped_area(). To allow that, move those two macros > >> out of mm/mmap.c into include/linux/sched/mm.h > >> > >> If these macros are not defined in architectural code then they default > >> to (TASK_SIZE) and (base) so should not introduce any behavioural > >> changes to architectures that do not define them. > >> > >> For the time being, only ARM64 is affected by this change. > >> > >> >From Catalin (ARM64): > >> We should have fixed hugetlb_get_unmapped_area() as well when we > >> added support for 52-bit VA. The reason for commit f6795053dac8 was to > >> prevent normal mmap() from returning addresses above 48-bit by default > >> as some user-space had hard assumptions about this. > >> > >> It's a slight ABI change if you do this for hugetlb_get_unmapped_area() > >> but I doubt anyone would notice. It's more likely that the current > >> behaviour would cause issues, so I'd rather have them consistent. > > > > I'm struggling to understand the need for a -stable backport from the > > above text. > > > > Could we please get a simple statement of the end-user visible effects > > of the shortcoming? Target audience is -stable tree maintainers, and > > people who we've never heard of who will be wondering whether they should > > add this to their organization's older kernel. > > Catalin, can you help answering this question ? It was your > recommendation to tag this patch for stable in > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/db238c1ca2d46e33c57328f8d450f2563e92f8c2.1639736449.git.christophe.leroy@csgroup.eu/ My reasoning was that we should have made hugetlb_get_unmapped_area() consistent with arch_get_unmapped_area() since commit f6795053dac8 ("mm: mmap: Allow for "high" userspace addresses"). Basically when arm64 gained support for 52-bit addresses we did not want user-space calling mmap() to suddenly get such high addresses, otherwise we could have inadvertently broken some programs (similar behaviour to x86 here). Hence we added commit f6795053dac8. But we missed hugetlbfs which could still get such high mmap() addresses. So in theory that's a potential regression that should have bee addressed at the same time as commit f6795053dac8 (and before arm64 enabled 52-bit addresses).
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 99c7477cee5c..dd3a088db11d 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -206,7 +206,7 @@ hugetlb_get_unmapped_area_bottomup(struct file *file, unsigned long addr, info.flags = 0; info.length = len; info.low_limit = current->mm->mmap_base; - info.high_limit = TASK_SIZE; + info.high_limit = arch_get_mmap_end(addr); info.align_mask = PAGE_MASK & ~huge_page_mask(h); info.align_offset = 0; return vm_unmapped_area(&info); @@ -222,7 +222,7 @@ hugetlb_get_unmapped_area_topdown(struct file *file, unsigned long addr, info.flags = VM_UNMAPPED_AREA_TOPDOWN; info.length = len; info.low_limit = max(PAGE_SIZE, mmap_min_addr); - info.high_limit = current->mm->mmap_base; + info.high_limit = arch_get_mmap_base(addr, current->mm->mmap_base); info.align_mask = PAGE_MASK & ~huge_page_mask(h); info.align_offset = 0; addr = vm_unmapped_area(&info); @@ -237,7 +237,7 @@ hugetlb_get_unmapped_area_topdown(struct file *file, unsigned long addr, VM_BUG_ON(addr != -ENOMEM); info.flags = 0; info.low_limit = current->mm->mmap_base; - info.high_limit = TASK_SIZE; + info.high_limit = arch_get_mmap_end(addr); addr = vm_unmapped_area(&info); } @@ -251,6 +251,7 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr, struct mm_struct *mm = current->mm; struct vm_area_struct *vma; struct hstate *h = hstate_file(file); + const unsigned long mmap_end = arch_get_mmap_end(addr); if (len & ~huge_page_mask(h)) return -EINVAL; @@ -266,7 +267,7 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr, if (addr) { addr = ALIGN(addr, huge_page_size(h)); vma = find_vma(mm, addr); - if (TASK_SIZE - len >= addr && + if (mmap_end - len >= addr && (!vma || addr + len <= vm_start_gap(vma))) return addr; } diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h index a80356e9dc69..1ad1f4bfa025 100644 --- a/include/linux/sched/mm.h +++ b/include/linux/sched/mm.h @@ -136,6 +136,14 @@ static inline void mm_update_next_owner(struct mm_struct *mm) #endif /* CONFIG_MEMCG */ #ifdef CONFIG_MMU +#ifndef arch_get_mmap_end +#define arch_get_mmap_end(addr) (TASK_SIZE) +#endif + +#ifndef arch_get_mmap_base +#define arch_get_mmap_base(addr, base) (base) +#endif + extern void arch_pick_mmap_layout(struct mm_struct *mm, struct rlimit *rlim_stack); extern unsigned long diff --git a/mm/mmap.c b/mm/mmap.c index 3aa839f81e63..313b57d55a63 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2117,14 +2117,6 @@ unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info) return addr; } -#ifndef arch_get_mmap_end -#define arch_get_mmap_end(addr) (TASK_SIZE) -#endif - -#ifndef arch_get_mmap_base -#define arch_get_mmap_base(addr, base) (base) -#endif - /* Get an address range which is currently unmapped. * For shmat() with addr=0. *
This is a fix for commit f6795053dac8 ("mm: mmap: Allow for "high" userspace addresses") for hugetlb. This patch adds support for "high" userspace addresses that are optionally supported on the system and have to be requested via a hint mechanism ("high" addr parameter to mmap). Architectures such as powerpc and x86 achieve this by making changes to their architectural versions of hugetlb_get_unmapped_area() function. However, arm64 uses the generic version of that function. So take into account arch_get_mmap_base() and arch_get_mmap_end() in hugetlb_get_unmapped_area(). To allow that, move those two macros out of mm/mmap.c into include/linux/sched/mm.h If these macros are not defined in architectural code then they default to (TASK_SIZE) and (base) so should not introduce any behavioural changes to architectures that do not define them. For the time being, only ARM64 is affected by this change. From Catalin (ARM64): We should have fixed hugetlb_get_unmapped_area() as well when we added support for 52-bit VA. The reason for commit f6795053dac8 was to prevent normal mmap() from returning addresses above 48-bit by default as some user-space had hard assumptions about this. It's a slight ABI change if you do this for hugetlb_get_unmapped_area() but I doubt anyone would notice. It's more likely that the current behaviour would cause issues, so I'd rather have them consistent. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> Cc: Steve Capper <steve.capper@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Fixes: f6795053dac8 ("mm: mmap: Allow for "high" userspace addresses") Cc: <stable@vger.kernel.org> # 5.0.x Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> Acked-by: Andrew Morton <akpm@linux-foundation.org> --- Resending the patch as standalone so that is can be merged via mm tree in 5.18 and stable. v10: - Moved as first patch of the series so that it can be applied separately as a flag and be easily applied back on stable. - Added text from Catalin explaining why it is a fixup. --- fs/hugetlbfs/inode.c | 9 +++++---- include/linux/sched/mm.h | 8 ++++++++ mm/mmap.c | 8 -------- 3 files changed, 13 insertions(+), 12 deletions(-)