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

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

[Maciej W. Rozycki]

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: macro@ds2.pg.gda.pl, 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;

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

[David S. Miller]

Maciej W. Rozycki writes:
 >  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.

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
davem@redhat.com

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

[Maciej W. Rozycki]

On Tue, 8 May 2001, David S. Miller wrote:

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

 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. 

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

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

[David S. Miller]

Czesc,

Maciej W. Rozycki writes:
 >  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. 

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
davem@redhat.com

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

[Alan Cox]

>  >  Thanks for your response, though -- maybe there is someone interested,
>  > after all. 
> 
> 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.

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

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

[Alan Cox]

>  >  Thanks for your response, though -- maybe there is someone interested,
>  > after all. 
> 
> 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.

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

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

[David S. Miller]

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
davem@redhat.com

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

[David S. Miller]

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
davem@redhat.com

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

[Maciej W. Rozycki]

On Tue, 8 May 2001, David S. Miller wrote:

> 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.

 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.

> 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.

 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. 

> 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.

 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. 

> 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?

 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

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

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)
-		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 (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;
 
-	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;

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

[Michael Shmulevich]

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: michaels@jungo.com
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)

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

[Maciej W. Rozycki]

On Wed, 9 May 2001, Michael Shmulevich wrote:

> 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). 

> 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.

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

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +