Re: [PATCH v2] binder: Don't use mmput() from shrinker function.
On 2020/07/17 1:29, Christian Brauner wrote: > Does this need a Cc: stable? Up to someone who applies this patch. I think this race is hard to hit. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] binder: Don't use mmput() from shrinker function.
syzbot is reporting that mmput() from shrinker function has a risk of
deadlock [1], for delayed_uprobe_add() from update_ref_ctr() calls
kzalloc(GFP_KERNEL) with delayed_uprobe_lock held, and
uprobe_clear_state() from __mmput() also holds delayed_uprobe_lock.
Commit a1b2289cef92ef0e ("android: binder: drop lru lock in isolate
callback") replaced mmput() with mmput_async() in order to avoid sleeping
with spinlock held. But this patch replaces mmput() with mmput_async() in
order not to start __mmput() from shrinker context.
[1]
https://syzkaller.appspot.com/bug?id=bc9e7303f537c41b2b0cc2dfcea3fc42964c2d45
Reported-by: syzbot
Reported-by: syzbot
Signed-off-by: Tetsuo Handa
---
drivers/android/binder_alloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 42c672f1584e..cbe6aa77d50d 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -947,7 +947,7 @@ enum lru_status binder_alloc_free_page(struct list_head
*item,
trace_binder_unmap_user_end(alloc, index);
}
mmap_read_unlock(mm);
- mmput(mm);
+ mmput_async(mm);
trace_binder_unmap_kernel_start(alloc, index);
--
2.18.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: Don't use mmput() from shrinker function.
On 2020/07/16 17:35, Michal Hocko wrote:
> On Thu 16-07-20 08:36:52, Tetsuo Handa wrote:
>> syzbot is reporting that mmput() from shrinker function has a risk of
>> deadlock [1]. Don't start synchronous teardown of mm when called from
>> shrinker function.
>
> Please add the actual lock dependency to the changelog.
>
> Anyway is this deadlock real? Mayve I have missed some details but the
> call graph points to these two paths.
> uprobe_mmap do_shrink_slab
> uprobes_mmap_hash #lock
> install_breakpointbinder_shrink_scan
> set_swbp binder_alloc_free_page
> uprobe_write_opcode __mmput
> update_ref_ctr uprobe_clear_state
> mutex_lock(_uprobe_lock)
> mutex_lock(_uprobe_lock);
> allocation -> reclaim
>
static int update_ref_ctr(struct uprobe *uprobe, struct mm_struct *mm, short d)
{
mutex_lock(_uprobe_lock);
ret = delayed_uprobe_add(uprobe, mm1) {
du = kzalloc(sizeof(*du), GFP_KERNEL) {
do_shrink_slab() {
binder_shrink_scan() {
binder_alloc_free_page() {
mmget_not_zero(mm2);
mmput(mm2) {
__mmput(mm2) {
uprobe_clear_state(mm2) {
mutex_lock(_uprobe_lock);
delayed_uprobe_remove(NULL, mm2);
mutex_unlock(_uprobe_lock);
}
}
}
}
}
}
}
}
mutex_unlock(_uprobe_lock);
}
> But in order for this to happen the shrinker would have to do the last
> put on the mm. But mm cannot go away from under uprobe_mmap so those two
> paths cannot race with each other.
and mm1 != mm2 is possible, isn't it?
>
> Unless I am missing something this is a false positive. I do not mind
> using mmput_async from the shrinker as a workaround but the changelog
> should be explicit about the fact.
>
binder_alloc_free_page() is already using mmput_async() 14 lines later.
It just took 18 months to hit this race for the third time, for it is
quite difficult to let the owner of mm2 to call mmput(mm2) between
binder_alloc_free_page() calls mmget_not_zero(mm2) and mmput(mm2).
The reason I added you is to see whether we can do
void mmput(struct mm_struct *mm)
{
might_sleep();
+ /* Calling mmput() from shrinker context can deadlock. */
+ WARN_ON(current->flags & PF_MEMALLOC);
if (atomic_dec_and_test(>mm_users))
__mmput(mm);
}
in order to catch this bug easier.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] binder: Don't use mmput() from shrinker function.
syzbot is reporting that mmput() from shrinker function has a risk of deadlock [1]. Don't start synchronous teardown of mm when called from shrinker function. [1] https://syzkaller.appspot.com/bug?id=bc9e7303f537c41b2b0cc2dfcea3fc42964c2d45 Reported-by: syzbot Reported-by: syzbot Signed-off-by: Tetsuo Handa --- drivers/android/binder_alloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 42c672f1584e..cbe6aa77d50d 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -947,7 +947,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item, trace_binder_unmap_user_end(alloc, index); } mmap_read_unlock(mm); - mmput(mm); + mmput_async(mm); trace_binder_unmap_kernel_start(alloc, index); -- 2.18.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ashmem: Avoid range_alloc() allocation with ashmem_mutex held.
On 2019/02/26 6:55, Joel Fernandes wrote: >> @@ -763,6 +767,8 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, >> unsigned long cmd, >> >> out_unlock: >> mutex_unlock(_mutex); >> +if (range) >> +kmem_cache_free(ashmem_range_cachep, range); > > This seems a bit broken to me. Once a range has been added to the LRU list, > it is then being freed here. So then the ashmem_lru_list will contain a > dangling range, right? If this range was used in range_alloc(), range == NULL here due to + struct ashmem_range *range = *new_range; + *new_range = NULL; . Thus, this range won't be freed here if range_alloc() was called. What am I missing? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: android: ashmem: Avoid range_alloc() allocation with ashmem_mutex held.
ashmem_pin() is calling range_shrink() without checking whether
range_alloc() succeeded. Also, doing memory allocation with ashmem_mutex
held should be avoided because ashmem_shrink_scan() tries to hold it.
Therefore, move memory allocation for range_alloc() to ashmem_pin_unpin()
and make range_alloc() not to fail.
This patch is mostly meant for backporting purpose for fuzz testing on
stable/distributor kernels, for there is a plan to remove this code in
near future.
Signed-off-by: Tetsuo Handa
Cc: sta...@vger.kernel.org
---
drivers/staging/android/ashmem.c | 42 +++-
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 5d5b091..74d497d 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -172,19 +172,15 @@ static inline void lru_del(struct ashmem_range *range)
* @end: The ending page (inclusive)
*
* This function is protected by ashmem_mutex.
- *
- * Return: 0 if successful, or -ENOMEM if there is an error
*/
-static int range_alloc(struct ashmem_area *asma,
- struct ashmem_range *prev_range, unsigned int purged,
- size_t start, size_t end)
+static void range_alloc(struct ashmem_area *asma,
+ struct ashmem_range *prev_range, unsigned int purged,
+ size_t start, size_t end,
+ struct ashmem_range **new_range)
{
- struct ashmem_range *range;
-
- range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
- if (!range)
- return -ENOMEM;
+ struct ashmem_range *range = *new_range;
+ *new_range = NULL;
range->asma = asma;
range->pgstart = start;
range->pgend = end;
@@ -194,8 +190,6 @@ static int range_alloc(struct ashmem_area *asma,
if (range_on_lru(range))
lru_add(range);
-
- return 0;
}
/**
@@ -597,7 +591,8 @@ static int get_name(struct ashmem_area *asma, void __user
*name)
*
* Caller must hold ashmem_mutex.
*/
-static int ashmem_pin(struct ashmem_area *asma, size_t pgstart, size_t pgend)
+static int ashmem_pin(struct ashmem_area *asma, size_t pgstart, size_t pgend,
+ struct ashmem_range **new_range)
{
struct ashmem_range *range, *next;
int ret = ASHMEM_NOT_PURGED;
@@ -650,7 +645,7 @@ static int ashmem_pin(struct ashmem_area *asma, size_t
pgstart, size_t pgend)
* second half and adjust the first chunk's endpoint.
*/
range_alloc(asma, range, range->purged,
- pgend + 1, range->pgend);
+ pgend + 1, range->pgend, new_range);
range_shrink(range, range->pgstart, pgstart - 1);
break;
}
@@ -664,7 +659,8 @@ static int ashmem_pin(struct ashmem_area *asma, size_t
pgstart, size_t pgend)
*
* Caller must hold ashmem_mutex.
*/
-static int ashmem_unpin(struct ashmem_area *asma, size_t pgstart, size_t pgend)
+static int ashmem_unpin(struct ashmem_area *asma, size_t pgstart, size_t pgend,
+ struct ashmem_range **new_range)
{
struct ashmem_range *range, *next;
unsigned int purged = ASHMEM_NOT_PURGED;
@@ -690,7 +686,8 @@ static int ashmem_unpin(struct ashmem_area *asma, size_t
pgstart, size_t pgend)
}
}
- return range_alloc(asma, range, purged, pgstart, pgend);
+ range_alloc(asma, range, purged, pgstart, pgend, new_range);
+ return 0;
}
/*
@@ -723,10 +720,17 @@ static int ashmem_pin_unpin(struct ashmem_area *asma,
unsigned long cmd,
struct ashmem_pin pin;
size_t pgstart, pgend;
int ret = -EINVAL;
+ struct ashmem_range *range = NULL;
if (copy_from_user(, p, sizeof(pin)))
return -EFAULT;
+ if (cmd == ASHMEM_PIN || cmd == ASHMEM_UNPIN) {
+ range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
+ if (!range)
+ return -ENOMEM;
+ }
+
mutex_lock(_mutex);
wait_event(ashmem_shrink_wait, !atomic_read(_shrink_inflight));
@@ -751,10 +755,10 @@ static int ashmem_pin_unpin(struct ashmem_area *asma,
unsigned long cmd,
switch (cmd) {
case ASHMEM_PIN:
- ret = ashmem_pin(asma, pgstart, pgend);
+ ret = ashmem_pin(asma, pgstart, pgend, );
break;
case ASHMEM_UNPIN:
- ret = ashmem_unpin(asma, pgstart, pgend);
+ ret = ashmem_unpin(asma, pgstart, pgend, );
break;
case ASHMEM_GET_PIN_STATUS:
ret = ashmem_get_pin_status(asma, pgstart, pgend);
@@ -763,6 +767,8 @@ static int ashmem_pin_unpin(struct ashmem_area *asma,
unsigned lo
Re: [PATCH 2/2] staging: android: ashmem: Don't allow range_alloc() to fail.
Joel Fernandes wrote:
> I am sorry, what has changed in the updated patch? Please send clear diff
> notes in your patches or at least mention exactly what changed since previous
> patch revision. It is very difficult to review if you don't even mention what
> changed since previous revision. Please resend the patches again.
>
> Also I am OK with assuming small alloc success, so we are on the same page
> about that.
>
> One more comment below..
>
> Thanks!
>
> > @@ -722,10 +719,17 @@ static int ashmem_pin_unpin(struct ashmem_area *asma,
> > unsigned long cmd,
> > struct ashmem_pin pin;
> > size_t pgstart, pgend;
> > int ret = -EINVAL;
> > + struct ashmem_range *range = NULL;
> >
> > if (copy_from_user(, p, sizeof(pin)))
> > return -EFAULT;
> >
> > + if (cmd == ASHMEM_PIN || cmd == ASHMEM_UNPIN) {
> > + range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
> > + if (!range)
> > + return -ENOMEM;
>
> According to the too-small-to-fail rule, why are you checking for errors
> here?
The "too small to fail" memory-allocation rule is not __GFP_NOFAIL; it is
almost __GFP_NOFAIL.
Small __GFP_NOFAIL allocation might fail if current thread was killed by the
OOM killer or
memory allocation fault injection forced it to fail. And syzbot is finding many
bugs (using
memory allocation fault injection) where the caller is not checking for
allocation failures.
The old patch tried to allow syzbot to test this module by using __GFP_NOFAIL
(i.e. minimal
change), and the new patch is trying to allow syzbot to test this module by
moving
small __GFP_KERNEL allocation out of ashmem_mutex (i.e. cleaner change).
>
> > + }
> > +
> > mutex_lock(_mutex);
> > wait_event(ashmem_shrink_wait, !atomic_read(_shrink_inflight));
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: android: ashmem: Don't allow range_alloc() to fail.
On 2019/02/08 23:45, Joel Fernandes wrote:
> On Wed, Feb 06, 2019 at 08:45:32AM +0900, Tetsuo Handa wrote:
>> Joel Fernandes wrote:
>>> On Tue, Feb 05, 2019 at 07:28:41PM +0900, Tetsuo Handa wrote:
>>>> ashmem_pin() is calling range_shrink() without checking whether
>>>> range_alloc() succeeded. Since memory allocation fault injection might
>>>> force range_alloc() to fail while range_alloc() is called for only once
>>>> for one ioctl() request, make range_alloc() not to fail.
>>>
>>> Why does this not need to fail? I am worried your change will introduce
>>> unwanted endless looping in the kernel instead of gracefully failing a
>>> pin/unpin request.
>>
>> This patch won't introduce endless looping in the kernel, due to a rule
>> called
>>
>> The "too small to fail" memory-allocation rule
>> https://lwn.net/Articles/627419/
>>
>> . In short, memory allocation by range_alloc() might fail only if current
>> thread
>> was killed by the OOM killer or memory allocation fault injection mechanism
>> forced it to fail. And this patch helps doing fuzzing test with minimal
>> changes.
>>
>>>
>>> Unless there is a good reason, I suggest to drop this patch from the series;
>>> but let us discuss more if you want.
>>
>> We can allocate memory for range_alloc() before holding ashmem_mutex at
>> ashmem_pin_unpin() if you don't want to use __GFP_NOFAIL. It is better to
>> avoid memory allocation with ashmem_mutex because ashmem_mutex is held by
>> shrinker function. But given that this module is going to be replaced
>> shortly,
>> does it worth moving the memory allocation to the caller?
>
> Even if memory allocation does not fail right now, I still want to return the
> error code correctly via ashmem_pin_unpin() so that if in the future there
> are changes to the allocator algorithm, then a silent success isn't reported
> when a failure should be reported..
>
> It doesn't make sense to return success when an allocation failed.. even if
> you are asking this code to rely on the allocator's "too small to fail"
> behavior.. can we guarantee this allocator behavior will always exist?
> Probably not.
>
Does always exist. Small GFP_KERNEL allocation without __GFP_NORETRY or
__GFP_RETRY_MAYFAIL won't fail unless current thread was killed by the OOM
killer or memory allocation fault injection mechanism forced it to fail.
Failing such allocation instead of invoking the OOM killer is effectively
equivalent to sending SIGKILL to current thread. If current thread got
SIGKILL, current thread can't receive the error code of ioctl() from
ashmem_pin_unpin() because userspace code is no longer executed.
MM people want to make !GFP_KERNEL memory allocations fail rather than
retry forever. But failing small GFP_KERNEL memory allocations is such
a horrible idea. Anyway, here is an updated patch...
>From f2c8a098ebf69122fc440d9e9ca3c9cd786cce8a Mon Sep 17 00:00:00 2001
From: Tetsuo Handa
Date: Sat, 9 Feb 2019 11:07:07 +0900
Subject: [PATCH] staging: android: ashmem: Avoid range_alloc() allocation with
ashmem_mutex held.
ashmem_pin() is calling range_shrink() without checking whether
range_alloc() succeeded. Also, doing memory allocation with ashmem_mutex
held should be avoided because ashmem_shrink_scan() tries to hold it.
Therefore, move memory allocation for range_alloc() to ashmem_pin_unpin()
and make range_alloc() not to fail.
Signed-off-by: Tetsuo Handa
---
drivers/staging/android/ashmem.c | 42 +++-
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index ade8438..910826d 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -171,19 +171,15 @@ static inline void lru_del(struct ashmem_range *range)
* @end: The ending page (inclusive)
*
* This function is protected by ashmem_mutex.
- *
- * Return: 0 if successful, or -ENOMEM if there is an error
*/
-static int range_alloc(struct ashmem_area *asma,
- struct ashmem_range *prev_range, unsigned int purged,
- size_t start, size_t end)
+static void range_alloc(struct ashmem_area *asma,
+ struct ashmem_range *prev_range, unsigned int purged,
+ size_t start, size_t end,
+ struct ashmem_range **new_range)
{
- struct ashmem_range *range;
-
- range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
- if (!range)
- return -ENOMEM;
+ struct ashmem_range *range = *new_range;
+ *new_range = NULL;
range->asma = asma;
range->pgstart =
Re: [PATCH 2/2] staging: android: ashmem: Don't allow range_alloc() to fail.
Joel Fernandes wrote: > On Tue, Feb 05, 2019 at 07:28:41PM +0900, Tetsuo Handa wrote: > > ashmem_pin() is calling range_shrink() without checking whether > > range_alloc() succeeded. Since memory allocation fault injection might > > force range_alloc() to fail while range_alloc() is called for only once > > for one ioctl() request, make range_alloc() not to fail. > > Why does this not need to fail? I am worried your change will introduce > unwanted endless looping in the kernel instead of gracefully failing a > pin/unpin request. This patch won't introduce endless looping in the kernel, due to a rule called The "too small to fail" memory-allocation rule https://lwn.net/Articles/627419/ . In short, memory allocation by range_alloc() might fail only if current thread was killed by the OOM killer or memory allocation fault injection mechanism forced it to fail. And this patch helps doing fuzzing test with minimal changes. > > Unless there is a good reason, I suggest to drop this patch from the series; > but let us discuss more if you want. We can allocate memory for range_alloc() before holding ashmem_mutex at ashmem_pin_unpin() if you don't want to use __GFP_NOFAIL. It is better to avoid memory allocation with ashmem_mutex because ashmem_mutex is held by shrinker function. But given that this module is going to be replaced shortly, does it worth moving the memory allocation to the caller? > > thanks, > > - Joel > > From the docs: > __GFP_NOFAIL - Indicate that this allocation is in no way allowed to fail > (think twice before using). > The "too small to fail" memory-allocation rule already made almost __GFP_NOFAIL. :-) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: android: ashmem: Don't allow range_alloc() to fail.
ashmem_pin() is calling range_shrink() without checking whether
range_alloc() succeeded. Since memory allocation fault injection might
force range_alloc() to fail while range_alloc() is called for only once
for one ioctl() request, make range_alloc() not to fail.
Signed-off-by: Tetsuo Handa
---
drivers/staging/android/ashmem.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index d40c1d2..a8070a2 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -171,18 +171,14 @@ static inline void lru_del(struct ashmem_range *range)
* @end: The ending page (inclusive)
*
* This function is protected by ashmem_mutex.
- *
- * Return: 0 if successful, or -ENOMEM if there is an error
*/
-static int range_alloc(struct ashmem_area *asma,
- struct ashmem_range *prev_range, unsigned int purged,
- size_t start, size_t end)
+static void range_alloc(struct ashmem_area *asma,
+ struct ashmem_range *prev_range, unsigned int purged,
+ size_t start, size_t end)
{
struct ashmem_range *range;
- range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
- if (!range)
- return -ENOMEM;
+ range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL |
__GFP_NOFAIL);
range->asma = asma;
range->pgstart = start;
@@ -193,8 +189,6 @@ static int range_alloc(struct ashmem_area *asma,
if (range_on_lru(range))
lru_add(range);
-
- return 0;
}
/**
@@ -687,7 +681,8 @@ static int ashmem_unpin(struct ashmem_area *asma, size_t
pgstart, size_t pgend)
}
}
- return range_alloc(asma, range, purged, pgstart, pgend);
+ range_alloc(asma, range, purged, pgstart, pgend);
+ return 0;
}
/*
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: android: ashmem: Don't call fallocate() with ashmem_mutex held.
syzbot is hitting lockdep warnings [1][2][3]. This patch tries to fix
the warning by eliminating ashmem_shrink_scan() => {shmem|vfs}_fallocate()
sequence.
[1]
https://syzkaller.appspot.com/bug?id=87c399f6fa6955006080b24142e2ce7680295ad4
[2]
https://syzkaller.appspot.com/bug?id=7ebea492de7521048355fc84210220e1038a7908
[3]
https://syzkaller.appspot.com/bug?id=e02419c12131c24e2a957ea050c2ab6dcbbc3270
Reported-by: syzbot
Reported-by: syzbot
Reported-by: syzbot
Signed-off-by: Tetsuo Handa
Cc: sta...@vger.kernel.org
---
drivers/staging/android/ashmem.c | 25 -
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 90a8a9f1ac7d..ade8438a827a 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -75,6 +75,9 @@ struct ashmem_range {
/* LRU list of unpinned pages, protected by ashmem_mutex */
static LIST_HEAD(ashmem_lru_list);
+static atomic_t ashmem_shrink_inflight = ATOMIC_INIT(0);
+static DECLARE_WAIT_QUEUE_HEAD(ashmem_shrink_wait);
+
/*
* long lru_count - The count of pages on our LRU list.
*
@@ -438,7 +441,6 @@ static int ashmem_mmap(struct file *file, struct
vm_area_struct *vma)
static unsigned long
ashmem_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
{
- struct ashmem_range *range, *next;
unsigned long freed = 0;
/* We might recurse into filesystem code, so bail out if necessary */
@@ -448,21 +450,33 @@ ashmem_shrink_scan(struct shrinker *shrink, struct
shrink_control *sc)
if (!mutex_trylock(_mutex))
return -1;
- list_for_each_entry_safe(range, next, _lru_list, lru) {
+ while (!list_empty(_lru_list)) {
+ struct ashmem_range *range =
+ list_first_entry(_lru_list, typeof(*range), lru);
loff_t start = range->pgstart * PAGE_SIZE;
loff_t end = (range->pgend + 1) * PAGE_SIZE;
+ struct file *f = range->asma->file;
- range->asma->file->f_op->fallocate(range->asma->file,
- FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
- start, end - start);
+ get_file(f);
+ atomic_inc(_shrink_inflight);
range->purged = ASHMEM_WAS_PURGED;
lru_del(range);
freed += range_size(range);
+ mutex_unlock(_mutex);
+ f->f_op->fallocate(f,
+ FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+ start, end - start);
+ fput(f);
+ if (atomic_dec_and_test(_shrink_inflight))
+ wake_up_all(_shrink_wait);
+ if (!mutex_trylock(_mutex))
+ goto out;
if (--sc->nr_to_scan <= 0)
break;
}
mutex_unlock(_mutex);
+out:
return freed;
}
@@ -713,6 +727,7 @@ static int ashmem_pin_unpin(struct ashmem_area *asma,
unsigned long cmd,
return -EFAULT;
mutex_lock(_mutex);
+ wait_event(ashmem_shrink_wait, !atomic_read(_shrink_inflight));
if (!asma->file)
goto out_unlock;
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] ANDROID: binder: Latelimit binder_debug().
On 2018/07/10 21:40, Martijn Coenen wrote: > On Tue, Jul 10, 2018 at 2:09 PM, Tetsuo Handa > wrote: >> I don't have benchmark data (I'm not an Android user). But an example log at >> https://syzkaller.appspot.com/text?tag=CrashLog=12f316fc40 got >> about 13214 messages in 124 seconds (over 100 messages per a second). > > I meant data for the cond_resched() change, which could have an impact > on IPC latency. But yeah, this is a lot. We typically don't see these > messages on Android userspace, because most of them would be caused be > userspace errors that can't happen when using binder libraries. > Obviously syzkaller doesn't use those. I think we'd like to keep > having these in Android builds, let me discuss internally if we can > just switch the default debug level and enable them in Android through > other means. Any progress on this problem? > >> Without disabling by default or latelimit printk(), the system shall become >> unusable. >> >> $ grep binder: log | wc -l >> 13214 >> $ head log >> [ 1167.389978] binder: 15631:15631 got reply transaction with no transaction >> stack >> [ 1167.391813] binder: 15629:15629 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1167.399282] binder: 15631:15631 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1167.399548] binder: undelivered TRANSACTION_ERROR: 29201 >> [ 1167.419645] binder: 15625:15625 got reply transaction with no transaction >> stack >> [ 1167.427800] binder: 15625:15625 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1167.461506] binder: 15634:15634 got reply transaction with no transaction >> stack >> [ 1167.469060] binder: 15634:15634 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1167.472747] binder: 15638:15638 got reply transaction with no transaction >> stack >> [ 1167.477550] binder: 15633:15633 got reply transaction with no transaction >> stack >> $ tail log >> [ 1291.131046] binder: 25566:25566 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1291.140761] binder: 25553:25553 got reply transaction with no transaction >> stack >> [ 1291.151698] binder: 25553:25553 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1291.182362] binder: 25568:25568 got reply transaction with no transaction >> stack >> [ 1291.183361] binder: undelivered TRANSACTION_ERROR: 29201 >> [ 1291.189926] binder: 25568:25568 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1291.204438] binder: 25569:25569 got reply transaction with no transaction >> stack >> [ 1291.211953] binder: 25569:25569 transaction failed 29201/-71, size 0-8 >> line 2759 >> [ 1291.213825] binder: 25572:25572 got reply transaction with no transaction >> stack >> [ 1291.227018] binder: 25572:25572 transaction failed 29201/-71, size 0-8 >> line 2759 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] ANDROID: binder: Latelimit binder_debug().
On 2018/07/09 23:29, Tetsuo Handa wrote: > On 2018/07/09 23:02, Martijn Coenen wrote: >> On Mon, Jul 9, 2018 at 3:27 PM, Dmitry Vyukov wrote: >>> I know almost nothing about binder. How these debug messages are >>> enabled? I don't see anything like CONFIG_BINDER_VERBOSE_DEBUG in the >>> config: >>> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-kasan.config >>> Also no mentions of binder in sysctl/cmline. >>> All binder maintainers are in CC already, perhaps they can shed some >>> light on this. >> >> Some are enabled by default here: >> https://github.com/torvalds/linux/blob/master/drivers/android/binder.c#L138 >> >> Perhaps we should revise that set then, as it can be quite noisy when >> lots of processes are dying (which also happens a lot in syzbot >> tests). >> > > OK. Then, I think we should consider disabling by default or latelimit > printk(). > Repeating hundreds of (or maybe thousands of ?) similar messages is not useful > (and blocking other threads due to processing flood of printk() is not good). > I don't have benchmark data (I'm not an Android user). But an example log at https://syzkaller.appspot.com/text?tag=CrashLog=12f316fc40 got about 13214 messages in 124 seconds (over 100 messages per a second). Without disabling by default or latelimit printk(), the system shall become unusable. $ grep binder: log | wc -l 13214 $ head log [ 1167.389978] binder: 15631:15631 got reply transaction with no transaction stack [ 1167.391813] binder: 15629:15629 transaction failed 29201/-71, size 0-8 line 2759 [ 1167.399282] binder: 15631:15631 transaction failed 29201/-71, size 0-8 line 2759 [ 1167.399548] binder: undelivered TRANSACTION_ERROR: 29201 [ 1167.419645] binder: 15625:15625 got reply transaction with no transaction stack [ 1167.427800] binder: 15625:15625 transaction failed 29201/-71, size 0-8 line 2759 [ 1167.461506] binder: 15634:15634 got reply transaction with no transaction stack [ 1167.469060] binder: 15634:15634 transaction failed 29201/-71, size 0-8 line 2759 [ 1167.472747] binder: 15638:15638 got reply transaction with no transaction stack [ 1167.477550] binder: 15633:15633 got reply transaction with no transaction stack $ tail log [ 1291.131046] binder: 25566:25566 transaction failed 29201/-71, size 0-8 line 2759 [ 1291.140761] binder: 25553:25553 got reply transaction with no transaction stack [ 1291.151698] binder: 25553:25553 transaction failed 29201/-71, size 0-8 line 2759 [ 1291.182362] binder: 25568:25568 got reply transaction with no transaction stack [ 1291.183361] binder: undelivered TRANSACTION_ERROR: 29201 [ 1291.189926] binder: 25568:25568 transaction failed 29201/-71, size 0-8 line 2759 [ 1291.204438] binder: 25569:25569 got reply transaction with no transaction stack [ 1291.211953] binder: 25569:25569 transaction failed 29201/-71, size 0-8 line 2759 [ 1291.213825] binder: 25572:25572 got reply transaction with no transaction stack [ 1291.227018] binder: 25572:25572 transaction failed 29201/-71, size 0-8 line 2759 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] ANDROID: binder: Latelimit binder_debug().
On 2018/07/09 23:02, Martijn Coenen wrote: > On Mon, Jul 9, 2018 at 3:27 PM, Dmitry Vyukov wrote: >> I know almost nothing about binder. How these debug messages are >> enabled? I don't see anything like CONFIG_BINDER_VERBOSE_DEBUG in the >> config: >> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-kasan.config >> Also no mentions of binder in sysctl/cmline. >> All binder maintainers are in CC already, perhaps they can shed some >> light on this. > > Some are enabled by default here: > https://github.com/torvalds/linux/blob/master/drivers/android/binder.c#L138 > > Perhaps we should revise that set then, as it can be quite noisy when > lots of processes are dying (which also happens a lot in syzbot > tests). > OK. Then, I think we should consider disabling by default or latelimit printk(). Repeating hundreds of (or maybe thousands of ?) similar messages is not useful (and blocking other threads due to processing flood of printk() is not good). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] ANDROID: binder: Latelimit binder_debug().
Dmitry, do you know how/why syzbot is enabling debug messages? On 2018/07/09 16:38, Greg Kroah-Hartman wrote: > On Mon, Jul 09, 2018 at 10:10:34AM +0900, Tetsuo Handa wrote: >> >From 62ddef96020cb397dcbf4b8574f1859b32f983de Mon Sep 17 00:00:00 2001 >> From: Tetsuo Handa >> Date: Mon, 9 Jul 2018 09:54:01 +0900 >> Subject: [PATCH] ANDROID: binder: Latelimit binder_debug(). >> >> syzbot is reporting hung tasks [1] [2]. This might be due to flooding of >> printk() messages from binder subsystem, for NMI backtrace says the CPU >> was busy doing printk() from binder subsystem. Since the kernel log buffer >> is trivially spammed by debug messages, let's latelimit binder_debug(). > > How is the binder debug messages being turned on? They are not enabled > by default, is syzbot enabling them? If so, then I don't know if this > needs to be changed, as you want those debug messages if you ask for > them, you shouldn't ratelimit them. > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] ANDROID: binder: Latelimit binder_debug().
>From 62ddef96020cb397dcbf4b8574f1859b32f983de Mon Sep 17 00:00:00 2001
From: Tetsuo Handa
Date: Mon, 9 Jul 2018 09:54:01 +0900
Subject: [PATCH] ANDROID: binder: Latelimit binder_debug().
syzbot is reporting hung tasks [1] [2]. This might be due to flooding of
printk() messages from binder subsystem, for NMI backtrace says the CPU
was busy doing printk() from binder subsystem. Since the kernel log buffer
is trivially spammed by debug messages, let's latelimit binder_debug().
While at it, let's add cond_resched() to binder_thread_write(),
binder_transaction() and binder_release_work() loops because they might
take long time.
[1]
https://syzkaller.appspot.com/bug?id=0e75779a6f0faac461510c6330514e8f0e893038
[2]
https://syzkaller.appspot.com/bug?id=aa11d2d767f3750ef9a40d156a149e9cfa735b73
Signed-off-by: Tetsuo Handa
Reported-by: syzbot+e38306788a2e7102a...@syzkaller.appspotmail.com
Reported-by: syzbot+4417a2fa149da3802...@syzkaller.appspotmail.com
---
drivers/android/binder.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 95283f3..c136fce 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -161,7 +161,7 @@ static int binder_set_stop_on_user_error(const char *val,
#define binder_debug(mask, x...) \
do { \
if (binder_debug_mask & mask) \
- pr_info(x); \
+ pr_info_ratelimited(x); \
} while (0)
#define binder_user_error(x...) \
@@ -3016,7 +3016,7 @@ static void binder_transaction(struct binder_proc *proc,
sg_bufp = (u8 *)(PTR_ALIGN(off_end, sizeof(void *)));
sg_buf_end = sg_bufp + extra_buffers_size;
off_min = 0;
- for (; offp < off_end; offp++) {
+ for (; offp < off_end; cond_resched(), offp++) {
struct binder_object_header *hdr;
size_t object_size = binder_validate_object(t->buffer, *offp);
@@ -3307,6 +3307,7 @@ static int binder_thread_write(struct binder_proc *proc,
if (get_user(cmd, (uint32_t __user *)ptr))
return -EFAULT;
+ cond_resched();
ptr += sizeof(uint32_t);
trace_binder_command(cmd);
if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
@@ -4193,6 +4194,7 @@ static void binder_release_work(struct binder_proc *proc,
struct binder_work *w;
while (1) {
+ cond_resched();
w = binder_dequeue_work_head(proc, list);
if (!w)
return;
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: android: ion: Check for register_shrinker() failure.
register_shrinker() might return -ENOMEM error since Linux 3.12.
But since callers of ion_device_add_heap() are not ready to receive an
error and it is not simple enough to fix within this patch, this patch
just prints a warning line when register_shrinker() failed.
Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Cc: John Stultz <john.stu...@linaro.org>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: Michal Hocko <mho...@suse.com>
---
drivers/staging/android/ion/ion_heap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/android/ion/ion_heap.c
b/drivers/staging/android/ion/ion_heap.c
index 91faa7f..56dcbd8 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -312,5 +312,6 @@ void ion_heap_init_shrinker(struct ion_heap *heap)
heap->shrinker.scan_objects = ion_heap_shrink_scan;
heap->shrinker.seeks = DEFAULT_SEEKS;
heap->shrinker.batch = 0;
- register_shrinker(>shrinker);
+ if (register_shrinker(>shrinker))
+ pr_err("Failed to create heap shrinker for %s\n", heap->name);
}
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] android: binder: Check for errors in binder_alloc_shrinker_init().
Both list_lru_init() and register_shrinker() might return an error.
Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Cc: Sherry Yang <sher...@android.com>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Michal Hocko <mho...@suse.com>
---
drivers/android/binder.c | 4 +++-
drivers/android/binder_alloc.c | 12 +---
drivers/android/binder_alloc.h | 2 +-
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 85b0bb4..a54a0f1 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5569,7 +5569,9 @@ static int __init binder_init(void)
struct binder_device *device;
struct hlist_node *tmp;
- binder_alloc_shrinker_init();
+ ret = binder_alloc_shrinker_init();
+ if (ret)
+ return ret;
atomic_set(_transaction_log.cur, ~0U);
atomic_set(_transaction_log_failed.cur, ~0U);
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 0dba2308..fdf9d9f 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -1006,8 +1006,14 @@ void binder_alloc_init(struct binder_alloc *alloc)
INIT_LIST_HEAD(>buffers);
}
-void binder_alloc_shrinker_init(void)
+int binder_alloc_shrinker_init(void)
{
- list_lru_init(_alloc_lru);
- register_shrinker(_shrinker);
+ int ret = list_lru_init(_alloc_lru);
+
+ if (ret == 0) {
+ ret = register_shrinker(_shrinker);
+ if (ret)
+ list_lru_destroy(_alloc_lru);
+ }
+ return ret;
}
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 0b14530..9ef64e5 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -130,7 +130,7 @@ extern struct binder_buffer *binder_alloc_new_buf(struct
binder_alloc *alloc,
size_t extra_buffers_size,
int is_async);
extern void binder_alloc_init(struct binder_alloc *alloc);
-void binder_alloc_shrinker_init(void);
+extern int binder_alloc_shrinker_init(void);
extern void binder_alloc_vma_close(struct binder_alloc *alloc);
extern struct binder_buffer *
binder_alloc_prepare_to_free(struct binder_alloc *alloc,
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: android: ashmem: Check for register_shrinker() failure.
register_shrinker() might return -ENOMEM error since Linux 3.12.
Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Cc: Robert Love <rl...@rlove.org>
Cc: Marco Nelissen <marc...@android.com>
Cc: John Stultz <john.stu...@linaro.org>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: Michal Hocko <mho...@suse.com>
---
drivers/staging/android/ashmem.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 0f695df..ab56f81 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -862,12 +862,18 @@ static int __init ashmem_init(void)
goto out_free2;
}
- register_shrinker(_shrinker);
+ ret = register_shrinker(_shrinker);
+ if (unlikely(ret)) {
+ pr_err("failed to register shrinker\n");
+ goto out_free3;
+ }
pr_info("initialized\n");
return 0;
+out_free3:
+ misc_deregister(_misc);
out_free2:
kmem_cache_destroy(ashmem_range_cachep);
out_free1:
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: lowmemorykiller: remove bogus NULL check
Dan Carpenter wrote:
> The NULL checking here doesn't make sense, so it causes a static checker
> warning. It turns out that p->mm can't be NULL so the inconsistency is
> harmless and we should just remove the check.
Commit 77ed2c5745d9 ("android,lowmemorykiller: Don't abuse TIF_MEMDIE.")
replaced test_tsk_thread_flag(p, TIF_MEMDIE) with task_lmk_waiting(p) && p->mm
because TIF_MEMDIE is cleared after p->mm became NULL whereas
PFA_LMK_WAITING is not cleared after p->mm became NULL.
But p is a thread which is guaranteed to be p->mm != NULL.
>
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
Acked-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
>
> diff --git a/drivers/staging/android/lowmemorykiller.c
> b/drivers/staging/android/lowmemorykiller.c
> index c79f224..24d2745 100644
> --- a/drivers/staging/android/lowmemorykiller.c
> +++ b/drivers/staging/android/lowmemorykiller.c
> @@ -131,7 +131,7 @@ static unsigned long lowmem_scan(struct shrinker *s,
> struct shrink_control *sc)
> if (!p)
> continue;
>
> - if (task_lmk_waiting(p) && p->mm &&
> + if (task_lmk_waiting(p) &&
> time_before_eq(jiffies, lowmem_deathpending_timeout)) {
> task_unlock(p);
> rcu_read_unlock();
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: android,lowmemorykiller: Don't abuse TIF_MEMDIE.
Dan Carpenter wrote:
> On Thu, Apr 07, 2016 at 06:49:58AM +0900, Tetsuo Handa wrote:
> > Dan Carpenter wrote:
> > > Hello Tetsuo Handa,
> >
> > Hello, Dan.
> >
> > >
> > > This is a semi-automatic email about new static checker warnings.
> > >
> > > The patch 77ed2c5745d9: "android,lowmemorykiller: Don't abuse
> > > TIF_MEMDIE." from Mar 8, 2016, leads to the following Smatch
> > > complaint:
> > >
> > > drivers/staging/android/lowmemorykiller.c:145 lowmem_scan()
> > >error: we previously assumed 'p->mm' could be null (see line 134)
> >
> > This is a false positive. find_lock_task_mm() returns a task_struct
> > whose mm is not NULL (with alloc_lock spinlock held).
>
> Yeah. Smatch isn't supposed to warn about this. I'll look at it. But
> we should still remove the unneeded NULL check, right?
>
Indeed, this "p->mm &&" is pointless. You can remove it.
Well,
task_lock(selected);
send_sig(SIGKILL, selected, 0);
if (selected->mm)
task_set_lmk_waiting(selected);
task_unlock(selected);
sets PFA_LMK_WAITING to only one of threads in the victim's thread group, and
if (task_lmk_waiting(p) &&
time_before_eq(jiffies, lowmem_deathpending_timeout)) {
task_unlock(p);
rcu_read_unlock();
return 0;
}
will select next thread in the same victim's thread group as soon as
previous thread in the same victim's thread group released its mm.
Maybe we are calling lowmem_print() more frequently than needed.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: android,lowmemorykiller: Don't abuse TIF_MEMDIE.
Dan Carpenter wrote:
> Hello Tetsuo Handa,
Hello, Dan.
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 77ed2c5745d9: "android,lowmemorykiller: Don't abuse
> TIF_MEMDIE." from Mar 8, 2016, leads to the following Smatch
> complaint:
>
> drivers/staging/android/lowmemorykiller.c:145 lowmem_scan()
>error: we previously assumed 'p->mm' could be null (see line 134)
This is a false positive. find_lock_task_mm() returns a task_struct
whose mm is not NULL (with alloc_lock spinlock held).
>
> drivers/staging/android/lowmemorykiller.c
>133
>134if (task_lmk_waiting(p) && p->mm &&
>^
> Patch adds a new check.
>
>135time_before_eq(jiffies,
> lowmem_deathpending_timeout)) {
>136task_unlock(p);
>137rcu_read_unlock();
>138return 0;
>139}
>140oom_score_adj = p->signal->oom_score_adj;
>141if (oom_score_adj < min_score_adj) {
>142task_unlock(p);
>143continue;
>144}
>145tasksize = get_mm_rss(p->mm);
> ^
> Old unchecked dereference inside function call.
At this point alloc_lock spinlock is still held.
Thus, this mm is not NULL.
>
>146task_unlock(p);
>147if (tasksize <= 0)
>
> regards,
> dan carpenter
>
Thanks.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android,lowmemorykiller: Don't abuse TIF_MEMDIE.
Greg KH wrote: > On Mon, Mar 21, 2016 at 08:00:49PM +0900, Tetsuo Handa wrote: > > Greg Kroah-Hartman wrote: > > > On Tue, Mar 08, 2016 at 03:18:59PM +0100, Michal Hocko wrote: > > > > On Tue 08-03-16 20:01:32, Tetsuo Handa wrote: > > > > > Currently, lowmemorykiller (LMK) is using TIF_MEMDIE for two purposes. > > > > > One is to remember processes killed by LMK, and the other is to > > > > > accelerate termination of processes killed by LMK. > > > > > > > > > > But since LMK is invoked as a memory shrinker function, there still > > > > > should be some memory available. It is very likely that memory > > > > > allocations by processes killed by LMK will succeed without using > > > > > ALLOC_NO_WATERMARKS via TIF_MEMDIE. Even if their allocations cannot > > > > > escape from memory allocation loop unless they use > > > > > ALLOC_NO_WATERMARKS, > > > > > lowmem_deathpending_timeout can guarantee forward progress by choosing > > > > > next victim process. > > > > > > > > > > On the other hand, mark_oom_victim() assumes that it must be called > > > > > with > > > > > oom_lock held and it must not be called after oom_killer_disable() was > > > > > called. But LMK is calling it without holding oom_lock and checking > > > > > oom_killer_disabled. It is possible that LMK calls mark_oom_victim() > > > > > due to allocation requests by kernel threads after current thread > > > > > returned from oom_killer_disabled(). This will break synchronization > > > > > for PM/suspend. > > > > > > > > > > This patch introduces per a task_struct flag for remembering processes > > > > > killed by LMK, and replaces TIF_MEMDIE with that flag. By applying > > > > > this > > > > > patch, assumption by mark_oom_victim() becomes true. > > > > > > > > Thanks for looking into this. A separate flag sounds like a better way > > > > to go (assuming that the flags are not scarce which doesn't seem to be > > > > the case here). > > > > > > > > The LMK cannot kill the frozen tasks now but this shouldn't be a big > > > > deal > > > > because this is not strictly necessary for the system to move on. We are > > > > not OOM. > > > > > > > > > Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp> > > > > > Cc: Michal Hocko <mho...@suse.cz> > > > > > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> > > > > > Cc: Arve Hjonnevag <a...@android.com> > > > > > Cc: Riley Andrews <riandr...@android.com> > > > > > > > > Acked-by: Michal Hocko <mho...@suse.com> > > > > > > So, any objection for me taking this through the staging tree? > > > > > Seems no objection. Please take this through the staging tree. > > Ok, will do so after 4.6-rc1 is out. > I haven't seen this patch in linux-next. Would you take this? Regards. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android,lowmemorykiller: Don't abuse TIF_MEMDIE.
Greg Kroah-Hartman wrote: > On Tue, Mar 08, 2016 at 03:18:59PM +0100, Michal Hocko wrote: > > On Tue 08-03-16 20:01:32, Tetsuo Handa wrote: > > > Currently, lowmemorykiller (LMK) is using TIF_MEMDIE for two purposes. > > > One is to remember processes killed by LMK, and the other is to > > > accelerate termination of processes killed by LMK. > > > > > > But since LMK is invoked as a memory shrinker function, there still > > > should be some memory available. It is very likely that memory > > > allocations by processes killed by LMK will succeed without using > > > ALLOC_NO_WATERMARKS via TIF_MEMDIE. Even if their allocations cannot > > > escape from memory allocation loop unless they use ALLOC_NO_WATERMARKS, > > > lowmem_deathpending_timeout can guarantee forward progress by choosing > > > next victim process. > > > > > > On the other hand, mark_oom_victim() assumes that it must be called with > > > oom_lock held and it must not be called after oom_killer_disable() was > > > called. But LMK is calling it without holding oom_lock and checking > > > oom_killer_disabled. It is possible that LMK calls mark_oom_victim() > > > due to allocation requests by kernel threads after current thread > > > returned from oom_killer_disabled(). This will break synchronization > > > for PM/suspend. > > > > > > This patch introduces per a task_struct flag for remembering processes > > > killed by LMK, and replaces TIF_MEMDIE with that flag. By applying this > > > patch, assumption by mark_oom_victim() becomes true. > > > > Thanks for looking into this. A separate flag sounds like a better way > > to go (assuming that the flags are not scarce which doesn't seem to be > > the case here). > > > > The LMK cannot kill the frozen tasks now but this shouldn't be a big deal > > because this is not strictly necessary for the system to move on. We are > > not OOM. > > > > > Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp> > > > Cc: Michal Hocko <mho...@suse.cz> > > > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> > > > Cc: Arve Hjonnevag <a...@android.com> > > > Cc: Riley Andrews <riandr...@android.com> > > > > Acked-by: Michal Hocko <mho...@suse.com> > > So, any objection for me taking this through the staging tree? > Seems no objection. Please take this through the staging tree. Regards. > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] android,lowmemorykiller: Don't abuse TIF_MEMDIE.
Currently, lowmemorykiller (LMK) is using TIF_MEMDIE for two purposes.
One is to remember processes killed by LMK, and the other is to
accelerate termination of processes killed by LMK.
But since LMK is invoked as a memory shrinker function, there still
should be some memory available. It is very likely that memory
allocations by processes killed by LMK will succeed without using
ALLOC_NO_WATERMARKS via TIF_MEMDIE. Even if their allocations cannot
escape from memory allocation loop unless they use ALLOC_NO_WATERMARKS,
lowmem_deathpending_timeout can guarantee forward progress by choosing
next victim process.
On the other hand, mark_oom_victim() assumes that it must be called with
oom_lock held and it must not be called after oom_killer_disable() was
called. But LMK is calling it without holding oom_lock and checking
oom_killer_disabled. It is possible that LMK calls mark_oom_victim()
due to allocation requests by kernel threads after current thread
returned from oom_killer_disabled(). This will break synchronization
for PM/suspend.
This patch introduces per a task_struct flag for remembering processes
killed by LMK, and replaces TIF_MEMDIE with that flag. By applying this
patch, assumption by mark_oom_victim() becomes true.
Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Cc: Michal Hocko <mho...@suse.cz>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Arve Hjonnevag <a...@android.com>
Cc: Riley Andrews <riandr...@android.com>
---
drivers/staging/android/lowmemorykiller.c | 9 ++---
include/linux/sched.h | 4
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/lowmemorykiller.c
b/drivers/staging/android/lowmemorykiller.c
index 4b8a56c..a1dd798 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -129,7 +129,7 @@ static unsigned long lowmem_scan(struct shrinker *s, struct
shrink_control *sc)
if (!p)
continue;
- if (test_tsk_thread_flag(p, TIF_MEMDIE) &&
+ if (task_lmk_waiting(p) && p->mm &&
time_before_eq(jiffies, lowmem_deathpending_timeout)) {
task_unlock(p);
rcu_read_unlock();
@@ -160,13 +160,8 @@ static unsigned long lowmem_scan(struct shrinker *s,
struct shrink_control *sc)
if (selected) {
task_lock(selected);
send_sig(SIGKILL, selected, 0);
- /*
-* FIXME: lowmemorykiller shouldn't abuse global OOM killer
-* infrastructure. There is no real reason why the selected
-* task should have access to the memory reserves.
-*/
if (selected->mm)
- mark_oom_victim(selected);
+ task_set_lmk_waiting(selected);
task_unlock(selected);
lowmem_print(1, "Killing '%s' (%d), adj %hd,\n"
" to free %ldkB on behalf of '%s' (%d)
because\n"
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 0b44fbc..de9ced9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2187,6 +2187,7 @@ static inline void memalloc_noio_restore(unsigned int
flags)
#define PFA_NO_NEW_PRIVS 0 /* May not gain new privileges. */
#define PFA_SPREAD_PAGE 1 /* Spread page cache over cpuset */
#define PFA_SPREAD_SLAB 2 /* Spread some slab caches over cpuset */
+#define PFA_LMK_WAITING 3 /* Lowmemorykiller is waiting */
#define TASK_PFA_TEST(name, func) \
@@ -2210,6 +2211,9 @@ TASK_PFA_TEST(SPREAD_SLAB, spread_slab)
TASK_PFA_SET(SPREAD_SLAB, spread_slab)
TASK_PFA_CLEAR(SPREAD_SLAB, spread_slab)
+TASK_PFA_TEST(LMK_WAITING, lmk_waiting)
+TASK_PFA_SET(LMK_WAITING, lmk_waiting)
+
/*
* task->jobctl flags
*/
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.
Greg KH wrote:
> On Fri, Sep 04, 2015 at 04:05:59PM +0200, Michal Hocko wrote:
> > On Wed 02-09-15 18:06:20, Greg KH wrote:
> > [...]
> > > And if we aren't taking patch 1/2, I guess this one isn't needed either?
> >
> > Unlike the patch1 which was pretty much cosmetic this fixes a real
> > issue.
>
> Ok, then it would be great to get this in a format that I can apply it
> in :)
I see. Here is a minimal patch.
(Acked-by: from http://lkml.kernel.org/r/20150827084443.ge14...@dhcp22.suse.cz )
>From 118609fa25700af11791b1b7e8349f8973a9e7e4 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Date: Sat, 5 Sep 2015 02:58:12 +0900
Subject: [PATCH] android, lmk: Send SIGKILL before setting TIF_MEMDIE.
It was observed that setting TIF_MEMDIE before sending SIGKILL at
oom_kill_process() allows memory reserves to be depleted by allocations
which are not needed for terminating the OOM victim.
This patch reverts commit 6bc2b856bb7c ("staging: android: lowmemorykiller:
set TIF_MEMDIE before send kill sig"), for oom_kill_process() was updated
to send SIGKILL before setting TIF_MEMDIE.
Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Acked-by: Michal Hocko <mho...@suse.com>
---
drivers/staging/android/lowmemorykiller.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/android/lowmemorykiller.c
b/drivers/staging/android/lowmemorykiller.c
index 872bd60..569d12c 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -157,26 +157,22 @@ static unsigned long lowmem_scan(struct shrinker *s,
struct shrink_control *sc)
}
if (selected) {
task_lock(selected);
- if (!selected->mm) {
- /* Already exited, cannot do mark_tsk_oom_victim() */
- task_unlock(selected);
- goto out;
- }
+ send_sig(SIGKILL, selected, 0);
/*
* FIXME: lowmemorykiller shouldn't abuse global OOM killer
* infrastructure. There is no real reason why the selected
* task should have access to the memory reserves.
*/
- mark_oom_victim(selected);
+ if (selected->mm)
+ mark_oom_victim(selected);
task_unlock(selected);
lowmem_print(1, "send sigkill to %d (%s), adj %hd, size %d\n",
selected->pid, selected->comm,
selected_oom_score_adj, selected_tasksize);
lowmem_deathpending_timeout = jiffies + HZ;
- send_sig(SIGKILL, selected, 0);
rem += selected_tasksize;
}
-out:
+
lowmem_print(4, "lowmem_scan %lu, %x, return %lu\n",
sc->nr_to_scan, sc->gfp_mask, rem);
rcu_read_unlock();
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.
Hello.
Should selected_tasksize be added to rem even when TIF_MEMDIE was not set?
Please see a thread from http://www.spinics.net/lists/linux-mm/msg93246.html
if you want to know why to reverse the order.
From 2d4cc11d8128e4c1397631b91fea78da3eaefb47 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Wed, 26 Aug 2015 20:52:39 +0900
Subject: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and
sending SIGKILL.
If we set TIF_MEMDIE before sending SIGKILL, memory reserves could be
spent for allocations which are not needed for terminating the victim.
Reverse the order as with oom_kill_process() does.
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
drivers/staging/android/lowmemorykiller.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/android/lowmemorykiller.c
b/drivers/staging/android/lowmemorykiller.c
index d5d25e4..c39b6a2 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -156,26 +156,24 @@ next:
}
if (selected) {
task_lock(selected);
- if (!selected-mm) {
- /* Already exited, cannot do mark_tsk_oom_victim() */
- task_unlock(selected);
- goto out;
- }
+ lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
+selected-pid, selected-comm,
+selected_oom_score_adj, selected_tasksize);
+ task_unlock(selected);
+ send_sig(SIGKILL, selected, 0);
/*
* FIXME: lowmemorykiller shouldn't abuse global OOM killer
* infrastructure. There is no real reason why the selected
* task should have access to the memory reserves.
*/
- mark_oom_victim(selected);
- lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
-selected-pid, selected-comm,
-selected_oom_score_adj, selected_tasksize);
+ task_lock(selected);
+ if (selected-mm) {
+ mark_oom_victim(selected);
+ lowmem_deathpending_timeout = jiffies + HZ;
+ rem += selected_tasksize;
+ }
task_unlock(selected);
- lowmem_deathpending_timeout = jiffies + HZ;
- send_sig(SIGKILL, selected, 0);
- rem += selected_tasksize;
}
-out:
lowmem_print(4, lowmem_scan %lu, %x, return %lu\n,
sc-nr_to_scan, sc-gfp_mask, rem);
rcu_read_unlock();
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] android, lmk: Protect task-comm with task_lock.
Hello.
Next patch is mm-related but this patch is not.
Via which tree should these patches go?
From 48c1b457eb32d7a029e9a078ee0a67974ada9261 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Wed, 26 Aug 2015 20:49:17 +0900
Subject: [PATCH 1/2] android, lmk: Protect task-comm with task_lock.
Passing task-comm to printk() wants task_lock() protection in order
to avoid potentially emitting garbage bytes.
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
drivers/staging/android/lowmemorykiller.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/android/lowmemorykiller.c
b/drivers/staging/android/lowmemorykiller.c
index 872bd60..d5d25e4 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -134,26 +134,25 @@ static unsigned long lowmem_scan(struct shrinker *s,
struct shrink_control *sc)
return 0;
}
oom_score_adj = p-signal-oom_score_adj;
- if (oom_score_adj min_score_adj) {
- task_unlock(p);
- continue;
- }
+ if (oom_score_adj min_score_adj)
+ goto next;
tasksize = get_mm_rss(p-mm);
- task_unlock(p);
if (tasksize = 0)
- continue;
+ goto next;
if (selected) {
if (oom_score_adj selected_oom_score_adj)
- continue;
+ goto next;
if (oom_score_adj == selected_oom_score_adj
tasksize = selected_tasksize)
- continue;
+ goto next;
}
selected = p;
selected_tasksize = tasksize;
selected_oom_score_adj = oom_score_adj;
lowmem_print(2, select %d (%s), adj %hd, size %d, to kill\n,
p-pid, p-comm, oom_score_adj, tasksize);
+next:
+ task_unlock(p);
}
if (selected) {
task_lock(selected);
@@ -168,10 +167,10 @@ static unsigned long lowmem_scan(struct shrinker *s,
struct shrink_control *sc)
* task should have access to the memory reserves.
*/
mark_oom_victim(selected);
- task_unlock(selected);
lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
selected-pid, selected-comm,
selected_oom_score_adj, selected_tasksize);
+ task_unlock(selected);
lowmem_deathpending_timeout = jiffies + HZ;
send_sig(SIGKILL, selected, 0);
rem += selected_tasksize;
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] android, lmk: Reverse the order of setting TIF_MEMDIE and sending SIGKILL.
Reposting updated version as it turned out that we can call do_send_sig_info()
with task_lock held. ;-) ( http://marc.info/?l=linux-mmm=144059948628905w=2 )
Tetsuo Handa wrote:
Should selected_tasksize be added to rem even when TIF_MEMDIE was not set?
Commit e1099a69a624 android, lmk: avoid setting TIF_MEMDIE if process
has already exited changed not to add selected_tasksize to rem. But I
noticed that rem is initialized to 0 and there is only one addition
(i.e. rem += selected_tasksize means rem = selected_tasksize) since
commit 7dc19d5affd7 drivers: convert shrinkers to new count/scan API.
I don't know what values we should return, but this patch restores
pre commit e1099a69a624 because omitting a call to mark_oom_victim()
due to race will not prevent from reclaiming memory because we already
sent SIGKILL.
From b7075abd3a1e903e88f1755c68adc017d2125b0d Mon Sep 17 00:00:00 2001
From: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
Date: Thu, 27 Aug 2015 00:13:57 +0900
Subject: [PATCH 2/2] android, lmk: Send SIGKILL before setting TIF_MEMDIE.
It was observed that setting TIF_MEMDIE before sending SIGKILL at
oom_kill_process() allows memory reserves to be depleted by allocations
which are not needed for terminating the OOM victim.
This patch reverts commit 6bc2b856bb7c (staging: android: lowmemorykiller:
set TIF_MEMDIE before send kill sig), for oom_kill_process() was updated
to send SIGKILL before setting TIF_MEMDIE.
Signed-off-by: Tetsuo Handa penguin-ker...@i-love.sakura.ne.jp
---
drivers/staging/android/lowmemorykiller.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/android/lowmemorykiller.c
b/drivers/staging/android/lowmemorykiller.c
index d5d25e4..af604cf 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -156,26 +156,21 @@ next:
}
if (selected) {
task_lock(selected);
- if (!selected-mm) {
- /* Already exited, cannot do mark_tsk_oom_victim() */
- task_unlock(selected);
- goto out;
- }
+ lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
+selected-pid, selected-comm,
+selected_oom_score_adj, selected_tasksize);
+ send_sig(SIGKILL, selected, 0);
/*
* FIXME: lowmemorykiller shouldn't abuse global OOM killer
* infrastructure. There is no real reason why the selected
* task should have access to the memory reserves.
*/
- mark_oom_victim(selected);
- lowmem_print(1, send sigkill to %d (%s), adj %hd, size %d\n,
-selected-pid, selected-comm,
-selected_oom_score_adj, selected_tasksize);
+ if (selected-mm)
+ mark_oom_victim(selected);
task_unlock(selected);
lowmem_deathpending_timeout = jiffies + HZ;
- send_sig(SIGKILL, selected, 0);
rem += selected_tasksize;
}
-out:
lowmem_print(4, lowmem_scan %lu, %x, return %lu\n,
sc-nr_to_scan, sc-gfp_mask, rem);
rcu_read_unlock();
--
1.8.3.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
