Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[David S. Miller]

   From: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
   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.

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[DervishD]

    Hi David :)

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

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

> Please revert this change, it adds absolutely nothing.

    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

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[David S. Miller]

   From: DervishD <raul@pleyades.net>
   Date: Tue, 10 Dec 2002 21:45:30 +0100

       Hi David :)
   
   >    + *	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.
   
       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?

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[DervishD]

    Hi David :)

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

    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?

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

    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

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[David S. Miller]

   From: DervishD <raul@pleyades.net>
   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.

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[Marcelo Tosatti]

On Tue, 10 Dec 2002, David S. Miller wrote:

>    From: DervishD <raul@pleyades.net>
>    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.

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[DervishD]

    Hi David :)

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

    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

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[David S. Miller]

   From: DervishD <raul@pleyades.net>
   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.

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[DervishD]

    Hi David :)

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

    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

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[David S. Miller]

   From: DervishD <raul@pleyades.net>
   Date: Tue, 10 Dec 2002 23:28:42 +0100
   
   > 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.
   
       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.

Re: [BK-2.4] [PATCH] Small do_mmap_pgoff correction

[DervishD]

    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:))
> If you could take care of this, I would really be happy.

    OK, then, I'll prepare the patch.

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

    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

Re: [BK-2.4] [Patch] Small do_mmap_pgoff correction

[DervishD]

    Hi David :)

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

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

> Please revert this change, it adds absolutely nothing.

    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