| Message ID | 20240709092122.41232-1-donettom@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fs/hugetlbfs/inode.c: Ensure generic_hugetlb_get_unmapped_area() returns higher address than mmap_min_addr | expand |
On Tue, Jul 09, 2024 at 04:21:22AM -0500, Donet Tom wrote: > generic_hugetlb_get_unmapped_area() was returning an address less > than mmap_min_addr if the mmap argument addr, after alignment, was > less than mmap_min_addr, causing mmap to fail. > > This is because current generic_hugetlb_get_unmapped_area() code does > not take into account mmap_min_addr. > > This patch ensures that generic_hugetlb_get_unmapped_area() always returns > an address that is greater than mmap_min_addr. Additionally, similar to > generic_get_unmapped_area(), vm_end_gap() checks are included to ensure > that the address is within the limit. Hi Donet, jfyi: I am already working on other parts of the kernel to avoid hugetlb code duplication vs mm core. I am also working on getting rid of hugetlb-unmapped_area specific code [1]. I still need to perform some more tests but looks promising code-deletion-wise: arch/parisc/mm/hugetlbpage.c | 23 ------- arch/powerpc/mm/book3s64/slice.c | 49 ++++++++------ arch/s390/mm/hugetlbpage.c | 84 ------------------------ arch/s390/mm/mmap.c | 14 +++- arch/sparc/kernel/sys_sparc_32.c | 16 +++-- arch/sparc/kernel/sys_sparc_64.c | 36 ++++++++--- arch/sparc/mm/hugetlbpage.c | 108 ------------------------------- arch/x86/kernel/sys_x86_64.c | 27 +++++--- arch/x86/mm/hugetlbpage.c | 100 ---------------------------- fs/hugetlbfs/inode.c | 97 ++------------------------- include/linux/hugetlb.h | 10 +++ mm/mmap.c | 25 ++++++- 12 files changed, 139 insertions(+), 450 deletions(-) I plan to post it in a day or two. [1] https://github.com/leberus/linux/tree/hugetlb-unmapped-area
On Tue, Jul 09, 2024 at 04:21:22AM -0500, Donet Tom wrote: > generic_hugetlb_get_unmapped_area() was returning an address less > than mmap_min_addr if the mmap argument addr, after alignment, was > less than mmap_min_addr, causing mmap to fail. > > This is because current generic_hugetlb_get_unmapped_area() code does > not take into account mmap_min_addr. > > This patch ensures that generic_hugetlb_get_unmapped_area() always returns > an address that is greater than mmap_min_addr. Additionally, similar to > generic_get_unmapped_area(), vm_end_gap() checks are included to ensure > that the address is within the limit. checks are included to maintain stack gap. > > How to reproduce > ================ > > #include <stdio.h> > #include <stdlib.h> > #include <sys/mman.h> > #include <unistd.h> > > #define HUGEPAGE_SIZE (16 * 1024 * 1024) > > int main() { > > void *addr = mmap((void *)-1, HUGEPAGE_SIZE, > PROT_READ | PROT_WRITE, > MAP_SHARED | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0); > if (addr == MAP_FAILED) { > perror("mmap"); > exit(EXIT_FAILURE); > } > > snprintf((char *)addr, HUGEPAGE_SIZE, "Hello, Huge Pages!"); > > printf("%s\n", (char *)addr); > > if (munmap(addr, HUGEPAGE_SIZE) == -1) { > perror("munmap"); > exit(EXIT_FAILURE); > } > > return 0; > } > > Result without fix > ================== > # cat /proc/meminfo |grep -i HugePages_Free > HugePages_Free: 20 > # ./test > mmap: Permission denied > # > > Result with fix > =============== > # cat /proc/meminfo |grep -i HugePages_Free > HugePages_Free: 20 > # ./test > Hello, Huge Pages! > # > > V2: > Added vm_end_gap() check. > > V1: > https://lore.kernel.org/all/20240705071150.84972-1-donettom@linux.ibm.com/ > > Reported-by Pavithra Prakash <pavrampu@linux.vnet.ibm.com> > Signed-off-by: Donet Tom <donettom@linux.ibm.com> Please use "hugetlbfs:" as subject prefix. No need to spell out full path. Otherwise, Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
On 7/9/24 15:26, Oscar Salvador wrote: > On Tue, Jul 09, 2024 at 04:21:22AM -0500, Donet Tom wrote: >> generic_hugetlb_get_unmapped_area() was returning an address less >> than mmap_min_addr if the mmap argument addr, after alignment, was >> less than mmap_min_addr, causing mmap to fail. >> >> This is because current generic_hugetlb_get_unmapped_area() code does >> not take into account mmap_min_addr. >> >> This patch ensures that generic_hugetlb_get_unmapped_area() always returns >> an address that is greater than mmap_min_addr. Additionally, similar to >> generic_get_unmapped_area(), vm_end_gap() checks are included to ensure >> that the address is within the limit. > Hi Donet, > > jfyi: I am already working on other parts of the kernel to avoid hugetlb code > duplication vs mm core. > I am also working on getting rid of hugetlb-unmapped_area specific code > [1]. > I still need to perform some more tests but looks promising > code-deletion-wise: > > arch/parisc/mm/hugetlbpage.c | 23 ------- > arch/powerpc/mm/book3s64/slice.c | 49 ++++++++------ > arch/s390/mm/hugetlbpage.c | 84 ------------------------ > arch/s390/mm/mmap.c | 14 +++- > arch/sparc/kernel/sys_sparc_32.c | 16 +++-- > arch/sparc/kernel/sys_sparc_64.c | 36 ++++++++--- > arch/sparc/mm/hugetlbpage.c | 108 ------------------------------- > arch/x86/kernel/sys_x86_64.c | 27 +++++--- > arch/x86/mm/hugetlbpage.c | 100 ---------------------------- > fs/hugetlbfs/inode.c | 97 ++------------------------- > include/linux/hugetlb.h | 10 +++ > mm/mmap.c | 25 ++++++- > 12 files changed, 139 insertions(+), 450 deletions(-) > > I plan to post it in a day or two. > > [1] https://github.com/leberus/linux/tree/hugetlb-unmapped-area Thank you Oscar. The issue I am trying to fix will also get fixed by this new changes right. So should we drop my patch or should we continue it? -Donet
On Tue, Jul 09, 2024 at 07:14:38PM +0530, Donet Tom wrote: > Thank you Oscar. Hi Donet, > The issue I am trying to fix will also get fixed by this new changes right. > So should we drop my patch or should we continue it? I would keep this patch as it is easier to backport as a small fixup. Thanks
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 412f295acebe..cdd8e53ddd19 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -222,13 +222,13 @@ generic_hugetlb_get_unmapped_area(struct file *file, unsigned long addr, unsigned long flags) { struct mm_struct *mm = current->mm; - struct vm_area_struct *vma; + struct vm_area_struct *vma, *prev; struct hstate *h = hstate_file(file); const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags); if (len & ~huge_page_mask(h)) return -EINVAL; - if (len > TASK_SIZE) + if (len > mmap_end - mmap_min_addr) return -ENOMEM; if (flags & MAP_FIXED) { @@ -239,9 +239,10 @@ generic_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 (mmap_end - len >= addr && - (!vma || addr + len <= vm_start_gap(vma))) + vma = find_vma_prev(mm, addr, &prev); + if (mmap_end - len >= addr && addr >= mmap_min_addr && + (!vma || addr + len <= vm_start_gap(vma)) && + (!prev || addr >= vm_end_gap(prev))) return addr; }
generic_hugetlb_get_unmapped_area() was returning an address less than mmap_min_addr if the mmap argument addr, after alignment, was less than mmap_min_addr, causing mmap to fail. This is because current generic_hugetlb_get_unmapped_area() code does not take into account mmap_min_addr. This patch ensures that generic_hugetlb_get_unmapped_area() always returns an address that is greater than mmap_min_addr. Additionally, similar to generic_get_unmapped_area(), vm_end_gap() checks are included to ensure that the address is within the limit. How to reproduce ================ #include <stdio.h> #include <stdlib.h> #include <sys/mman.h> #include <unistd.h> #define HUGEPAGE_SIZE (16 * 1024 * 1024) int main() { void *addr = mmap((void *)-1, HUGEPAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0); if (addr == MAP_FAILED) { perror("mmap"); exit(EXIT_FAILURE); } snprintf((char *)addr, HUGEPAGE_SIZE, "Hello, Huge Pages!"); printf("%s\n", (char *)addr); if (munmap(addr, HUGEPAGE_SIZE) == -1) { perror("munmap"); exit(EXIT_FAILURE); } return 0; } Result without fix ================== # cat /proc/meminfo |grep -i HugePages_Free HugePages_Free: 20 # ./test mmap: Permission denied # Result with fix =============== # cat /proc/meminfo |grep -i HugePages_Free HugePages_Free: 20 # ./test Hello, Huge Pages! # V2: Added vm_end_gap() check. V1: https://lore.kernel.org/all/20240705071150.84972-1-donettom@linux.ibm.com/ Reported-by Pavithra Prakash <pavrampu@linux.vnet.ibm.com> Signed-off-by: Donet Tom <donettom@linux.ibm.com> --- fs/hugetlbfs/inode.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)