Message-id: <54F0AB6E.80302@samsung.com>
Date: Fri, 27 Feb 2015 20:37:50 +0300
From: Andrey Ryabinin <a.ryabinin@samsung.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101
 Thunderbird/31.4.0
MIME-version: 1.0
To: John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>, dvyukov@google.com, kcc@google.com,
        dmitryc@google.com, adech.fo@gmail.com, tetra2005@gmail.com,
        koct9i@gmail.com, Sasha Levin <sasha.levin@oracle.com>,
        Christoph Lameter <cl@linux.com>, iamjoonsoo.kim@lge.com,
        Dave Hansen <dave.hansen@intel.com>, Andi Kleen <andi@firstfloor.org>,
        Ingo Molnar <mingo@elte.hu>, Thomas Gleixner <tglx@linutronix.de>,
        "H. Peter Anvin" <hpa@zytor.com>, Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        riandrews@android.com,
        Serban Constantinescu <serban.constantinescu@arm.com>,
        Sumit Semwal <sumit.semwal@linaro.org>, devel@driverdev.osuosl.org
Subject: Re: [PATCH] android: binder: fix binder mmap failures
References: <CAMi1Hd0frW_BqLwPQ7M_CKbA8-SeYhykwWYFZ-_iSre5r3zK7Q@mail.gmail.com>
 <1425054614-388-1-git-send-email-a.ryabinin@samsung.com>
 <CALAqxLWGno+=nrpu3W5RkMnzqWLDXQQFymRYtVCngxXvw2c09w@mail.gmail.com>
In-reply-to: <CALAqxLWGno+=nrpu3W5RkMnzqWLDXQQFymRYtVCngxXvw2c09w@mail.gmail.com>
Content-type: text/plain; charset=utf-8
Content-transfer-encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4721
Lines: 101

On 02/27/2015 08:26 PM, John Stultz wrote:
> On Fri, Feb 27, 2015 at 8:30 AM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>> binder_update_page_range() initializes only addr and size
>> fields in 'struct vm_struct tmp_area;' and passes it to
>> map_vm_area().
>>
>> Before 71394fe50146 ("mm: vmalloc: add flag preventing guard hole allocation")
>> this was because map_vm_area() didn't use any other fields
>> in vm_struct except addr and size.
>>
>> Now get_vm_area_size() (used in map_vm_area()) reads vm_struct's
>> flags to determine whether vm area has guard hole or not.
>>
>> binder_update_page_range() don't initialize flags field, so
>> this causes following binder mmap failures:
>> -----------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1971 at mm/vmalloc.c:130
>> vmap_page_range_noflush+0x119/0x144()
>> CPU: 0 PID: 1971 Comm: healthd Not tainted 4.0.0-rc1-00399-g7da3fdc-dirty #157
>> Hardware name: ARM-Versatile Express
>> [<c001246d>] (unwind_backtrace) from [<c000f7f9>] (show_stack+0x11/0x14)
>> [<c000f7f9>] (show_stack) from [<c049a221>] (dump_stack+0x59/0x7c)
>> [<c049a221>] (dump_stack) from [<c001cf21>] (warn_slowpath_common+0x55/0x84)
>> [<c001cf21>] (warn_slowpath_common) from [<c001cfe3>]
>> (warn_slowpath_null+0x17/0x1c)
>> [<c001cfe3>] (warn_slowpath_null) from [<c00c66c5>]
>> (vmap_page_range_noflush+0x119/0x144)
>> [<c00c66c5>] (vmap_page_range_noflush) from [<c00c716b>] (map_vm_area+0x27/0x48)
>> [<c00c716b>] (map_vm_area) from [<c038ddaf>]
>> (binder_update_page_range+0x12f/0x27c)
>> [<c038ddaf>] (binder_update_page_range) from [<c038e857>]
>> (binder_mmap+0xbf/0x1ac)
>> [<c038e857>] (binder_mmap) from [<c00c2dc7>] (mmap_region+0x2eb/0x4d4)
>> [<c00c2dc7>] (mmap_region) from [<c00c3197>] (do_mmap_pgoff+0x1e7/0x250)
>> [<c00c3197>] (do_mmap_pgoff) from [<c00b35b5>] (vm_mmap_pgoff+0x45/0x60)
>> [<c00b35b5>] (vm_mmap_pgoff) from [<c00c1f39>] (SyS_mmap_pgoff+0x5d/0x80)
>> [<c00c1f39>] (SyS_mmap_pgoff) from [<c000ce81>] (ret_fast_syscall+0x1/0x5c)
>> ---[ end trace 48c2c4b9a1349e54 ]---
>> binder: 1982: binder_alloc_buf failed to map page at f0e00000 in kernel
>> binder: binder_mmap: 1982 b6bde000-b6cdc000 alloc small buf failed -12
>>
>> Use map_kernel_range_noflush() instead of map_vm_area() as this is better
>> API for binder's purposes and it allows to get rid of 'vm_struct tmp_area' at all.
>>
>> Fixes: 71394fe50146 ("mm: vmalloc: add flag preventing guard hole allocation")
>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>> ---
>>  drivers/android/binder.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 33b09b6..a984fbb 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -551,7 +551,6 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
>>  {
>>         void *page_addr;
>>         unsigned long user_page_addr;
>> -       struct vm_struct tmp_area;
>>         struct page **page;
>>         struct mm_struct *mm;
>>
>> @@ -600,9 +599,10 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
>>                                 proc->pid, page_addr);
>>                         goto err_alloc_page_failed;
>>                 }
>> -               tmp_area.addr = page_addr;
>> -               tmp_area.size = PAGE_SIZE + PAGE_SIZE /* guard page? */;
>> -               ret = map_vm_area(&tmp_area, PAGE_KERNEL, page);
>> +               ret = map_kernel_range_noflush((unsigned long)page_addr,
>> +                                       PAGE_SIZE, PAGE_KERNEL, page);
>> +               flush_cache_vmap((unsigned long)page_addr,
>> +                               (unsigned long)page_addr + PAGE_SIZE);
>>                 if (ret) {
>>                         pr_err("%d: binder_alloc_buf failed to map page at %p in kernel\n",
>>                                proc->pid, page_addr);
> 
> So with this patch I don't see the warnings, but I'm still seeing:
> [   11.154283] binder: 1956: binder_alloc_buf failed to map page at
> f0340000 in kernel
> [   11.154916] binder: binder_mmap: 1956 b6ce0000-b6d00000 alloc small
> buf failed -12
> 
> over and over.  So I don't think this patch is quite right.
> 

Thanks.

Seems that error check is wrong now.
map_vm_area() returns 0 on success, map_kernel_range_noflush() number of mapped pages.

> thanks
> -john
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/