[PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

In case of 5-level paging, we don't put any mapping above 47-bit, unless
userspace explicitly asked for it.

Userspace can ask for allocation from full address space by specifying
hint address above 47-bit.

Nicholas noticed that current implementation violates this interface:
we can get vma partly in high addresses if we ask for a mapping at very
end of 47-bit address space.

Let's make sure that, when consider hint address for non-MAP_FIXED
mapping, start and end of resulting vma are on the same side of 47-bit
border.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Nicholas Piggin <npiggin@gmail.com>
---
 v2:
   - add a comment to explain behaviour;
   - cover hugetlb case too;
---
 arch/x86/kernel/sys_x86_64.c | 36 ++++++++++++++++++++++++++++++++++--
 arch/x86/mm/hugetlbpage.c    | 13 +++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index a63fe77b3217..472de4a9f0a6 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -198,11 +198,43 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	/* requesting a specific address */
 	if (addr) {
 		addr = PAGE_ALIGN(addr);
+		if (TASK_SIZE - len < addr)
+			goto get_unmapped_area;
+
+		/*
+		 * We don't want to put a mapping directly accross 47-bit
+		 * boundary. It helps to address following theoretical issue:
+		 *
+		 * We have an application that tries, for some reason, to
+		 * allocate memory with mmap(addr), without MAP_FIXED, where addr
+		 * is near the borderline of 47-bit address space and addr+len is
+		 * above the border.
+		 *
+		 * On 4-level paging machine this request would succeed, but the
+		 * address will always be within 47-bit VA -- cannot allocate by
+		 * hint address, ignore it.
+		 *
+		 * If the application cannot handle high address this might be an
+		 * issue on 5-level paging machine as such call would succeed
+		 * *and* allocate memory by the specified hint address. In this
+		 * case part of the mapping would be above the border line and
+		 * may lead to misbehaviour if the application cannot handle
+		 * addresses above 47-bit.
+		 *
+		 * Note, that mmap(addr, MAP_FIXED) would fail on 4-level
+		 * paging machine if addr+len is above 47-bit. It's reasonable
+		 * to expect that nobody would rely on such failure and it's
+		 * safe to allocate such mapping.
+		 */
+		if ((addr > DEFAULT_MAP_WINDOW) !=
+				(addr + len > DEFAULT_MAP_WINDOW))
+			goto get_unmapped_area;
+
 		vma = find_vma(mm, addr);
-		if (TASK_SIZE - len >= addr &&
-				(!vma || addr + len <= vm_start_gap(vma)))
+		if (!vma || addr + len <= vm_start_gap(vma))
 			return addr;
 	}
+get_unmapped_area:
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
index 8ae0000cbdb3..5cdcb3ee9748 100644
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c
@@ -166,11 +166,20 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
 
 	if (addr) {
 		addr = ALIGN(addr, huge_page_size(h));
+		if (TASK_SIZE - len >= addr)
+			goto get_unmapped_area;
+
+		/* See a comment in arch_get_unmapped_area_topdown */
+		if ((addr > DEFAULT_MAP_WINDOW) !=
+				(addr + len > DEFAULT_MAP_WINDOW))
+			goto get_unmapped_area;
+
 		vma = find_vma(mm, addr);
-		if (TASK_SIZE - len >= addr &&
-		    (!vma || addr + len <= vm_start_gap(vma)))
+		if (!vma || addr + len <= vm_start_gap(vma))
 			return addr;
 	}
+
+get_unmapped_area:
 	if (mm->get_unmapped_area == arch_get_unmapped_area)
 		return hugetlb_get_unmapped_area_bottomup(file, addr, len,
 				pgoff, flags);
-- 
2.15.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCHv2 2/2] x86/selftests: Add test for mapping placement for 5-level paging

[Kirill A. Shutemov]

With 5-level paging, we have 56-bit virtual address space available for
userspace. But we don't want to expose userspace to addresses above
47-bits, unless it asked specifically for it.

We use mmap(2) hint address as a way for kernel to know if it's okay to
allocate virtual memory above 47-bit.

Let's add a self-test that covers few corner cases of the interface.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 tools/testing/selftests/x86/5lvl.c   | 53 ++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/x86/Makefile |  2 +-
 2 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/x86/5lvl.c

diff --git a/tools/testing/selftests/x86/5lvl.c b/tools/testing/selftests/x86/5lvl.c
new file mode 100644
index 000000000000..94610fd13ba2
--- /dev/null
+++ b/tools/testing/selftests/x86/5lvl.c
@@ -0,0 +1,53 @@
+#include <stdio.h>
+#include <sys/mman.h>
+
+#define PAGE_SIZE	4096
+#define SIZE		(2 * PAGE_SIZE)
+#define LOW_ADDR	((void *) (1UL << 30))
+#define HIGH_ADDR	((void *) (1UL << 50))
+#define TASK_SIZE	((void *) (1UL << 47))
+
+int main(int argc, char **argv)
+{
+	void *p;
+
+	p = mmap(NULL, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	printf("mmap(NULL): %p %s\n", p, p > TASK_SIZE ? "FAILED!" : "");
+
+	p = mmap(LOW_ADDR, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	printf("mmap(%p): %p %s\n", LOW_ADDR, p,
+			p > TASK_SIZE ? "FAILED!" : "");
+
+	p = mmap(HIGH_ADDR, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	printf("mmap(%p): %p\n", HIGH_ADDR, p);
+
+	p = mmap(HIGH_ADDR, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	printf("mmap(%p) again: %p\n", HIGH_ADDR, p);
+
+	p = mmap(HIGH_ADDR, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
+	printf("mmap(%p, MAP_FIXED): %p\n", HIGH_ADDR, p);
+
+	p = mmap((void *)-1, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	printf("mmap(-1): %p\n", p);
+
+	p = mmap((void *)-1, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	printf("mmap(-1) again: %p\n", p);
+
+	p = mmap(TASK_SIZE - PAGE_SIZE, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	printf("mmap(%p, %d): %p %s\n", TASK_SIZE - PAGE_SIZE, SIZE, p,
+			p > TASK_SIZE ? "FAILED!" : "");
+
+	p = mmap(TASK_SIZE - PAGE_SIZE, SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
+	printf("mmap(TASK_SIZE - PAGE_SIZE, MAP_FIXED): %p\n", p);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index 7b1adeee4b0f..939a337128db 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -11,7 +11,7 @@ TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_sysc
 TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
-TARGETS_C_64BIT_ONLY := fsgsbase sysret_rip
+TARGETS_C_64BIT_ONLY := fsgsbase sysret_rip 5lvl
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)
 TARGETS_C_64BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_64BIT_ONLY)
-- 
2.15.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Thomas Gleixner]

On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -166,11 +166,20 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>  
>  	if (addr) {
>  		addr = ALIGN(addr, huge_page_size(h));
> +		if (TASK_SIZE - len >= addr)
> +			goto get_unmapped_area;

That's wrong. You got it right in arch_get_unmapped_area_topdown() ...

> +
> +		/* See a comment in arch_get_unmapped_area_topdown */

This is lame, really.

> +		if ((addr > DEFAULT_MAP_WINDOW) !=
> +				(addr + len > DEFAULT_MAP_WINDOW))
> +			goto get_unmapped_area;

Instead of duplicating that horrible formatted condition and adding this
lousy comment why can't you just put all of it (including the TASK_SIZE
check) into a proper validation function and put the comment there?

The fixed up variant of your patch below does that.

Aside of that please spend a bit more time on describing things precisely
at the technical and factual level next time. I fixed that up (once more)
both in the comment and the changelog.

Please double check.

Thanks,

	tglx

8<----------------
Subject: x86/mm: Prevent non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Tue, 14 Nov 2017 16:43:21 +0300

In case of 5-level paging, the kernel does not place any mapping above
47-bit, unless userspace explicitly asks for it.

Userspace can request an allocation from the full address space by
specifying the mmap address hint above 47-bit.

Nicholas noticed that the current implementation violates this interface:

  If user space requests a mapping at the end of the 47-bit address space
  with a length which causes the mapping to cross the 47-bit border
  (DEFAULT_MAP_WINDOW), then the vma is partially in the address space
  below and above.

Sanity check the mmap address hint so that start and end of the resulting
vma are on the same side of the 47-bit border. If that's not the case fall
back to the code path which ignores the address hint and allocate from the
regular address space below 47-bit.

[ tglx: Moved the address check to a function and massaged comment and
  	changelog ]

Reported-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: linux-mm@kvack.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lkml.kernel.org/r/20171114134322.40321-1-kirill.shutemov@linux.intel.com

---
 arch/x86/include/asm/elf.h   |    1 
 arch/x86/kernel/sys_x86_64.c |    8 +++++--
 arch/x86/mm/hugetlbpage.c    |    9 ++++++-
 arch/x86/mm/mmap.c           |   49 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 4 deletions(-)

--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -309,6 +309,7 @@ static inline int mmap_is_ia32(void)
 extern unsigned long task_size_32bit(void);
 extern unsigned long task_size_64bit(int full_addr_space);
 extern unsigned long get_mmap_base(int is_legacy);
+extern bool mmap_address_hint_valid(unsigned long addr, unsigned long len);
 
 #ifdef CONFIG_X86_32
 
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -188,6 +188,7 @@ arch_get_unmapped_area_topdown(struct fi
 	if (len > TASK_SIZE)
 		return -ENOMEM;
 
+	/* No address checking. See comment at mmap_address_hint_valid() */
 	if (flags & MAP_FIXED)
 		return addr;
 
@@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
 	/* requesting a specific address */
 	if (addr) {
 		addr = PAGE_ALIGN(addr);
+		if (!mmap_address_hint_valid(addr, len))
+			goto get_unmapped_area;
+
 		vma = find_vma(mm, addr);
-		if (TASK_SIZE - len >= addr &&
-				(!vma || addr + len <= vm_start_gap(vma)))
+		if (!vma || addr + len <= vm_start_gap(vma))
 			return addr;
 	}
+get_unmapped_area:
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c
@@ -158,6 +158,7 @@ hugetlb_get_unmapped_area(struct file *f
 	if (len > TASK_SIZE)
 		return -ENOMEM;
 
+	/* No address checking. See comment at mmap_address_hint_valid() */
 	if (flags & MAP_FIXED) {
 		if (prepare_hugepage_range(file, addr, len))
 			return -EINVAL;
@@ -166,11 +167,15 @@ hugetlb_get_unmapped_area(struct file *f
 
 	if (addr) {
 		addr = ALIGN(addr, huge_page_size(h));
+		if (!mmap_address_hint_valid(addr, len))
+			goto get_unmapped_area;
+
 		vma = find_vma(mm, addr);
-		if (TASK_SIZE - len >= addr &&
-		    (!vma || addr + len <= vm_start_gap(vma)))
+		if (!vma || addr + len <= vm_start_gap(vma))
 			return addr;
 	}
+
+get_unmapped_area:
 	if (mm->get_unmapped_area == arch_get_unmapped_area)
 		return hugetlb_get_unmapped_area_bottomup(file, addr, len,
 				pgoff, flags);
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -174,3 +174,52 @@ const char *arch_vma_name(struct vm_area
 		return "[mpx]";
 	return NULL;
 }
+
+/**
+ * mmap_address_hint_valid - Validate the address hint of mmap
+ * @addr:	Address hint
+ * @len:	Mapping length
+ *
+ * Check whether @addr and @addr + @len result in a valid mapping.
+ *
+ * On 32bit this only checks whether @addr + @len is <= TASK_SIZE.
+ *
+ * On 64bit with 5-level page tables another sanity check is required
+ * because mappings requested by mmap(@addr, 0) which cross the 47-bit
+ * virtual address boundary can cause the following theoretical issue:
+ *
+ *  An application calls mmap(addr, 0), i.e. without MAP_FIXED, where @addr
+ *  is below the border of the 47-bit address space and @addr + @len is
+ *  above the border.
+ *
+ *  With 4-level paging this request succeeds, but the resulting mapping
+ *  address will always be within the 47-bit virtual address space, because
+ *  the hint address does not result in a valid mapping and is
+ *  ignored. Hence applications which are not prepared to handle virtual
+ *  addresses above 47-bit work correctly.
+ *
+ *  With 5-level paging this request would be granted and result in a
+ *  mapping which crosses the border of the 47-bit virtual address
+ *  space. If the application cannot handle addresses above 47-bit this
+ *  will lead to misbehaviour and hard to diagnose failures.
+ *
+ * Therefore ignore address hints which would result in a mapping crossing
+ * the 47-bit virtual address boundary.
+ *
+ * Note, that in the same scenario with MAP_FIXED the behaviour is
+ * different. The request with @addr < 47-bit and @addr + @len > 47-bit
+ * fails on a 4-level paging machine but succeeds on a 5-level paging
+ * machine. It is reasonable to expect that an application does not rely on
+ * the failure of such a fixed mapping request, so the restriction is not
+ * applied.
+ */
+bool mmap_address_hint_valid(unsigned long addr, unsigned long len)
+{
+	if (TASK_SIZE - len < addr)
+		return false;
+#if CONFIG_PGTABLE_LEVELS >= 5
+	return (addr > DEFAULT_MAP_WINDOW) == (addr + len > DEFAULT_MAP_WINDOW);
+#else
+	return true;
+#endif
+}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> > --- a/arch/x86/mm/hugetlbpage.c
> > +++ b/arch/x86/mm/hugetlbpage.c
> > @@ -166,11 +166,20 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> >  
> >  	if (addr) {
> >  		addr = ALIGN(addr, huge_page_size(h));
> > +		if (TASK_SIZE - len >= addr)
> > +			goto get_unmapped_area;
> 
> That's wrong. You got it right in arch_get_unmapped_area_topdown() ...

Ouch.

Please ignore selftest patch. I'll rework it to cover hugetlb.

> > +
> > +		/* See a comment in arch_get_unmapped_area_topdown */
> 
> This is lame, really.
> 
> > +		if ((addr > DEFAULT_MAP_WINDOW) !=
> > +				(addr + len > DEFAULT_MAP_WINDOW))
> > +			goto get_unmapped_area;
> 
> Instead of duplicating that horrible formatted condition and adding this
> lousy comment why can't you just put all of it (including the TASK_SIZE
> check) into a proper validation function and put the comment there?
> 
> The fixed up variant of your patch below does that.
> 
> Aside of that please spend a bit more time on describing things precisely
> at the technical and factual level next time. I fixed that up (once more)
> both in the comment and the changelog.
> 
> Please double check.

Works fine.

> +bool mmap_address_hint_valid(unsigned long addr, unsigned long len)
> +{
> +	if (TASK_SIZE - len < addr)
> +		return false;
> +#if CONFIG_PGTABLE_LEVELS >= 5
> +	return (addr > DEFAULT_MAP_WINDOW) == (addr + len > DEFAULT_MAP_WINDOW);

Is it micro optimization? I don't feel it necessary. It's not that hot
codepath to care about few cycles. (And one more place to care about for
boot-time switching.)

If you think it's needed, maybe IS_ENABLED() instead?

> +#else
> +	return true;
> +#endif
> +}

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Thomas Gleixner]

On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > +bool mmap_address_hint_valid(unsigned long addr, unsigned long len)
> > +{
> > +	if (TASK_SIZE - len < addr)
> > +		return false;
> > +#if CONFIG_PGTABLE_LEVELS >= 5
> > +	return (addr > DEFAULT_MAP_WINDOW) == (addr + len > DEFAULT_MAP_WINDOW);
> 
> Is it micro optimization? I don't feel it necessary. It's not that hot
> codepath to care about few cycles. (And one more place to care about for
> boot-time switching.)
> 
> If you think it's needed, maybe IS_ENABLED() instead?

You're right. It's can be unconditional, For page table levels < 5 its just
redundant as its covered by the TASK_SIZE check already.

Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
>  	/* requesting a specific address */
>  	if (addr) {
>  		addr = PAGE_ALIGN(addr);
> +		if (!mmap_address_hint_valid(addr, len))
> +			goto get_unmapped_area;
> +

Here and in hugetlb_get_unmapped_area(), we should align the addr after
the check, not before. Otherwise the alignment itself can bring us over
the borderline as we align up.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Thomas Gleixner]

On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:

> On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> >  	/* requesting a specific address */
> >  	if (addr) {
> >  		addr = PAGE_ALIGN(addr);
> > +		if (!mmap_address_hint_valid(addr, len))
> > +			goto get_unmapped_area;
> > +
> 
> Here and in hugetlb_get_unmapped_area(), we should align the addr after
> the check, not before. Otherwise the alignment itself can bring us over
> the borderline as we align up.

Hmm, then I wonder whether the next check against vm_start_gap() which
checks against the aligned address is correct:

                addr = PAGE_ALIGN(addr);
                vma = find_vma(mm, addr);

                if (end - len >= addr &&
                    (!vma || addr + len <= vm_start_gap(vma)))
                        return addr;

Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Tue, Nov 14, 2017 at 09:54:52PM +0100, Thomas Gleixner wrote:
> On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> 
> > On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> > >  	/* requesting a specific address */
> > >  	if (addr) {
> > >  		addr = PAGE_ALIGN(addr);
> > > +		if (!mmap_address_hint_valid(addr, len))
> > > +			goto get_unmapped_area;
> > > +
> > 
> > Here and in hugetlb_get_unmapped_area(), we should align the addr after
> > the check, not before. Otherwise the alignment itself can bring us over
> > the borderline as we align up.
> 
> Hmm, then I wonder whether the next check against vm_start_gap() which
> checks against the aligned address is correct:
> 
>                 addr = PAGE_ALIGN(addr);
>                 vma = find_vma(mm, addr);
> 
>                 if (end - len >= addr &&
>                     (!vma || addr + len <= vm_start_gap(vma)))
>                         return addr;

I think the check is correct. The check is against resulting addresses
that end up in vm_start/vm_end. In our case we want to figure out what
user asked for.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Thomas Gleixner]

On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> On Tue, Nov 14, 2017 at 09:54:52PM +0100, Thomas Gleixner wrote:
> > On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> > 
> > > On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > > > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> > > >  	/* requesting a specific address */
> > > >  	if (addr) {
> > > >  		addr = PAGE_ALIGN(addr);
> > > > +		if (!mmap_address_hint_valid(addr, len))
> > > > +			goto get_unmapped_area;
> > > > +
> > > 
> > > Here and in hugetlb_get_unmapped_area(), we should align the addr after
> > > the check, not before. Otherwise the alignment itself can bring us over
> > > the borderline as we align up.
> > 
> > Hmm, then I wonder whether the next check against vm_start_gap() which
> > checks against the aligned address is correct:
> > 
> >                 addr = PAGE_ALIGN(addr);
> >                 vma = find_vma(mm, addr);
> > 
> >                 if (end - len >= addr &&
> >                     (!vma || addr + len <= vm_start_gap(vma)))
> >                         return addr;
> 
> I think the check is correct. The check is against resulting addresses
> that end up in vm_start/vm_end. In our case we want to figure out what
> user asked for.

Well, but then checking just against the user supplied addr is only half of
the story.

    addr = boundary - PAGE_SIZE - PAGE_SIZE / 2;
    len = PAGE_SIZE - PAGE_SIZE / 2;

That fits, but then after alignment we end up with

    addr = boudary - PAGE_SIZE;

and due to len > PAGE_SIZE this will result in a mapping which crosses the
boundary, right? So checking against the PAGE_ALIGN(addr) should be the
right thing to do.

Thanks,

	tglx



    

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Wed, Nov 15, 2017 at 12:00:46AM +0100, Thomas Gleixner wrote:
> On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > On Tue, Nov 14, 2017 at 09:54:52PM +0100, Thomas Gleixner wrote:
> > > On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> > > 
> > > > On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > > > > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> > > > >  	/* requesting a specific address */
> > > > >  	if (addr) {
> > > > >  		addr = PAGE_ALIGN(addr);
> > > > > +		if (!mmap_address_hint_valid(addr, len))
> > > > > +			goto get_unmapped_area;
> > > > > +
> > > > 
> > > > Here and in hugetlb_get_unmapped_area(), we should align the addr after
> > > > the check, not before. Otherwise the alignment itself can bring us over
> > > > the borderline as we align up.
> > > 
> > > Hmm, then I wonder whether the next check against vm_start_gap() which
> > > checks against the aligned address is correct:
> > > 
> > >                 addr = PAGE_ALIGN(addr);
> > >                 vma = find_vma(mm, addr);
> > > 
> > >                 if (end - len >= addr &&
> > >                     (!vma || addr + len <= vm_start_gap(vma)))
> > >                         return addr;
> > 
> > I think the check is correct. The check is against resulting addresses
> > that end up in vm_start/vm_end. In our case we want to figure out what
> > user asked for.
> 
> Well, but then checking just against the user supplied addr is only half of
> the story.
> 
>     addr = boundary - PAGE_SIZE - PAGE_SIZE / 2;
>     len = PAGE_SIZE - PAGE_SIZE / 2;
> 
> That fits, but then after alignment we end up with
> 
>     addr = boudary - PAGE_SIZE;
> 
> and due to len > PAGE_SIZE this will result in a mapping which crosses the
> boundary, right? So checking against the PAGE_ALIGN(addr) should be the
> right thing to do.

IIUC, this is only the case if 'len' is not aligned, right?

>From what I see we expect caller to align it (and mm/mmap.c does this, I
haven't checked other callers).

And hugetlb would actively reject non-aligned len.

I *think* we should be fine with checking unaligned 'addr'.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Thomas Gleixner]

On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> On Wed, Nov 15, 2017 at 12:00:46AM +0100, Thomas Gleixner wrote:
> > On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > > On Tue, Nov 14, 2017 at 09:54:52PM +0100, Thomas Gleixner wrote:
> > > > On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> > > > 
> > > > > On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > > > > > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> > > > > >  	/* requesting a specific address */
> > > > > >  	if (addr) {
> > > > > >  		addr = PAGE_ALIGN(addr);
> > > > > > +		if (!mmap_address_hint_valid(addr, len))
> > > > > > +			goto get_unmapped_area;
> > > > > > +
> > > > > 
> > > > > Here and in hugetlb_get_unmapped_area(), we should align the addr after
> > > > > the check, not before. Otherwise the alignment itself can bring us over
> > > > > the borderline as we align up.
> > > > 
> > > > Hmm, then I wonder whether the next check against vm_start_gap() which
> > > > checks against the aligned address is correct:
> > > > 
> > > >                 addr = PAGE_ALIGN(addr);
> > > >                 vma = find_vma(mm, addr);
> > > > 
> > > >                 if (end - len >= addr &&
> > > >                     (!vma || addr + len <= vm_start_gap(vma)))
> > > >                         return addr;
> > > 
> > > I think the check is correct. The check is against resulting addresses
> > > that end up in vm_start/vm_end. In our case we want to figure out what
> > > user asked for.
> > 
> > Well, but then checking just against the user supplied addr is only half of
> > the story.
> > 
> >     addr = boundary - PAGE_SIZE - PAGE_SIZE / 2;
> >     len = PAGE_SIZE - PAGE_SIZE / 2;
> > 
> > That fits, but then after alignment we end up with
> > 
> >     addr = boudary - PAGE_SIZE;
> > 
> > and due to len > PAGE_SIZE this will result in a mapping which crosses the
> > boundary, right? So checking against the PAGE_ALIGN(addr) should be the
> > right thing to do.
> 
> IIUC, this is only the case if 'len' is not aligned, right?
> 
> >From what I see we expect caller to align it (and mm/mmap.c does this, I
> haven't checked other callers).
> 
> And hugetlb would actively reject non-aligned len.
> 
> I *think* we should be fine with checking unaligned 'addr'.

I think we should keep it consistent for the normal and the huge case and
just check aligned and be done with it.

Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Wed, Nov 15, 2017 at 12:39:40PM +0100, Thomas Gleixner wrote:
> On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > On Wed, Nov 15, 2017 at 12:00:46AM +0100, Thomas Gleixner wrote:
> > > On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > > > On Tue, Nov 14, 2017 at 09:54:52PM +0100, Thomas Gleixner wrote:
> > > > > On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> > > > > 
> > > > > > On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > > > > > > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> > > > > > >  	/* requesting a specific address */
> > > > > > >  	if (addr) {
> > > > > > >  		addr = PAGE_ALIGN(addr);
> > > > > > > +		if (!mmap_address_hint_valid(addr, len))
> > > > > > > +			goto get_unmapped_area;
> > > > > > > +
> > > > > > 
> > > > > > Here and in hugetlb_get_unmapped_area(), we should align the addr after
> > > > > > the check, not before. Otherwise the alignment itself can bring us over
> > > > > > the borderline as we align up.
> > > > > 
> > > > > Hmm, then I wonder whether the next check against vm_start_gap() which
> > > > > checks against the aligned address is correct:
> > > > > 
> > > > >                 addr = PAGE_ALIGN(addr);
> > > > >                 vma = find_vma(mm, addr);
> > > > > 
> > > > >                 if (end - len >= addr &&
> > > > >                     (!vma || addr + len <= vm_start_gap(vma)))
> > > > >                         return addr;
> > > > 
> > > > I think the check is correct. The check is against resulting addresses
> > > > that end up in vm_start/vm_end. In our case we want to figure out what
> > > > user asked for.
> > > 
> > > Well, but then checking just against the user supplied addr is only half of
> > > the story.
> > > 
> > >     addr = boundary - PAGE_SIZE - PAGE_SIZE / 2;
> > >     len = PAGE_SIZE - PAGE_SIZE / 2;
> > > 
> > > That fits, but then after alignment we end up with
> > > 
> > >     addr = boudary - PAGE_SIZE;
> > > 
> > > and due to len > PAGE_SIZE this will result in a mapping which crosses the
> > > boundary, right? So checking against the PAGE_ALIGN(addr) should be the
> > > right thing to do.
> > 
> > IIUC, this is only the case if 'len' is not aligned, right?
> > 
> > >From what I see we expect caller to align it (and mm/mmap.c does this, I
> > haven't checked other callers).
> > 
> > And hugetlb would actively reject non-aligned len.
> > 
> > I *think* we should be fine with checking unaligned 'addr'.
> 
> I think we should keep it consistent for the normal and the huge case and
> just check aligned and be done with it.

Aligned 'addr'? Or 'len'? Both?

We would have problem with checking aligned addr. I steped it in hugetlb
case:

  - User asks for mmap((1UL << 47) - PAGE_SIZE, 2 << 20, MAP_HUGETLB);

  - On 4-level paging machine this gives us invalid hint address as
    'TASK_SIZE - len' is more than 'addr'. Goto get_unmapped_area.

  - On 5-level paging machine hint address gets rounded up to next 2MB
    boundary that is exactly 1UL << 47 and we happily allocate from full
    address space which may lead to trouble.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Wed, Nov 15, 2017 at 03:10:42PM +0300, Kirill A. Shutemov wrote:
> On Wed, Nov 15, 2017 at 12:39:40PM +0100, Thomas Gleixner wrote:
> > On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > > On Wed, Nov 15, 2017 at 12:00:46AM +0100, Thomas Gleixner wrote:
> > > > On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > > > > On Tue, Nov 14, 2017 at 09:54:52PM +0100, Thomas Gleixner wrote:
> > > > > > On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> > > > > > 
> > > > > > > On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > > > > > > > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> > > > > > > >  	/* requesting a specific address */
> > > > > > > >  	if (addr) {
> > > > > > > >  		addr = PAGE_ALIGN(addr);
> > > > > > > > +		if (!mmap_address_hint_valid(addr, len))
> > > > > > > > +			goto get_unmapped_area;
> > > > > > > > +
> > > > > > > 
> > > > > > > Here and in hugetlb_get_unmapped_area(), we should align the addr after
> > > > > > > the check, not before. Otherwise the alignment itself can bring us over
> > > > > > > the borderline as we align up.
> > > > > > 
> > > > > > Hmm, then I wonder whether the next check against vm_start_gap() which
> > > > > > checks against the aligned address is correct:
> > > > > > 
> > > > > >                 addr = PAGE_ALIGN(addr);
> > > > > >                 vma = find_vma(mm, addr);
> > > > > > 
> > > > > >                 if (end - len >= addr &&
> > > > > >                     (!vma || addr + len <= vm_start_gap(vma)))
> > > > > >                         return addr;
> > > > > 
> > > > > I think the check is correct. The check is against resulting addresses
> > > > > that end up in vm_start/vm_end. In our case we want to figure out what
> > > > > user asked for.
> > > > 
> > > > Well, but then checking just against the user supplied addr is only half of
> > > > the story.
> > > > 
> > > >     addr = boundary - PAGE_SIZE - PAGE_SIZE / 2;
> > > >     len = PAGE_SIZE - PAGE_SIZE / 2;
> > > > 
> > > > That fits, but then after alignment we end up with
> > > > 
> > > >     addr = boudary - PAGE_SIZE;
> > > > 
> > > > and due to len > PAGE_SIZE this will result in a mapping which crosses the
> > > > boundary, right? So checking against the PAGE_ALIGN(addr) should be the
> > > > right thing to do.
> > > 
> > > IIUC, this is only the case if 'len' is not aligned, right?
> > > 
> > > >From what I see we expect caller to align it (and mm/mmap.c does this, I
> > > haven't checked other callers).
> > > 
> > > And hugetlb would actively reject non-aligned len.
> > > 
> > > I *think* we should be fine with checking unaligned 'addr'.
> > 
> > I think we should keep it consistent for the normal and the huge case and
> > just check aligned and be done with it.
> 
> Aligned 'addr'? Or 'len'? Both?
> 
> We would have problem with checking aligned addr. I steped it in hugetlb
> case:
> 
>   - User asks for mmap((1UL << 47) - PAGE_SIZE, 2 << 20, MAP_HUGETLB);
> 
>   - On 4-level paging machine this gives us invalid hint address as
>     'TASK_SIZE - len' is more than 'addr'. Goto get_unmapped_area.
> 
>   - On 5-level paging machine hint address gets rounded up to next 2MB
>     boundary that is exactly 1UL << 47 and we happily allocate from full
>     address space which may lead to trouble.

Below is updated patch with self-test.

Output on 5-level paging machine:

mmap(NULL): 0x7fbbad1f3000 - OK
mmap(LOW_ADDR): 0x40000000 - OK
mmap(HIGH_ADDR): 0x4000000000000 - OK
mmap(HIGH_ADDR) again: 0xffffbbad1fb000 - OK
mmap(HIGH_ADDR, MAP_FIXED): 0x4000000000000 - OK
mmap(-1): 0xffffbbad1f9000 - OK
mmap(-1) again: 0xffffbbad1f7000 - OK
mmap((1UL << 47), 2 * PAGE_SIZE): 0x7fbbad1f3000 - OK
mmap((1UL << 47), 2 * PAGE_SIZE / 2): 0x7fbbad1f1000 - OK
mmap((1UL << 47) - PAGE_SIZE, 2 * PAGE_SIZE, MAP_FIXED): 0x7ffffffff000 - OK
mmap(NULL, MAP_HUGETLB): 0x7fbbac400000 - OK
mmap(LOW_ADDR, MAP_HUGETLB): 0x40000000 - OK
mmap(HIGH_ADDR, MAP_HUGETLB): 0x4000000000000 - OK
mmap(HIGH_ADDR, MAP_HUGETLB) again: 0xffffbbace00000 - OK
mmap(HIGH_ADDR, MAP_FIXED | MAP_HUGETLB): 0x4000000000000 - OK
mmap(-1, MAP_HUGETLB): (nil) - OK
mmap(-1, MAP_HUGETLB) again: 0x7fbbac400000 - OK
mmap((1UL << 47), 2UL << 20, MAP_HUGETLB): 0x800000000000 - FAILED
mmap((1UL << 47) - (2UL << 20), 4UL << 20, MAP_FIXED | MAP_HUGETLB): 0x7fffffe00000 - OK

So, only hugetlb is problematic. mmap() aligns addr to PAGE_SIZE.
See round_hint_to_min(). In this case we round address *down* and it works
fine.

Replacing 'addr = ALIGN(addr, huge_page_size(h))' in hugetlbpage.c with
'addr &= huge_page_mask(h)' fixes the issue.

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Wed, Nov 15, 2017 at 05:04:26PM +0300, Kirill A. Shutemov wrote:
> On Wed, Nov 15, 2017 at 03:10:42PM +0300, Kirill A. Shutemov wrote:
> > On Wed, Nov 15, 2017 at 12:39:40PM +0100, Thomas Gleixner wrote:
> > > On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > > > On Wed, Nov 15, 2017 at 12:00:46AM +0100, Thomas Gleixner wrote:
> > > > > On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > > > > > On Tue, Nov 14, 2017 at 09:54:52PM +0100, Thomas Gleixner wrote:
> > > > > > > On Tue, 14 Nov 2017, Kirill A. Shutemov wrote:
> > > > > > > 
> > > > > > > > On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote:
> > > > > > > > > @@ -198,11 +199,14 @@ arch_get_unmapped_area_topdown(struct fi
> > > > > > > > >  	/* requesting a specific address */
> > > > > > > > >  	if (addr) {
> > > > > > > > >  		addr = PAGE_ALIGN(addr);
> > > > > > > > > +		if (!mmap_address_hint_valid(addr, len))
> > > > > > > > > +			goto get_unmapped_area;
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > Here and in hugetlb_get_unmapped_area(), we should align the addr after
> > > > > > > > the check, not before. Otherwise the alignment itself can bring us over
> > > > > > > > the borderline as we align up.
> > > > > > > 
> > > > > > > Hmm, then I wonder whether the next check against vm_start_gap() which
> > > > > > > checks against the aligned address is correct:
> > > > > > > 
> > > > > > >                 addr = PAGE_ALIGN(addr);
> > > > > > >                 vma = find_vma(mm, addr);
> > > > > > > 
> > > > > > >                 if (end - len >= addr &&
> > > > > > >                     (!vma || addr + len <= vm_start_gap(vma)))
> > > > > > >                         return addr;
> > > > > > 
> > > > > > I think the check is correct. The check is against resulting addresses
> > > > > > that end up in vm_start/vm_end. In our case we want to figure out what
> > > > > > user asked for.
> > > > > 
> > > > > Well, but then checking just against the user supplied addr is only half of
> > > > > the story.
> > > > > 
> > > > >     addr = boundary - PAGE_SIZE - PAGE_SIZE / 2;
> > > > >     len = PAGE_SIZE - PAGE_SIZE / 2;
> > > > > 
> > > > > That fits, but then after alignment we end up with
> > > > > 
> > > > >     addr = boudary - PAGE_SIZE;
> > > > > 
> > > > > and due to len > PAGE_SIZE this will result in a mapping which crosses the
> > > > > boundary, right? So checking against the PAGE_ALIGN(addr) should be the
> > > > > right thing to do.
> > > > 
> > > > IIUC, this is only the case if 'len' is not aligned, right?
> > > > 
> > > > >From what I see we expect caller to align it (and mm/mmap.c does this, I
> > > > haven't checked other callers).
> > > > 
> > > > And hugetlb would actively reject non-aligned len.
> > > > 
> > > > I *think* we should be fine with checking unaligned 'addr'.
> > > 
> > > I think we should keep it consistent for the normal and the huge case and
> > > just check aligned and be done with it.
> > 
> > Aligned 'addr'? Or 'len'? Both?
> > 
> > We would have problem with checking aligned addr. I steped it in hugetlb
> > case:
> > 
> >   - User asks for mmap((1UL << 47) - PAGE_SIZE, 2 << 20, MAP_HUGETLB);
> > 
> >   - On 4-level paging machine this gives us invalid hint address as
> >     'TASK_SIZE - len' is more than 'addr'. Goto get_unmapped_area.
> > 
> >   - On 5-level paging machine hint address gets rounded up to next 2MB
> >     boundary that is exactly 1UL << 47 and we happily allocate from full
> >     address space which may lead to trouble.
> 
> Below is updated patch with self-test.
> 
> Output on 5-level paging machine:
> 
> mmap(NULL): 0x7fbbad1f3000 - OK
> mmap(LOW_ADDR): 0x40000000 - OK
> mmap(HIGH_ADDR): 0x4000000000000 - OK
> mmap(HIGH_ADDR) again: 0xffffbbad1fb000 - OK
> mmap(HIGH_ADDR, MAP_FIXED): 0x4000000000000 - OK
> mmap(-1): 0xffffbbad1f9000 - OK
> mmap(-1) again: 0xffffbbad1f7000 - OK
> mmap((1UL << 47), 2 * PAGE_SIZE): 0x7fbbad1f3000 - OK
> mmap((1UL << 47), 2 * PAGE_SIZE / 2): 0x7fbbad1f1000 - OK
> mmap((1UL << 47) - PAGE_SIZE, 2 * PAGE_SIZE, MAP_FIXED): 0x7ffffffff000 - OK
> mmap(NULL, MAP_HUGETLB): 0x7fbbac400000 - OK
> mmap(LOW_ADDR, MAP_HUGETLB): 0x40000000 - OK
> mmap(HIGH_ADDR, MAP_HUGETLB): 0x4000000000000 - OK
> mmap(HIGH_ADDR, MAP_HUGETLB) again: 0xffffbbace00000 - OK
> mmap(HIGH_ADDR, MAP_FIXED | MAP_HUGETLB): 0x4000000000000 - OK
> mmap(-1, MAP_HUGETLB): (nil) - OK
> mmap(-1, MAP_HUGETLB) again: 0x7fbbac400000 - OK
> mmap((1UL << 47), 2UL << 20, MAP_HUGETLB): 0x800000000000 - FAILED
> mmap((1UL << 47) - (2UL << 20), 4UL << 20, MAP_FIXED | MAP_HUGETLB): 0x7fffffe00000 - OK
> 
> So, only hugetlb is problematic. mmap() aligns addr to PAGE_SIZE.
> See round_hint_to_min(). In this case we round address *down* and it works
> fine.
> 
> Replacing 'addr = ALIGN(addr, huge_page_size(h))' in hugetlbpage.c with
> 'addr &= huge_page_mask(h)' fixes the issue.

What about this fixup:

diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index 835b78720ca2..676774b9bb8d 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -198,7 +198,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 
 	/* requesting a specific address */
 	if (addr) {
-		addr = PAGE_ALIGN(addr);
+		addr &= PAGE_MASK;
 		if (!mmap_address_hint_valid(addr, len))
 			goto get_unmapped_area;
 
diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
index 92db903c3dad..00b296617ca4 100644
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c
@@ -166,7 +166,7 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
 	}
 
 	if (addr) {
-		addr = ALIGN(addr, huge_page_size(h));
+		addr &= huge_page_mask(h);
 		if (!mmap_address_hint_valid(addr, len))
 			goto get_unmapped_area;
 
-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Thomas Gleixner]

On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> On Wed, Nov 15, 2017 at 05:04:26PM +0300, Kirill A. Shutemov wrote:
>  	/* requesting a specific address */
>  	if (addr) {
> -		addr = PAGE_ALIGN(addr);
> +		addr &= PAGE_MASK;
>  		if (!mmap_address_hint_valid(addr, len))
>  			goto get_unmapped_area;
>  
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 92db903c3dad..00b296617ca4 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -166,7 +166,7 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>  	}
>  
>  	if (addr) {
> -		addr = ALIGN(addr, huge_page_size(h));
> +		addr &= huge_page_mask(h);
>  		if (!mmap_address_hint_valid(addr, len))
>  			goto get_unmapped_area;

That should work. Care to pickup my variant, make the fixups and resend
along with the selftest which covers both normal and huge mappings?

Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Kirill A. Shutemov]

On Wed, Nov 15, 2017 at 03:23:32PM +0100, Thomas Gleixner wrote:
> On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> > On Wed, Nov 15, 2017 at 05:04:26PM +0300, Kirill A. Shutemov wrote:
> >  	/* requesting a specific address */
> >  	if (addr) {
> > -		addr = PAGE_ALIGN(addr);
> > +		addr &= PAGE_MASK;
> >  		if (!mmap_address_hint_valid(addr, len))
> >  			goto get_unmapped_area;
> >  
> > diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> > index 92db903c3dad..00b296617ca4 100644
> > --- a/arch/x86/mm/hugetlbpage.c
> > +++ b/arch/x86/mm/hugetlbpage.c
> > @@ -166,7 +166,7 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> >  	}
> >  
> >  	if (addr) {
> > -		addr = ALIGN(addr, huge_page_size(h));
> > +		addr &= huge_page_mask(h);
> >  		if (!mmap_address_hint_valid(addr, len))
> >  			goto get_unmapped_area;
> 
> That should work. Care to pickup my variant, make the fixups and resend
> along with the selftest which covers both normal and huge mappings?

Sure.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border

[Thomas Gleixner]

On Wed, 15 Nov 2017, Kirill A. Shutemov wrote:
> On Wed, Nov 15, 2017 at 12:39:40PM +0100, Thomas Gleixner wrote:
> > > I *think* we should be fine with checking unaligned 'addr'.
> > 
> > I think we should keep it consistent for the normal and the huge case and
> > just check aligned and be done with it.
> 
> Aligned 'addr'? Or 'len'? Both?
> 
> We would have problem with checking aligned addr. I steped it in hugetlb
> case:
> 
>   - User asks for mmap((1UL << 47) - PAGE_SIZE, 2 << 20, MAP_HUGETLB);
> 
>   - On 4-level paging machine this gives us invalid hint address as
>     'TASK_SIZE - len' is more than 'addr'. Goto get_unmapped_area.
> 
>   - On 5-level paging machine hint address gets rounded up to next 2MB
>     boundary that is exactly 1UL << 47 and we happily allocate from full
>     address space which may lead to trouble.

Ah, right because that ALIGN is using huge_page_size(h) and not PAGE_SIZE.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>