Small do_mmap_pgoff correction

From: Linux Kernel Mailing List <[email protected]>
Date: Tue, 10 Dec 2002 15:51:07 +0000

+
+/*
+ * 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.
+ */
+

This assumption is wrong. It is totally possible for TASK_SIZE to be the entire 64-bit address space on sparc64 and thus larger than SIZE_MAX - PAGE_SIZE, and I definitely plan on supporting that.

User processes live entirely in their very own address space seperate from the kernel, so kernel stuff does not take up any part of the user virtual addresses.

Please revert this change, it adds absolutely nothing.

Hi David :)

OK, then another way of fixing the corner case that exists in
do_mmap_pgoff is needed. You cannot mmap a chunk of memory whose size is bigger than SIZE_MAX-PAGE_SIZE, because 'PAGE_ALIGN' will return 0 when page-aligning the size.

Anyway you cannot use a size larger than SIZE_MAX-PAGE_SIZE even
on sparc64, since mmap will fail when page aligning such a size, returning 0 :((( Reverting the change is worse (IMHO).

It corrects the corner case. See below. If you have a better
solution for the corner case problem that doesn't involve limiting the max size you can request for mmaping so it doesn't get the last page, it is welcome, of course :))

The code says:

if ((len = PAGE_ALIGN(len)) == 0)

and this returns 0 if the requested size ('len', here) is between
SIZE_MAX-PAGE_SIZE and SIZE_MAX. And this is wrong. Don't know if under sparc64 the PAGE_ALIGN macro returns correct values, but I don't think so, since the correct value for an address in the last page is 0 when page aligned. The problem is that we are aligning a SIZE, not an address :((

Maybe a new macro needed here...

If you want the entire explanation, just tell :) I wrote in the
past for the same patch. Anyway, nor Linus nor Alan did see anything wrong with this :??

Raúl

From: DervishD <[email protected]>
Date: Tue, 10 Dec 2002 21:45:30 +0100

Hi David :)

OK, then another way of fixing the corner case that exists in
do_mmap_pgoff is needed. You cannot mmap a chunk of memory whose size is bigger than SIZE_MAX-PAGE_SIZE, because 'PAGE_ALIGN' will return 0 when page-aligning the size.

And after your patch, we'd use a zero length. That is a bug.

Anyway you cannot use a size larger than SIZE_MAX-PAGE_SIZE even
on sparc64, since mmap will fail when page aligning such a size, returning 0 :((( Reverting the change is worse (IMHO).

This is wrong.

I said that the address space can be this huge size. I didn't say that this means such a huge single mmap() could work.

It makes that your assumption that allows for the code change you made is invalid.

if ((len = PAGE_ALIGN(len)) == 0)

and this returns 0 if the requested size ('len', here) is between
SIZE_MAX-PAGE_SIZE and SIZE_MAX. And this is wrong.

And your change causes us to use a len of "zero" in this case, how is that more valid?

Look at what happens, you PAGE_ALIGN(len) after all the range checks then we use a len of '0' for the rest of the function. How is that supposed to be better?

Hi David :)

With my patch, we don't use a zero length :?? My patch sees if
PAGE_ALIGN will f*ck the length, and if so, it returns EINVAL. This is better than getting '0' as a valid address when specifying a large size, don't you think so?

Because PAGE_ALIGN won't return 0? I don't see your assumption of
'len' going to zero due to my patch :?? With my patch, if the requested size is '0', then the hint address is returned, and if the size is so high that PAGE_ALIGN will barf at it, returning 0 when it shouldn't, mmap will return EINVAL. The function never uses a 'len' of 0. Never.

Raúl

From: DervishD <[email protected]>
Date: Tue, 10 Dec 2002 21:59:06 +0100

Because PAGE_ALIGN won't return 0?

What if TASK_SIZE is ~0? Both your checks will pass for the case of (SIZE_MAX-PAGE_SIZE + 1) to ~0 cases.

Reverted.

Hi David :)

Well, the checks were already there... I didn't add them, I just
move a comparison to a better place. If TASK_SIZE is ~0, then the patch works. If you look at the patch, you will notice that I've only changed the order of the checks.

But all of this is pointless. The patch has been reverted and the
2.4.x branch will keep on silently failing when the requested size for an mmap() call is too large. That's good?

The patch just took a wrong comparison, did not introduce any
TASK_SIZE comparison and made mmap() work in a corner case.

FYI, without the patch mmap will silently fail when size is
between SIZE_MAX-PAGE_SIZE and SIZE_MAX. With the patch, it will return -EINVAL or, in the worst case, it will still silently fail. This happens only when TASK_SIZE is larger than SIZE_MAX-PAGE_SIZE, ok, but then propose another solution. The true problem is PAGE_ALIGN. We shouldn't use it for aligning sizes...

- if ((len = PAGE_ALIGN(len)) == 0)
+ if (!len)
return addr;

if (len > TASK_SIZE)
return -EINVAL;

+ len = PAGE_ALIGN(len); /* This cannot be zero now */

Anyway, without the patch, mmap fails on all architectures. With
it, it only fails on archs where TASK_SIZE is the entire address space. On those archs, nothing change so, why punish the other archs?

Sincerely, I don't understand why this patch is bad. Is no worse
than the previous situation :??

Raúl

From: DervishD <[email protected]>
Date: Tue, 10 Dec 2002 23:13:57 +0100

Sincerely, I don't understand why this patch is bad. Is no worse
than the previous situation :??

How about something like:

	if (len == 0)
		return addr;

	len = PAGE_ALIGN(len);
	if (len > TASK_SIZE || len == 0)
		return -EINVAL;

That should cover all cases and not make the TASK_SIZE assumption.

Hi David :)

Perfect :) If you want, I can make the patch and tell to Alan and
Linus. Anyway, I think you will better heared than me O:))

Anyway, I'll take a look at a new macro (lets say PAGE_ALIGN_SIZE
or something as ugly as this ;)))

Thanks for your correction.

Raúl

From: DervishD <[email protected]>
Date: Tue, 10 Dec 2002 23:28:42 +0100

Perfect :) If you want, I can make the patch and tell to Alan and
Linus. Anyway, I think you will better heared than me O:))

If you could take care of this, I would really be happy.

Anyway, I'll take a look at a new macro (lets say PAGE_ALIGN_SIZE
or something as ugly as this ;)))

How many places do we try to apply PAGE_ALIGN to a length? If it's just one or two spots, perhaps the special macro isn't worthwhile.

Hi David :)

OK, then, I'll prepare the patch.

I've seen four or five, without a detailed examination. Anyway,
since it would be a dangerous change (being in the MM code), I'll count ocurrences and problems. There is no intrinsic problem in using PAGE_ALIGN on a size if we know that size is small enough.

Thanks for all, Dave :)
Raúl