Fix unmap_and_free_vma backout path

This may not be all we want. Even with this patch, performing such a failing map on to of an existing mapping will clobber (unmap) that pre-existing mapping. This is in contrast to the analogous situation with normal page mappings - mapping on top with a misaligned offset will fail early enough not to clobber the pre-existing mapping.

Quoted 5 lines · David Gibson

This may not be all we want. Even with this patch, performing such a failing map on to of an existing mapping will clobber (unmap) that pre-existing mapping. This is in contrast to the analogous situation with normal page mappings - mapping on top with a misaligned offset will fail early enough not to clobber the pre-existing mapping.

Then it is best to check the huge page alignment at the same place as regular alignment.

Quoted 8 lines · David Gibson

Quoted 6 lines

This may not be all we want. Even with this patch, performing such a failing map on to of an existing mapping will clobber (unmap) that pre-existing mapping. This is in contrast to the analogous situation with normal page mappings - mapping on top with a misaligned offset will fail early enough not to clobber the pre-existing mapping.

Quoted 2 lines

Then it is best to check the huge page alignment at the same place as regular alignment.

Probably, yes, although it's yet another "if (hugepage) specialcase()". But I still think we want the above patch as well. It will make sure we correctly back out from any other possible failure cases in hugetlbfs_file_mmap() - ones I haven't thought of, or which get added later.

David Gibson wrote on Sunday, November 12, 2006 9:57 PM

Quoted 9 lines · Christoph Lameter

Quoted 8 lines · David Gibson

Quoted 6 lines

This may not be all we want. Even with this patch, performing such a failing map on to of an existing mapping will clobber (unmap) that pre-existing mapping. This is in contrast to the analogous situation with normal page mappings - mapping on top with a misaligned offset will fail early enough not to clobber the pre-existing mapping.

Quoted 8 lines

Quoted 2 lines

Then it is best to check the huge page alignment at the same place as regular alignment.

Probably, yes, although it's yet another "if (hugepage) specialcase()". But I still think we want the above patch as well. It will make sure we correctly back out from any other possible failure cases in hugetlbfs_file_mmap() - ones I haven't thought of, or which get added later.

Something like this? I haven't tested it yet. But looks plausible because we already have if is_file_hugepages() in the generic path.

Quoted 7 lines

Quoted 5 lines

Probably, yes, although it's yet another "if (hugepage) specialcase()". But I still think we want the above patch as well. It will make sure we correctly back out from any other possible failure cases in hugetlbfs_file_mmap() - ones I haven't thought of, or which get added later.

Quoted 2 lines

Something like this? I haven't tested it yet. But looks plausible because we already have if is_file_hugepages() in the generic path.

Um.. if you're going to test pgoff here, you should also test the address.

Oh, and that point is too late to catch MAP_FIXED mappings.

David Gibson wrote on Sunday, November 12, 2006 10:23 PM

Quoted 6 lines

Quoted 3 lines

Something like this? I haven't tested it yet. But looks plausible because we already have if is_file_hugepages() in the generic path.

Um.. if you're going to test pgoff here, you should also test the address.

prepare_hugepage_range() should catch misaligned memory address, right? What more does get_unmapped_area() need to test?

Quoted 2 lines

Oh, and that point is too late to catch MAP_FIXED mappings.

I don't understand what you mean by that.
In do_mmap_pgoff(), very early in the code it tries to get an valid virtual address:

        addr = get_unmapped_area(file, addr, len, pgoff, flags);
        if (addr & ~PAGE_MASK)
                return addr;

We don't even have a vma at this point, there is no error to recover. If get_unmapped_area() tests the validity of pgoff and return an error code, the immediate two lines of code will catch that and everything stops there. I don't see where the unmap gets called here. Did I miss something?

Quoted 8 lines

David Gibson wrote on Sunday, November 12, 2006 10:23 PM

Quoted 6 lines

Quoted 3 lines

Something like this? I haven't tested it yet. But looks plausible because we already have if is_file_hugepages() in the generic path.

Um.. if you're going to test pgoff here, you should also test the address.

Quoted 2 lines

prepare_hugepage_range() should catch misaligned memory address, right? What more does get_unmapped_area() need to test?

David made that remark, I see now, because PowerPC alone omits to check address and length alignment in its prepare_hugepage_range:
it should be checking those as ia64 and generic do.

Quoted 10 lines

Quoted 1 line

Oh, and that point is too late to catch MAP_FIXED mappings.

I don't understand what you mean by that.
In do_mmap_pgoff(), very early in the code it tries to get an valid virtual address:

        addr = get_unmapped_area(file, addr, len, pgoff, flags);
        if (addr & ~PAGE_MASK)
                return addr;

Quoted 5 lines

We don't even have a vma at this point, there is no error to recover. If get_unmapped_area() tests the validity of pgoff and return an error code, the immediate two lines of code will catch that and everything stops there. I don't see where the unmap gets called here. Did I miss something?

I agree with Ken on that. I agree with just about everything said by people so far. But I think the check looks nicer tucked away with the other alignment checks in prepare_hugepage_range: how about this version?

(Perhaps, in another mood, I've have chosen BUG_ONs instead of just deleting all the redundant tests - another deleted in the ppc case.)

[PATCH] hugetlb: prepare_hugepage_range check offset too

prepare_hugepage_range should check file offset alignment when it checks virtual address and length, to stop MAP_FIXED with a bad huge offset from unmapping before it fails further down. PowerPC should apply the same prepare_hugepage_range alignment checks as ia64 and all the others do.

Then none of the alignment checks in hugetlbfs_file_mmap are required (nor is the check for too small a mapping); but even so, move up setting of VM_HUGETLB and add a comment to warn of what David Gibson discovered - if hugetlbfs_file_mmap fails before setting it, do_mmap_pgoff's unmap_region when unwinding from error will go the non-huge way, which may cause bad behaviour on architectures (powerpc and ia64) which segregate their huge mappings into a separate region of the address space.

Signed-off-by Hugh Dickins <[email protected]>

---

 arch/ia64/mm/hugetlbpage.c    |    4 +++-
 arch/powerpc/mm/hugetlbpage.c |    8 ++++++--
 fs/hugetlbfs/inode.c          |   21 ++++++++-------------
 include/linux/hugetlb.h       |   10 +++++++---
 mm/mmap.c                     |    2 +-
 5 files changed, 25 insertions(+), 20 deletions(-)

Looks good to me, notwithstanding the nano-nit below.

Review

Quoted 6 lines · Hugh Dickins

(quoted diff) +6 -0

+	/*
+	 * vma alignment has already been checked by prepare_hugepage_range.
+	 * If you add any error returns here, do so after setting VM_HUGETLB,
+	 * so is_vm_huge_tlb_page tests below unmap_region go the right way
+	 * when do_mmap_pgoff unwinds (may be important on powerpc and ia64).
+	 */

Sorry. This is hardly worth it, but the function referred to by this comment is actually is_vm_hugetlb_page() :-/

Quoted 12 lines · Adam Litke

Looks good to me, notwithstanding the nano-nit below.

fs/hugetlbfs/inode.c static int hugetlbfs_file_mmap(struct fi · line 70

Quoted 6 lines · Hugh Dickins

fs/hugetlbfs/inode.c +6 -0

+	/*
+	 * vma alignment has already been checked by prepare_hugepage_range.
+	 * If you add any error returns here, do so after setting VM_HUGETLB,
+	 * so is_vm_huge_tlb_page tests below unmap_region go the right way
+	 * when do_mmap_pgoff unwinds (may be important on powerpc and ia64).
+	 */

Sorry. This is hardly worth it, but the function referred to by this comment is actually is_vm_hugetlb_page() :-/

Indeed, thanks!

[PATCH] hugetlb: prepare_hugepage_range check offset too

prepare_hugepage_range should check file offset alignment when it checks virtual address and length, to stop MAP_FIXED with a bad huge offset from unmapping before it fails further down. PowerPC should apply the same prepare_hugepage_range alignment checks as ia64 and all the others do.

Then none of the alignment checks in hugetlbfs_file_mmap are required (nor is the check for too small a mapping); but even so, move up setting of VM_HUGETLB and add a comment to warn of what David Gibson discovered - if hugetlbfs_file_mmap fails before setting it, do_mmap_pgoff's unmap_region when unwinding from error will go the non-huge way, which may cause bad behaviour on architectures (powerpc and ia64) which segregate their huge mappings into a separate region of the address space.

Signed-off-by Hugh Dickins <[email protected]>

Quoted 12 lines · Adam Litke

Looks good to me, notwithstanding the nano-nit below.

fs/hugetlbfs/inode.c static int hugetlbfs_file_mmap(struct fi · line 70

Quoted 6 lines · Hugh Dickins

fs/hugetlbfs/inode.c +6 -0

+	/*
+	 * vma alignment has already been checked by prepare_hugepage_range.
+	 * If you add any error returns here, do so after setting VM_HUGETLB,
+	 * so is_vm_huge_tlb_page tests below unmap_region go the right way
+	 * when do_mmap_pgoff unwinds (may be important on powerpc and ia64).
+	 */

Sorry. This is hardly worth it, but the function referred to by this comment is actually is_vm_hugetlb_page() :-/

Indeed, thanks!

[PATCH] hugetlb: prepare_hugepage_range check offset too

prepare_hugepage_range should check file offset alignment when it checks virtual address and length, to stop MAP_FIXED with a bad huge offset from unmapping before it fails further down. PowerPC should apply the same prepare_hugepage_range alignment checks as ia64 and all the others do.

Then none of the alignment checks in hugetlbfs_file_mmap are required (nor is the check for too small a mapping); but even so, move up setting of VM_HUGETLB and add a comment to warn of what David Gibson discovered - if hugetlbfs_file_mmap fails before setting it, do_mmap_pgoff's unmap_region when unwinding from error will go the non-huge way, which may cause bad behaviour on architectures (powerpc and ia64) which segregate their huge mappings into a separate region of the address space.

[PATCH] hugetlb: prepare_hugepage_range check offset too prepare_hugepage_range should check file offset alignment when it checks virtual address and length, to stop MAP_FIXED with a bad huge offset from unmapping before it fails further down. PowerPC should apply the same prepare_hugepage_range alignment checks as ia64 and all the others do. Then none of the alignment checks in hugetlbfs_file_mmap are required (nor is the check for too small a mapping); but even so, move up setting of VM_HUGETLB and add a comment to warn of what David Gibson discovered - if hugetlbfs_file_mmap fails before setting it, do_mmap_pgoff's unmap_region when unwinding from error will go the non-huge way, which may cause bad behaviour on architectures (powerpc and ia64) which segregate their huge mappings into a separate region of the address space.

Signed-off-by Hugh Dickins <[email protected]>

David Gibson wrote on Sunday, November 12, 2006 10:23 PM

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Probably, yes, although it's yet another "if (hugepage) specialcase()". But I still think we want the above patch as well. It will make sure we correctly back out from any other possible failure cases in hugetlbfs_file_mmap() - ones I haven't thought of, or which get added later.

Quoted 5 lines

Quoted 2 lines

Something like this? I haven't tested it yet. But looks plausible because we already have if is_file_hugepages() in the generic path.

Um.. if you're going to test pgoff here, you should also test the address.

prepare_hugepage_range() should catch misaligned memory address, right? What more does get_unmapped_area() need to test?

Quoted 1 line

Oh, and that point is too late to catch MAP_FIXED mappings.

I don't understand what you mean by that.