Re: [cfs_trace_lock_tcd] BUG: KASAN: null-ptr-deref in cfs_trace_lock_tcd+0x25/0xeb
On 04/19/2018 04:35 PM, Andrey Ryabinin wrote: > > > On 04/18/2018 09:37 PM, Linus Torvalds wrote: >> Ugh, that lustre code is disgusting. >> >> I thought we were getting rid of it. >> >> Anyway, I started looking at why the stack trace is such an incredible >> mess, with lots of stale entries. >> >> The reason (well, _one_ reason) seems to be "ksocknal_startup". It has >> a 500-byte stack frame for some incomprehensible reason. I assume due >> to excessive inlining, because the function itself doesn't seem to be >> that bad. >> >> Similarly, LNetNIInit has a 300-byte stack frame. So it gets pretty deep. >> >> I'm getting the feeling that KASAN is making things worse because >> probably it's disabling all the sane stack frame stuff (ie no merging >> of stack slot entries, perhaps?). >> > > AFAIR no merging of stack slots policy enabled only if > -fsanitize-address-use-after-scope > is on (which is CONFIG_KASAN_EXTRA). This feature does cause sometimes > significant stack bloat, > but hasn't been proven to be very useful, so I wouldn't mind disabling it > completely. > > So far I know only about a single BUG - > https://lkml.kernel.org/r/<151238865557.4852.10258661301122491...@mail.alporthouse.com> > it has found. Actually, there is one more - https://syzkaller.appspot.com/bug?id=6a929b72a32ca0b1a6985126fa1bc77c03c12304 so two bugs. > There are also a lot of other I didn't finish this sentence: There are also a lot of other reports about use-after-scope, but seem all of them are false positives caused by STRUCTLEAK plugin. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [cfs_trace_lock_tcd] BUG: KASAN: null-ptr-deref in cfs_trace_lock_tcd+0x25/0xeb
On 04/18/2018 09:37 PM, Linus Torvalds wrote:
> Ugh, that lustre code is disgusting.
>
> I thought we were getting rid of it.
>
> Anyway, I started looking at why the stack trace is such an incredible
> mess, with lots of stale entries.
>
> The reason (well, _one_ reason) seems to be "ksocknal_startup". It has
> a 500-byte stack frame for some incomprehensible reason. I assume due
> to excessive inlining, because the function itself doesn't seem to be
> that bad.
>
> Similarly, LNetNIInit has a 300-byte stack frame. So it gets pretty deep.
>
> I'm getting the feeling that KASAN is making things worse because
> probably it's disabling all the sane stack frame stuff (ie no merging
> of stack slot entries, perhaps?).
>
AFAIR no merging of stack slots policy enabled only if
-fsanitize-address-use-after-scope
is on (which is CONFIG_KASAN_EXTRA). This feature does cause sometimes
significant stack bloat,
but hasn't been proven to be very useful, so I wouldn't mind disabling it
completely.
So far I know only about a single BUG -
https://lkml.kernel.org/r/<151238865557.4852.10258661301122491...@mail.alporthouse.com>
it has found.
There are also a lot of other
> Without KASAN (but also without a lot of other things, so I might be
> blaming KASAN incorrectly), the stack usage of ksocknal_startup() is
> just under 100 bytes, so if it is KASAN, it's really a big difference.
>
Yes, it's because of KASAN:
CONFIG_KASAN=n
socklnd.c:2795:1:ksocknal_startup 144 static
CONFIG_KASAN=y
CONFIG_KASAN_OUTLINE=y
CONFIG_KASAN_EXTRA=n
socklnd.c:2795:1:ksocknal_startup 552 static
CONFIG_KASAN=y
CONFIG_KASAN_OUTLINE=y
CONFIG_KASAN_EXTRA=y
socklnd.c:2795:1:ksocknal_startup 624 static
It's expected that KASAN may cause sometimes significant stack usage growth.
This is needed to catch out-of-bounds accesses to stack data.
When compiler can't proof that access to stack variable is valid (e.g.
reference to
stack variable passed to some external function), it will create redzones
around such
stack variable.
E.g. ksocknal_enumerate_interfaces() which is called only from
ksocknal_startup(), thus probably
inlined into ksocknal_startup() does this:
for (i = j = 0; i < n; i++) {
int up;
__u32 ip;
__u32 mask;
if (!strcmp(names[i], "lo")) /* skip the loopback IF */
continue;
rc = lnet_ipif_query(names[i], &up, &ip, &mask);
With KASAN stack might look something like this:
[32-byte left redzone of the stack frame] [up (4 bytes)] [28-bytes
redzone][ip (4 bytes)] [28-bytes redzone][mask (4 bytes)] [28-bytes
redzone][32-byte right redzone of the stack frame]
GCC always use 32-bytes redzones. AFAIK clang is more smart about this, it has
adaptive redzone policy - smaller redzones for small variables, and bigger for
big.
In this particular case, the best way to reduce stack usage is to refactor the
code.
1) Drop 'int *up' argument from lnet_ipif_query(). When interface is down
lnet_ipif_query() sets up to zero and doesn't return error.
But all callers treat up == 0 as error. So instead, lnet_ipif_query() should
simply return error code, and 'up' won't be needed.
This will simplify the code, and should drop the stack usage with KASAN and
without KASAN.
2) Instead of using local ip, mask variables, pass pointers
'&net->ksnn_interfaces[j].ksni_ipaddr', '&net->ksnn_interfaces[j].ksni_netmask'.
As in 1) this should alst drop the stack usage both with KASAN and without
KASAN
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] android: binder: fix binder mmap failures
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
[] (unwind_backtrace) from [] (show_stack+0x11/0x14)
[] (show_stack) from [] (dump_stack+0x59/0x7c)
[] (dump_stack) from [] (warn_slowpath_common+0x55/0x84)
[] (warn_slowpath_common) from []
(warn_slowpath_null+0x17/0x1c)
[] (warn_slowpath_null) from []
(vmap_page_range_noflush+0x119/0x144)
[] (vmap_page_range_noflush) from [] (map_vm_area+0x27/0x48)
[] (map_vm_area) from []
(binder_update_page_range+0x12f/0x27c)
[] (binder_update_page_range) from []
(binder_mmap+0xbf/0x1ac)
[] (binder_mmap) from [] (mmap_region+0x2eb/0x4d4)
[] (mmap_region) from [] (do_mmap_pgoff+0x1e7/0x250)
[] (do_mmap_pgoff) from [] (vm_mmap_pgoff+0x45/0x60)
[] (vm_mmap_pgoff) from [] (SyS_mmap_pgoff+0x5d/0x80)
[] (SyS_mmap_pgoff) from [] (ret_fast_syscall+0x1/0x5c)
---[ end trace 48c2c4b9a1349e54 ]---
binder: 1982: binder_alloc_buf failed to map page at f0e0 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
Reported-by: Amit Pundir
---
Changes since v1:
- fixed ret check after map_kernel_ranges_noflush().
drivers/android/binder.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 33b09b6..6607f3c 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,10 +599,11 @@ 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);
- if (ret) {
+ 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 != 1) {
pr_err("%d: binder_alloc_buf failed to map page at %p
in kernel\n",
proc->pid, page_addr);
goto err_map_kernel_failed;
--
2.3.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: binder: fix binder mmap failures
On 02/27/2015 08:26 PM, John Stultz wrote:
> On Fri, Feb 27, 2015 at 8:30 AM, Andrey Ryabinin
> 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
>> [] (unwind_backtrace) from [] (show_stack+0x11/0x14)
>> [] (show_stack) from [] (dump_stack+0x59/0x7c)
>> [] (dump_stack) from [] (warn_slowpath_common+0x55/0x84)
>> [] (warn_slowpath_common) from []
>> (warn_slowpath_null+0x17/0x1c)
>> [] (warn_slowpath_null) from []
>> (vmap_page_range_noflush+0x119/0x144)
>> [] (vmap_page_range_noflush) from []
>> (map_vm_area+0x27/0x48)
>> [] (map_vm_area) from []
>> (binder_update_page_range+0x12f/0x27c)
>> [] (binder_update_page_range) from []
>> (binder_mmap+0xbf/0x1ac)
>> [] (binder_mmap) from [] (mmap_region+0x2eb/0x4d4)
>> [] (mmap_region) from [] (do_mmap_pgoff+0x1e7/0x250)
>> [] (do_mmap_pgoff) from [] (vm_mmap_pgoff+0x45/0x60)
>> [] (vm_mmap_pgoff) from [] (SyS_mmap_pgoff+0x5d/0x80)
>> [] (SyS_mmap_pgoff) from [] (ret_fast_syscall+0x1/0x5c)
>> ---[ end trace 48c2c4b9a1349e54 ]---
>> binder: 1982: binder_alloc_buf failed to map page at f0e0 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
>> Reported-by: Amit Pundir
>> ---
>> 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
> f034 in kernel
> [ 11.154916] binder: binder_mmap: 1956 b6ce-b6d0 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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] android: binder: fix binder mmap failures
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
[] (unwind_backtrace) from [] (show_stack+0x11/0x14)
[] (show_stack) from [] (dump_stack+0x59/0x7c)
[] (dump_stack) from [] (warn_slowpath_common+0x55/0x84)
[] (warn_slowpath_common) from []
(warn_slowpath_null+0x17/0x1c)
[] (warn_slowpath_null) from []
(vmap_page_range_noflush+0x119/0x144)
[] (vmap_page_range_noflush) from [] (map_vm_area+0x27/0x48)
[] (map_vm_area) from []
(binder_update_page_range+0x12f/0x27c)
[] (binder_update_page_range) from []
(binder_mmap+0xbf/0x1ac)
[] (binder_mmap) from [] (mmap_region+0x2eb/0x4d4)
[] (mmap_region) from [] (do_mmap_pgoff+0x1e7/0x250)
[] (do_mmap_pgoff) from [] (vm_mmap_pgoff+0x45/0x60)
[] (vm_mmap_pgoff) from [] (SyS_mmap_pgoff+0x5d/0x80)
[] (SyS_mmap_pgoff) from [] (ret_fast_syscall+0x1/0x5c)
---[ end trace 48c2c4b9a1349e54 ]---
binder: 1982: binder_alloc_buf failed to map page at f0e0 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
Reported-by: Amit Pundir
---
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);
--
2.3.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
