Subject: [patch] 2.4.4: mmap() fails for certain legal requests

From: "Maciej W. Rozycki" <[email-masked]>
Date: Tue, 8 May 2001 22:21:55 +0200 (MET DST)

Hi,

 The mmap() call fails when addr is specified, MAP_FIXED is cleared in
flags and no address space can be allocated either at addr or above it. 
This is a legal request and it should not fail as long as there is space
available below addr.  Following is a patch that fixes the problem. 

 This is nothing new -- I already submitted a similar patch against
2.4.0-test4 once upon a time.  This patch is clean(er), though, and I
believe it can be safely applied to the upcoming 2.4.5 release. 

 A simple test case to trigger the current mmap() bad behaviour for 32-bit
CPUs is something like: 

fd = open("/dev/zero", O_RDONLY);
p = mmap((void *)0xfffff000, 4096, PROT_READ, MAP_SHARED, fd, 0);

With my patch the code does not fail anymore -- p is set to an available
address lower than 0xfffff000. 

 The bug was discovered when tracking down the reason of dlopen() failures
when called from statically linked binaries on MIPS/Linux.  The patch
fixes them.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: [email-masked], PGP key available        +

diff -up --recursive --new-file linux-2.4.4.macro/mm/mmap.c linux-2.4.4/mm/mmap.c
--- linux-2.4.4.macro/mm/mmap.c	Tue May  1 17:24:25 2001
+++ linux-2.4.4/mm/mmap.c	Tue May  1 18:23:25 2001
@@ -219,7 +219,7 @@ unsigned long do_mmap_pgoff(struct file 
 	if ((len = PAGE_ALIGN(len)) == 0)
 		return addr;
 
-	if (len > TASK_SIZE || addr > TASK_SIZE-len)
+	if (len > TASK_SIZE)
 		return -EINVAL;
 
 	/* offset overflow? */
@@ -405,9 +405,15 @@ static inline unsigned long arch_get_unm
 
 	if (len > TASK_SIZE)
 		return -ENOMEM;
-	if (!addr)
-		addr = TASK_UNMAPPED_BASE;
-	addr = PAGE_ALIGN(addr);
+
+	if (addr) {
+		addr = PAGE_ALIGN(addr);
+		vma = find_vma(current->mm, addr);
+		if (TASK_SIZE - len >= addr &&
+		    (!vma || addr + len <= vma->vm_start))
+			return addr;
+	}
+	addr = PAGE_ALIGN(TASK_UNMAPPED_BASE);
 
 	for (vma = find_vma(current->mm, addr); ; vma = vma->vm_next) {
 		/* At this point:  (!vma || addr < vma->vm_end). */
@@ -425,6 +431,8 @@ extern unsigned long arch_get_unmapped_a
 unsigned long get_unmapped_area(struct file *file, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags)
 {
 	if (flags & MAP_FIXED) {
+		if (addr > TASK_SIZE - len)
+			return -EINVAL;
 		if (addr & ~PAGE_MASK)
 			return -EINVAL;
 		return addr;

---

From: "David S. Miller" <[email-masked]>
Date: Tue, 8 May 2001 13:47:57 -0700 (PDT)

> when called from statically linked binaries on MIPS/Linux.  The patch

There are several get_unmapped_area() implementations besides the
standard one (search for HAVE_ARCH_UNMAPPED_AREA).  Please fix
them up too.

Later,
David S. Miller
[email-masked]

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Tue, 8 May 2001 23:32:21 +0200 (MET DST)

> standard one (search for HAVE_ARCH_UNMAPPED_AREA).  Please fix

 Yep, I know (ia64 and sparc*).  But being lazy enough (and being short on
time) I won't do it until I know the idea of the change is accepted.  I'm
sorry -- I sent previous versions of the patch twice since last Summer
with no response at all and doing bits no one is interested in is a waste
of time.

 Thanks for your response, though -- maybe there is someone interested,
after all.

---

From: "David S. Miller" <[email-masked]>
Date: Tue, 8 May 2001 14:54:48 -0700 (PDT)

Czesc,

 >  Thanks for your response, though -- maybe there is someone interested,

That's pretty arrogant that cut and pasting a few lines into some
architecture specific files and reporting the updated patch is too
much to ask.

Perhaps reviewing your change is also, too much to ask.  Perhaps
we are too lazy and short on time to have a look at your change.

I don't think it's asking a lot to provide a complete change.

I'm sure the MIPS folks know all too well whats it's like when their
port is crapped up because someone only made changes to x86 port
portions.  At least for me on after working on Sparc for some time,
I'm adamant about providing complete changes so that this kind of
grief is avoided for other port maintainers.

In the time you used to compose your response to me, and now
to read this email from me, you could have fixed up the patch
perhaps 2 or 3 times.  Just do it and get it over with ok?

Dziekuje.

Later,
David S. Miller
[email-masked]

---

From: Alan Cox <[email-masked]>
Date: Tue, 8 May 2001 23:59:33 +0100 (BST)

> architecture specific files and reporting the updated patch is too

And just how is he going to test it ? Considering he was just asking if the
concept was reasonable I think you are a little out of order

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Wed, 9 May 2001 01:46:35 +0200 (MET DST)

> architecture specific files and reporting the updated patch is too

 I'm sorry if you find me arrogant -- that certainly was not my intent.  I
did look at the files and changes are not as trivial as cut and paste.

> we are too lazy and short on time to have a look at your change.

 Well, I've been using similar changes since July.  I may live with
patches forever and be fine.  Still this is not the point with free
software.  It would be malicious if I had a fix and I wouldn't share it. 
Sooner or later someone would discover the problem again and would waste
time to track it down unnecessarily.  And again, and again...

> I don't think it's asking a lot to provide a complete change.

 It's not a lot, supposedly, but look at the case from my point of view. 
It's a bugfix and not a new feature.  I've invested a few hours in finding
the cause of a weird bug on a MIPS/Linux machine.  I am providing a ready
solution that works for most architectures with the exception of a few
ones I'm not familiar with. 

 Well, it's great I have an opportunity to get better knowledge on these
architectures, but I cannot always afford it and I know there are people
who already have enough knowledge to be sure bits get written correctly
immediately.  I never hesitate to do job myself in the areas I am familiar
with or when I have enough free time (and I do have, from time to time). 
I don't have time currently, I am afraid (basically I am now stealing the
time I would otherwise spend sleeping for a task that was quite low on my
priority list) and I am sure someone familiar with the specific ports
would spend less time than I do.  Finally I do consider my time equally
worth to anyone else's one, so why should I have to spend x units of time,
whilst some else would only spend x/2 or x/3 or whatever...  Of course I
consider this rule working both ways. 

> grief is avoided for other port maintainers.

 The port gets crapped from time to time, although Ralf is doing great job
to keep it fine, so it's more that specific MIPS hosts lag behind the rest
of the kernel.  Still I consider it the specific maintainer's job to get
things synchronized.  It just works better this way. 

> perhaps 2 or 3 times.  Just do it and get it over with ok?

 I'm not so sure, I'm afraid, especially at this time of the day.  Check
timestamps of mails if curious... 

> Dziekuje.

 Nie za ma co. ;-)

 A patch follows.  Architecture-specific changes are completely untested. 
I hope I got things right, otherwise I'll consider my time wasted.

 BTW, I've noticed the "if (flags & MAP_FIXED)" statements in
arch_get_unmapped_area() in arch/sparc*/kernel/sys_sparc.c are dead code
now, as get_unmapped_area() in mm/mmap.c never calls it if MAP_FIXED is
set in flags.

  Maciej

diff -up --recursive --new-file linux-2.4.4.macro/arch/ia64/kernel/sys_ia64.c linux-2.4.4/arch/ia64/kernel/sys_ia64.c
--- linux-2.4.4.macro/arch/ia64/kernel/sys_ia64.c	Mon May  7 16:43:50 2001
+++ linux-2.4.4/arch/ia64/kernel/sys_ia64.c	Tue May  8 23:25:49 2001
@@ -28,13 +28,22 @@ arch_get_unmapped_area (struct file *fil
 
 	if (len > RGN_MAP_LIMIT)
 		return -ENOMEM;
-	if (!addr)
 
+	if (addr) {
+		if (flags & MAP_SHARED)
+			addr = COLOR_ALIGN(addr);
+		else
+			addr = PAGE_ALIGN(addr);
+		vmm = find_vma(current->mm, addr);
+		if (TASK_SIZE - len >= addr &&
+		    rgn_offset(addr) + len <= RGN_MAP_LIMIT) &&
+		    (!vmm || addr + len <= vmm->vm_start))
+			return addr;
+	}
 	if (flags & MAP_SHARED)
-		addr = COLOR_ALIGN(addr);
+		addr = COLOR_ALIGN(TASK_UNMAPPED_BASE);
 	else
-		addr = PAGE_ALIGN(addr);
+		addr = PAGE_ALIGN(TASK_UNMAPPED_BASE);
 
 	for (vmm = find_vma(current->mm, addr); ; vmm = vmm->vm_next) {
 		/* At this point:  (!vmm || addr < vmm->vm_end). */
diff -up --recursive --new-file linux-2.4.4.macro/arch/sparc/kernel/sys_sparc.c linux-2.4.4/arch/sparc/kernel/sys_sparc.c
--- linux-2.4.4.macro/arch/sparc/kernel/sys_sparc.c	Mon May  7 16:22:42 2001
+++ linux-2.4.4/arch/sparc/kernel/sys_sparc.c	Tue May  8 22:50:51 2001
@@ -54,17 +54,31 @@ unsigned long arch_get_unmapped_area(str
 		return -ENOMEM;
 	if (ARCH_SUN4C_SUN4 && len > 0x20000000)
 		return -ENOMEM;
-	if (!addr)
-		addr = TASK_UNMAPPED_BASE;
 
+	if (addr) {
+		if (flags & MAP_SHARED)
+			addr = COLOR_ALIGN(addr);
+		else
+			addr = PAGE_ALIGN(addr);
+		vmm = find_vma(current->mm, addr);
+		if (ARCH_SUN4C_SUN4 && addr < 0xe0000000 &&
+		    0x20000000 - len < addr) {
+			addr = PAGE_OFFSET;
+			vmm = find_vma(current->mm, PAGE_OFFSET);
+		}
+		if (TASK_SIZE - PAGE_SIZE - len >= addr &&
+		    (!vmm || addr + len <= vmm->vm_start))
+			return addr;
+	}
 	if (flags & MAP_SHARED)
-		addr = COLOUR_ALIGN(addr);
+		addr = COLOR_ALIGN(TASK_UNMAPPED_BASE);
 	else
-		addr = PAGE_ALIGN(addr);
+		addr = PAGE_ALIGN(TASK_UNMAPPED_BASE);
 
 	for (vmm = find_vma(current->mm, addr); ; vmm = vmm->vm_next) {
 		/* At this point:  (!vmm || addr < vmm->vm_end). */
-		if (ARCH_SUN4C_SUN4 && addr < 0xe0000000 && 0x20000000 - len < addr) {
+		if (ARCH_SUN4C_SUN4 && addr < 0xe0000000 &&
+		    0x20000000 - len < addr) {
 			addr = PAGE_OFFSET;
 			vmm = find_vma(current->mm, PAGE_OFFSET);
 		}
diff -up --recursive --new-file linux-2.4.4.macro/arch/sparc64/kernel/sys_sparc.c linux-2.4.4/arch/sparc64/kernel/sys_sparc.c
--- linux-2.4.4.macro/arch/sparc64/kernel/sys_sparc.c	Mon May  7 16:44:01 2001
+++ linux-2.4.4/arch/sparc64/kernel/sys_sparc.c	Tue May  8 22:30:09 2001
@@ -60,15 +60,27 @@ unsigned long arch_get_unmapped_area(str
 		task_size = 0xf0000000UL;
 	if (len > task_size || len > -PAGE_OFFSET)
 		return -ENOMEM;
-	if (!addr)
-		addr = TASK_UNMAPPED_BASE;
 
+	task_size -= len;
+
+	if (addr) {
+		if (flags & MAP_SHARED)
+			addr = COLOR_ALIGN(addr);
+		else
+			addr = PAGE_ALIGN(addr);
+		vmm = find_vma(current->mm, addr);
+		if (addr < PAGE_OFFSET && -PAGE_OFFSET - len < addr) {
+			addr = PAGE_OFFSET;
+			vmm = find_vma(current->mm, PAGE_OFFSET);
+		}
+		if (task_size >= addr &&
+		    (!vmm || addr + len <= vmm->vm_start))
+			return addr;
+	}
 	if (flags & MAP_SHARED)
-		addr = COLOUR_ALIGN(addr);
+		addr = COLOR_ALIGN(TASK_UNMAPPED_BASE);
 	else
-		addr = PAGE_ALIGN(addr);
-
-	task_size -= len;
+		addr = PAGE_ALIGN(TASK_UNMAPPED_BASE);
 
 	for (vmm = find_vma(current->mm, addr); ; vmm = vmm->vm_next) {
 		/* At this point:  (!vmm || addr < vmm->vm_end). */
diff -up --recursive --new-file linux-2.4.4.macro/mm/mmap.c linux-2.4.4/mm/mmap.c
--- linux-2.4.4.macro/mm/mmap.c	Mon May  7 16:44:32 2001
+++ linux-2.4.4/mm/mmap.c	Tue May  8 22:16:00 2001
@@ -219,7 +219,7 @@ unsigned long do_mmap_pgoff(struct file 
 	if ((len = PAGE_ALIGN(len)) == 0)
 		return addr;

 	/* offset overflow? */
 
 	if (len > TASK_SIZE)
 		return -ENOMEM;
-	if (!addr)

---

From: "David S. Miller" <[email-masked]>
Date: Tue, 8 May 2001 16:59:03 -0700 (PDT)

Alan Cox writes:
 > And just how is he going to test it ? Considering he was just
 > asking if the concept was reasonable I think you are a little out
 > of order

I can't test every platform when I have to make such changes.
But it always serves to show the port maintainer "what" the
change was.

Yes, I am slightly out of order if the intent is just "does
this idea look fine" (which it does btw, I can't  find any
problems with it).

I apologize to Maciej, but I do deplore him to actually do the
final bits for the other ports when he makes his final patch
submission.

Later,
David S. Miller
[email-masked]

---

From: Michael Shmulevich <[email-masked]>
Date: Wed, 09 May 2001 15:03:48 +0300

As a side question: does this patch apply to 2.2.x kernel too?

While working on ld.so.1-9 port on MIPS I seen there's a try to mmap() 
some address > 0x80000000 which fails due to the same 
if(...TASK_SIZE...) mentioned in the patch.

Just wondering if this applies to me too :-)

Sincerely yours,
Michael Shmulevich
______________________________________
Software Developer
Jungo - R&D
email: [email-masked]
web: http://www.jungo.com
Phone: 1-877-514-0537(USA)  +972-9-8859365(Worldwide) ext. 233
Fax:   1-877-514-0538(USA)  +972-9-8859366(Worldwide)

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Wed, 9 May 2001 14:41:10 +0200 (MET DST)

> As a side question: does this patch apply to 2.2.x kernel too?

 Feel free to backport it. ;-)  One hunk fails for 2.2.19, but it can be
applied by hand (note the vma variable is named vmm in 2.2). 

> if(...TASK_SIZE...) mentioned in the patch.

 Yep, that's the exact problem, assuming MAP_FIXED is not set in flags
(something is screwed if it is).