[PATCH] s390/mm: return -ENOMEM in arch_get_unmapped_area[_topdown]

[PATCH] s390/mm: return -ENOMEM in arch_get_unmapped_area[_topdown]

[Li Wang]

That would be very hard to get -ENOMEM returned in crst_table_upgrade()
because the condition(addr + len <= TASK_SIZE) makes all 'end' value
is smaller/equal than 'TASK_SIZE_TASK'. So let's move it to the upper
layer.

Fixes: 8ab867cb0806 (s390/mm: fix BUG_ON in crst_table_upgrade)
Signed-off-by: Li Wang <liwang@redhat.com>
---
 arch/s390/mm/mmap.c    | 6 ++++++
 arch/s390/mm/pgalloc.c | 3 +--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/s390/mm/mmap.c b/arch/s390/mm/mmap.c
index 5bea139..8ddb13a 100644
--- a/arch/s390/mm/mmap.c
+++ b/arch/s390/mm/mmap.c
@@ -119,6 +119,9 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
 		return addr;
 
 check_asce_limit:
+	if (addr + len >= TASK_SIZE_MAX)
+		return -ENOMEM;
+
 	if (addr + len > current->mm->context.asce_limit &&
 	    addr + len <= TASK_SIZE) {
 		rc = crst_table_upgrade(mm, addr + len);
@@ -184,6 +187,9 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	}
 
 check_asce_limit:
+	if (addr + len >= TASK_SIZE_MAX)
+		return -ENOMEM;
+
 	if (addr + len > current->mm->context.asce_limit &&
 	    addr + len <= TASK_SIZE) {
 		rc = crst_table_upgrade(mm, addr + len);
diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
index 05f1f27..5e4b887 100644
--- a/arch/s390/mm/pgalloc.c
+++ b/arch/s390/mm/pgalloc.c
@@ -84,8 +84,7 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
 
 	/* upgrade should only happen from 3 to 4, 3 to 5, or 4 to 5 levels */
 	VM_BUG_ON(mm->context.asce_limit < _REGION2_SIZE);
-	if (end >= TASK_SIZE_MAX)
-		return -ENOMEM;
+
 	rc = 0;
 	notify = 0;
 	while (mm->context.asce_limit < end) {
-- 
2.9.3

Re: [PATCH] s390/mm: return -ENOMEM in arch_get_unmapped_area[_topdown]

[Martin Schwidefsky]

On Thu, 26 Oct 2017 15:36:10 +0800
Li Wang <liwang@redhat.com> wrote:

> That would be very hard to get -ENOMEM returned in crst_table_upgrade()
> because the condition(addr + len <= TASK_SIZE) makes all 'end' value
> is smaller/equal than 'TASK_SIZE_TASK'. So let's move it to the upper
> layer.

I have a hard time understanding what scenario you describe. There is no
'TASK_SIZE_TASK', only TASK_SIZE, TASK_SIZE_OF and TASK_SIZE_MAX. 

The code in mmap.c checks for the per-task limit, 31-bit vs 64-bit.
pgalloc.c checks for the maximum allowed address and does not care
about the task.

> Fixes: 8ab867cb0806 (s390/mm: fix BUG_ON in crst_table_upgrade)
> Signed-off-by: Li Wang <liwang@redhat.com>

I don't think this patch fixes anything.

> ---
>  arch/s390/mm/mmap.c    | 6 ++++++
>  arch/s390/mm/pgalloc.c | 3 +--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/mm/mmap.c b/arch/s390/mm/mmap.c
> index 5bea139..8ddb13a 100644
> --- a/arch/s390/mm/mmap.c
> +++ b/arch/s390/mm/mmap.c
> @@ -119,6 +119,9 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>  		return addr;
> 
>  check_asce_limit:
> +	if (addr + len >= TASK_SIZE_MAX)
> +		return -ENOMEM;
> +
>  	if (addr + len > current->mm->context.asce_limit &&
>  	    addr + len <= TASK_SIZE) {
>  		rc = crst_table_upgrade(mm, addr + len);
> @@ -184,6 +187,9 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
>  	}
> 
>  check_asce_limit:
> +	if (addr + len >= TASK_SIZE_MAX)
> +		return -ENOMEM;
> +
>  	if (addr + len > current->mm->context.asce_limit &&
>  	    addr + len <= TASK_SIZE) {
>  		rc = crst_table_upgrade(mm, addr + len);
> diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
> index 05f1f27..5e4b887 100644
> --- a/arch/s390/mm/pgalloc.c
> +++ b/arch/s390/mm/pgalloc.c
> @@ -84,8 +84,7 @@ int crst_table_upgrade(struct mm_struct *mm, unsigned long end)
> 
>  	/* upgrade should only happen from 3 to 4, 3 to 5, or 4 to 5 levels */
>  	VM_BUG_ON(mm->context.asce_limit < _REGION2_SIZE);
> -	if (end >= TASK_SIZE_MAX)
> -		return -ENOMEM;
> +
>  	rc = 0;
>  	notify = 0;
>  	while (mm->context.asce_limit < end) {


-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH] s390/mm: return -ENOMEM in arch_get_unmapped_area[_topdown]

[Li Wang]

On Thu, Oct 26, 2017 at 5:26 PM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
> On Thu, 26 Oct 2017 15:36:10 +0800
> Li Wang <liwang@redhat.com> wrote:
>
>> That would be very hard to get -ENOMEM returned in crst_table_upgrade()
>> because the condition(addr + len <= TASK_SIZE) makes all 'end' value
>> is smaller/equal than 'TASK_SIZE_TASK'. So let's move it to the upper
>> layer.
>
> I have a hard time understanding what scenario you describe. There is no
> 'TASK_SIZE_TASK', only TASK_SIZE, TASK_SIZE_OF and TASK_SIZE_MAX.

Sorry for the typo, I was thinking about to write TASK_SIZE_MAX.

>
> The code in mmap.c checks for the per-task limit, 31-bit vs 64-bit.
> pgalloc.c checks for the maximum allowed address and does not care
> about the task.
>
>> Fixes: 8ab867cb0806 (s390/mm: fix BUG_ON in crst_table_upgrade)
>> Signed-off-by: Li Wang <liwang@redhat.com>
>
> I don't think this patch fixes anything.

At least there is a logic error i think, after apply the patch
"s390/mm: fix BUG_ON in crst_table_upgrade",
it makes no sense to compare "if (end >= TASK_SIZE_MAX) return
-ENOMEM" in crst_table_upgrade() function.

isn't it?


Thanks for reviewing quick.


-- 
Li Wang
liwang@redhat.com

Re: [PATCH] s390/mm: return -ENOMEM in arch_get_unmapped_area[_topdown]

[Martin Schwidefsky]

On Thu, 26 Oct 2017 17:47:39 +0800
Li Wang <liwang@redhat.com> wrote:

> On Thu, Oct 26, 2017 at 5:26 PM, Martin Schwidefsky
> <schwidefsky@de.ibm.com> wrote:
> > On Thu, 26 Oct 2017 15:36:10 +0800
> > Li Wang <liwang@redhat.com> wrote:
> >  
> > The code in mmap.c checks for the per-task limit, 31-bit vs 64-bit.
> > pgalloc.c checks for the maximum allowed address and does not care
> > about the task.
> >  
> >> Fixes: 8ab867cb0806 (s390/mm: fix BUG_ON in crst_table_upgrade)
> >> Signed-off-by: Li Wang <liwang@redhat.com>  
> >
> > I don't think this patch fixes anything.  
> 
> At least there is a logic error i think, after apply the patch
> "s390/mm: fix BUG_ON in crst_table_upgrade",
> it makes no sense to compare "if (end >= TASK_SIZE_MAX) return
> -ENOMEM" in crst_table_upgrade() function.
> 
> isn't it?

Be careful with TASK_SIZE vs. TASK_SIZE_MAX. They return different
values for 31-bit compat tasks.

If the addr parameter is correctly aligned then the if condition in
crst_table_upgrade is superfluous as TASK_SIZE_MAX is now -PAGE_SIZE
with the introduction of 5 level page tables. It is important for older
kernels with only 4 level page tables with a TASK_SIZE_MAX of 2**53.

On the other hand if addr is ever a value between -PAGE_SIZE and -1
we would end up with an endless loop. That makes the if condition a
safe-guard and I would like to keep it.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH] s390/mm: return -ENOMEM in arch_get_unmapped_area[_topdown]

[Li Wang]

On Thu, Oct 26, 2017 at 6:16 PM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
> On Thu, 26 Oct 2017 17:47:39 +0800
> Li Wang <liwang@redhat.com> wrote:
>
>> On Thu, Oct 26, 2017 at 5:26 PM, Martin Schwidefsky
>> <schwidefsky@de.ibm.com> wrote:
>> > On Thu, 26 Oct 2017 15:36:10 +0800
>> > Li Wang <liwang@redhat.com> wrote:
>> >
>> > The code in mmap.c checks for the per-task limit, 31-bit vs 64-bit.
>> > pgalloc.c checks for the maximum allowed address and does not care
>> > about the task.
>> >
>> >> Fixes: 8ab867cb0806 (s390/mm: fix BUG_ON in crst_table_upgrade)
>> >> Signed-off-by: Li Wang <liwang@redhat.com>
>> >
>> > I don't think this patch fixes anything.
>>
>> At least there is a logic error i think, after apply the patch
>> "s390/mm: fix BUG_ON in crst_table_upgrade",
>> it makes no sense to compare "if (end >= TASK_SIZE_MAX) return
>> -ENOMEM" in crst_table_upgrade() function.
>>
>> isn't it?
>
> Be careful with TASK_SIZE vs. TASK_SIZE_MAX. They return different
> values for 31-bit compat tasks.

what do you think this reproducer now failed(mmap into high region
succeeded) on the latest kernel?
should we enlarge the HIGH_ADDR to -PAGE_SIZE?

#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <stdlib.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/types.h>

#define HIGH_ADDR (void *)(1L << 53)

int main(void)
{

    void *addr;
    long map_sz = getpagesize();
    int fd = open("testfile", O_RDWR | O_CREAT, 0666);

    /* Attempt to mmap into highmem addr, should get ENOMEM */
    addr = mmap(HIGH_ADDR, map_sz, PROT_READ,
            MAP_SHARED | MAP_FIXED, fd, 0);
    if (addr != MAP_FAILED) {
        printf("FAIL: mmap into high region succeeded unexpectedly\n");
        munmap(addr, map_sz);
        close(fd);
    }

    if (errno != ENOMEM) {
        printf("FAIL: mmap into high region failed unexpectedly -
expect errno=ENOMEM, got\n");
    } else {
        printf("PASS: mmap into high region failed as expected\n");
    }

    return 0;
}



>
> If the addr parameter is correctly aligned then the if condition in
> crst_table_upgrade is superfluous as TASK_SIZE_MAX is now -PAGE_SIZE
> with the introduction of 5 level page tables. It is important for older
> kernels with only 4 level page tables with a TASK_SIZE_MAX of 2**53.
>
> On the other hand if addr is ever a value between -PAGE_SIZE and -1
> we would end up with an endless loop. That makes the if condition a
> safe-guard and I would like to keep it.
>
> --
> blue skies,
>    Martin.
>
> "Reality continues to ruin my life." - Calvin.
>



-- 
Li Wang
liwang@redhat.com