mmap() doesn't like certain value...

Hello all :))

While writing a test program I used mmap using SIZE_MAX (which is
the maximum value storeable in a size_t variable) as the length, just to see when mmap starts failing with EINVAL or ENOMEM.

Well, mmap() acted quite good and reasonable and gave me the
values I was searching for... except when a value of SIZE_MAX-4095 or higher is passed to it.

I've taken a look at the kernel sources and in the file
mm/mmap.c, in the function do_mmap_pgoff, the length supplied is page aligned (thru the macro PAGE_ALIGN). But this macro correctly says that when we are at address SIZE_MAX-4095 or higher the next page in the addressable space is the page at address 0. But we are dealing with *sizes*, not addresses.

The matter is that mmap() doesn't fail with values of
SIZE_MAX-4095 and higher (as it should do), but succeeds returning '0' as the address... This is due the calculus that PAGE_ALIGN does with the enormous length passed (namely 4294963200 or higher, up to the limit marked by SIZE_MAX: 2^32-1). This macro cannot be used with numbers near to the limit. mmap() should return -1 and set errno to EINVAL, as properly does when the enormous length is less than 2^32-4096.

I know: this lengths are enormous, nobody uses them, etc... but I
think that mmap shouldn't behave as bad just because nobody will use the entire domain of the function. If the length domain is [0, 2^32) the function should behave correctly, returning errors as appropriate, not succeeding falsely. So please consider correcting the problem (should suffice with eliminating the use of PAGE_ALIGN or adding special cases to the test prior to its use).

If this is not a bug, but an intended behaviour please excuse me.
Moreover, I can provide a patch (I suppose) against the 2.4.18 tree.

Thanks a lot for reading this :)
Raúl

unsigned long do_mmap_pgoff(struct file * file, unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flags, unsigned long pgoff)

{
        [local var defs snipped]
        if (file && (!file->f_op || !file->f_op->mmap))
                return -ENODEV;
        if ((len = PAGE_ALIGN(len)) == 0)
                return addr;
        if (len > TASK_SIZE)
                return -EINVAL;
        /* offset overflow? */
        if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
                return -EINVAL;

...

So,

- if ((len = PAGE_ALIGN(len)) == 0)
- return addr;
- if (len > TASK_SIZE)
+ if (!len)
+ return addr;
+ len = PAGE_ALIGN(len);
+ if (!len || len > TASK_SIZE) /* !len: address wrapped to 0 in ALIGN */

Looks like you found an obscure corner case. Good!

Do it.

BTW, does anybody know why len==0 is not flagged as error? --
vda

Hi Denis :)

Thanks for answering :)

This is the problem.

And is corrected just by inverting the two quoted code snips :)

If we test for len being greater than TASK_SIZE *before* aligning
it then the function works properly (the alignment will be done properly in the task address space). No patch needed really ;)

I'll give a try to the inversion, that should work. I have
written a small stress program for mmap, so in a few hours the patch will be ready. Must I post it here or send it directly to Marcello?

Ok :) I'll prepare the patch right now. Unified diff?

Well, I suppose that if you request 0 bytes, any address returned
is valid, since it cannot be accessed (SIGSEGV...). Anyway the man page for mmap() clearly says that the address returned is NEVER 0, so we must test for len==0 and invert the other tests.

Thanks a lot for answering, I was supposing that anybody on the
list was ignoring this obvious and easy to correct bug :?

Raúl

You are right

Post here and to Marcelo. BTW, is 2.5 affected?
--
vda

Hi Denis and Marcelo :)

I've added a couple of comments and one more test. I don't think
that mmap() should return 'addr' when len=0. Moreover, mmap() cannot return '0' under *any* circumstance (so says the man page).

I've tested the patch with a little program to test every
possible 'len' value and checking for mmap() returning the correct value.

This is the patch:

--- begin ---

@@ -389,6 +389,11 @@
 	return 0;
 }
 
+
+/*
+	NOTE: in this function we rely on TASK_SIZE being lower than
+SIZE_MAX-PAGE_SIZE at least. I'm pretty sure that it is.
+*/
 unsigned long do_mmap_pgoff(struct file * file, unsigned long addr, unsigned long len,
 	unsigned long prot, unsigned long flags, unsigned long pgoff)
 {

@@ -402,12 +407,11 @@
 	if (file && (!file->f_op || !file->f_op->mmap))
 		return -ENODEV;
 
-	if ((len = PAGE_ALIGN(len)) == 0)
-		return addr;
-
-	if (len > TASK_SIZE)
+	if (!len || len > TASK_SIZE)
 		return -EINVAL;
 
+	len = PAGE_ALIGN(len);  /* This CANNOT be zero */
+
 	/* offset overflow? */
 	if ((pgoff + (len >> PAGE_SHIFT)) < pgoff)
 		return -EINVAL;

--- end ---

I don't really know, but I'm pretty sure... Anyway I think that
the same patch is valid for both kernels with little or no modification.

BTW, the patch is not only mine, David Gómez Espinosa
([email protected]) has helped me with this issue too.

Raúl