* [PATCH] mm: kmemleak: wait for scan completion before disabling free
@ 2018-03-26 11:23 Vinayak Menon
2018-03-26 15:44 ` Catalin Marinas
0 siblings, 1 reply; 7+ messages in thread
From: Vinayak Menon @ 2018-03-26 11:23 UTC (permalink / raw)
To: catalin.marinas; +Cc: linux-mm, Vinayak Menon
A crash is observed when kmemleak_scan accesses the
object->pointer, likely due to the following race.
TASK A TASK B TASK C
kmemleak_write
(with "scan" and
NOT "scan=on")
kmemleak_scan()
create_object
kmem_cache_alloc fails
kmemleak_disable
kmemleak_do_cleanup
kmemleak_free_enabled = 0
kfree
kmemleak_free bails out
(kmemleak_free_enabled is 0)
slub frees object->pointer
update_checksum
crash - object->pointer
freed (DEBUG_PAGEALLOC)
kmemleak_do_cleanup waits for the scan thread to complete, but not for
direct call to kmemleak_scan via kmemleak_write. So add a wait for
kmemleak_scan completion before disabling kmemleak_free.
Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
---
mm/kmemleak.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 8b9afc5..aa9c84b 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1931,12 +1931,15 @@ static void kmemleak_do_cleanup(struct work_struct *work)
{
stop_scan_thread();
+ mutex_lock(&scan_mutex);
/*
- * Once the scan thread has stopped, it is safe to no longer track
- * object freeing. Ordering of the scan thread stopping and the memory
- * accesses below is guaranteed by the kthread_stop() function.
+ * Once it is made sure that kmemleak_scan has stopped, it is safe to no
+ * longer track object freeing. Ordering of the scan thread stopping and
+ * the memory accesses below is guaranteed by the kthread_stop()
+ * function.
*/
kmemleak_free_enabled = 0;
+ mutex_unlock(&scan_mutex);
if (!kmemleak_found_leaks)
__kmemleak_do_cleanup();
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free
2018-03-26 11:23 [PATCH] mm: kmemleak: wait for scan completion before disabling free Vinayak Menon
@ 2018-03-26 15:44 ` Catalin Marinas
2018-03-26 19:26 ` Andrew Morton
0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2018-03-26 15:44 UTC (permalink / raw)
To: Vinayak Menon; +Cc: linux-mm, Andrew Morton
On Mon, Mar 26, 2018 at 04:53:49PM +0530, Vinayak Menon wrote:
> A crash is observed when kmemleak_scan accesses the
> object->pointer, likely due to the following race.
>
> TASK A TASK B TASK C
> kmemleak_write
> (with "scan" and
> NOT "scan=on")
> kmemleak_scan()
> create_object
> kmem_cache_alloc fails
> kmemleak_disable
> kmemleak_do_cleanup
> kmemleak_free_enabled = 0
> kfree
> kmemleak_free bails out
> (kmemleak_free_enabled is 0)
> slub frees object->pointer
> update_checksum
> crash - object->pointer
> freed (DEBUG_PAGEALLOC)
>
> kmemleak_do_cleanup waits for the scan thread to complete, but not for
> direct call to kmemleak_scan via kmemleak_write. So add a wait for
> kmemleak_scan completion before disabling kmemleak_free.
>
> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
It looks fine to me. Maybe Andrew can pick it up.
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free
2018-03-26 15:44 ` Catalin Marinas
@ 2018-03-26 19:26 ` Andrew Morton
2018-03-26 19:27 ` Andrew Morton
2018-03-27 5:29 ` Vinayak Menon
0 siblings, 2 replies; 7+ messages in thread
From: Andrew Morton @ 2018-03-26 19:26 UTC (permalink / raw)
To: Catalin Marinas; +Cc: Vinayak Menon, linux-mm
On Mon, 26 Mar 2018 16:44:21 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Mon, Mar 26, 2018 at 04:53:49PM +0530, Vinayak Menon wrote:
> > A crash is observed when kmemleak_scan accesses the
> > object->pointer, likely due to the following race.
> >
> > TASK A TASK B TASK C
> > kmemleak_write
> > (with "scan" and
> > NOT "scan=on")
> > kmemleak_scan()
> > create_object
> > kmem_cache_alloc fails
> > kmemleak_disable
> > kmemleak_do_cleanup
> > kmemleak_free_enabled = 0
> > kfree
> > kmemleak_free bails out
> > (kmemleak_free_enabled is 0)
> > slub frees object->pointer
> > update_checksum
> > crash - object->pointer
> > freed (DEBUG_PAGEALLOC)
> >
> > kmemleak_do_cleanup waits for the scan thread to complete, but not for
> > direct call to kmemleak_scan via kmemleak_write. So add a wait for
> > kmemleak_scan completion before disabling kmemleak_free.
> >
> > Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
>
> It looks fine to me. Maybe Andrew can pick it up.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Well, the comment says:
/*
* Stop the automatic memory scanning thread. This function must be called
* with the scan_mutex held.
*/
static void stop_scan_thread(void)
So shouldn't we do it this way?
--- a/mm/kmemleak.c~mm-kmemleak-wait-for-scan-completion-before-disabling-free-fix
+++ a/mm/kmemleak.c
@@ -1919,9 +1919,9 @@ static void __kmemleak_do_cleanup(void)
*/
static void kmemleak_do_cleanup(struct work_struct *work)
{
+ mutex_lock(&scan_mutex);
stop_scan_thread();
- mutex_lock(&scan_mutex);
/*
* Once it is made sure that kmemleak_scan has stopped, it is safe to no
* longer track object freeing. Ordering of the scan thread stopping and
_
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free
2018-03-26 19:26 ` Andrew Morton
@ 2018-03-26 19:27 ` Andrew Morton
2018-03-27 5:29 ` Vinayak Menon
1 sibling, 0 replies; 7+ messages in thread
From: Andrew Morton @ 2018-03-26 19:27 UTC (permalink / raw)
To: Catalin Marinas, Vinayak Menon, linux-mm
On Mon, 26 Mar 2018 12:26:11 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > It looks fine to me. Maybe Andrew can pick it up.
> >
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
> Well, the comment says:
>
> /*
> * Stop the automatic memory scanning thread. This function must be called
> * with the scan_mutex held.
> */
> static void stop_scan_thread(void)
>
>
> So shouldn't we do it this way?
If "yes" then could someone please runtime test this?
> --- a/mm/kmemleak.c~mm-kmemleak-wait-for-scan-completion-before-disabling-free-fix
> +++ a/mm/kmemleak.c
> @@ -1919,9 +1919,9 @@ static void __kmemleak_do_cleanup(void)
> */
> static void kmemleak_do_cleanup(struct work_struct *work)
> {
> + mutex_lock(&scan_mutex);
> stop_scan_thread();
>
> - mutex_lock(&scan_mutex);
> /*
> * Once it is made sure that kmemleak_scan has stopped, it is safe to no
> * longer track object freeing. Ordering of the scan thread stopping and
> _
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free
2018-03-26 19:26 ` Andrew Morton
2018-03-26 19:27 ` Andrew Morton
@ 2018-03-27 5:29 ` Vinayak Menon
2018-03-27 17:49 ` Catalin Marinas
1 sibling, 1 reply; 7+ messages in thread
From: Vinayak Menon @ 2018-03-27 5:29 UTC (permalink / raw)
To: Andrew Morton, Catalin Marinas; +Cc: linux-mm
On 3/27/2018 12:56 AM, Andrew Morton wrote:
> On Mon, 26 Mar 2018 16:44:21 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
>
>> On Mon, Mar 26, 2018 at 04:53:49PM +0530, Vinayak Menon wrote:
>>> A crash is observed when kmemleak_scan accesses the
>>> object->pointer, likely due to the following race.
>>>
>>> TASK A TASK B TASK C
>>> kmemleak_write
>>> (with "scan" and
>>> NOT "scan=on")
>>> kmemleak_scan()
>>> create_object
>>> kmem_cache_alloc fails
>>> kmemleak_disable
>>> kmemleak_do_cleanup
>>> kmemleak_free_enabled = 0
>>> kfree
>>> kmemleak_free bails out
>>> (kmemleak_free_enabled is 0)
>>> slub frees object->pointer
>>> update_checksum
>>> crash - object->pointer
>>> freed (DEBUG_PAGEALLOC)
>>>
>>> kmemleak_do_cleanup waits for the scan thread to complete, but not for
>>> direct call to kmemleak_scan via kmemleak_write. So add a wait for
>>> kmemleak_scan completion before disabling kmemleak_free.
>>>
>>> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
>> It looks fine to me. Maybe Andrew can pick it up.
>>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Well, the comment says:
>
> /*
> * Stop the automatic memory scanning thread. This function must be called
> * with the scan_mutex held.
> */
> static void stop_scan_thread(void)
>
>
> So shouldn't we do it this way?
Earlier it was done the way you mentioned. But that was changed to fix a deadlock by
commit 5f369f374ba4889fe3c17883402db5ee8d254216
Author: Catalin Marinas <catalin.marinas@arm.com>
Date:A A Wed Jun 24 16:58:31 2015 -0700
A A A mm: kmemleak: do not acquire scan_mutex in kmemleak_do_cleanup()
Not able to see a reason why stop_scan_thread must be called with scan_mutex held. The comment needs a fix ?
>
> --- a/mm/kmemleak.c~mm-kmemleak-wait-for-scan-completion-before-disabling-free-fix
> +++ a/mm/kmemleak.c
> @@ -1919,9 +1919,9 @@ static void __kmemleak_do_cleanup(void)
> */
> static void kmemleak_do_cleanup(struct work_struct *work)
> {
> + mutex_lock(&scan_mutex);
> stop_scan_thread();
>
> - mutex_lock(&scan_mutex);
> /*
> * Once it is made sure that kmemleak_scan has stopped, it is safe to no
> * longer track object freeing. Ordering of the scan thread stopping and
> _
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free
2018-03-27 5:29 ` Vinayak Menon
@ 2018-03-27 17:49 ` Catalin Marinas
2018-03-28 6:51 ` Vinayak Menon
0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2018-03-27 17:49 UTC (permalink / raw)
To: Vinayak Menon; +Cc: Andrew Morton, linux-mm
On Tue, Mar 27, 2018 at 10:59:31AM +0530, Vinayak Menon wrote:
> On 3/27/2018 12:56 AM, Andrew Morton wrote:
> > On Mon, 26 Mar 2018 16:44:21 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> >> On Mon, Mar 26, 2018 at 04:53:49PM +0530, Vinayak Menon wrote:
> >>> A crash is observed when kmemleak_scan accesses the
> >>> object->pointer, likely due to the following race.
> >>>
> >>> TASK A TASK B TASK C
> >>> kmemleak_write
> >>> (with "scan" and
> >>> NOT "scan=on")
> >>> kmemleak_scan()
> >>> create_object
> >>> kmem_cache_alloc fails
> >>> kmemleak_disable
> >>> kmemleak_do_cleanup
> >>> kmemleak_free_enabled = 0
> >>> kfree
> >>> kmemleak_free bails out
> >>> (kmemleak_free_enabled is 0)
> >>> slub frees object->pointer
> >>> update_checksum
> >>> crash - object->pointer
> >>> freed (DEBUG_PAGEALLOC)
> >>>
> >>> kmemleak_do_cleanup waits for the scan thread to complete, but not for
> >>> direct call to kmemleak_scan via kmemleak_write. So add a wait for
> >>> kmemleak_scan completion before disabling kmemleak_free.
> >>>
> >>> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
> >> It looks fine to me. Maybe Andrew can pick it up.
> >>
> >> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > Well, the comment says:
> >
> > /*
> > * Stop the automatic memory scanning thread. This function must be called
> > * with the scan_mutex held.
> > */
> > static void stop_scan_thread(void)
> >
> >
> > So shouldn't we do it this way?
>
> Earlier it was done the way you mentioned. But that was changed to fix
> a deadlock by
>
> commit 5f369f374ba4889fe3c17883402db5ee8d254216
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date: Wed Jun 24 16:58:31 2015 -0700
>
> mm: kmemleak: do not acquire scan_mutex in kmemleak_do_cleanup()
>
> Not able to see a reason why stop_scan_thread must be called with
> scan_mutex held. The comment needs a fix ?
Indeed, the comment needs fixing as waiting on the mutex here may lead
deadlock. Would you mind sending an updated patch? Feel free to keep my
reviewed-by tag.
Thanks.
--
Catalin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free
2018-03-27 17:49 ` Catalin Marinas
@ 2018-03-28 6:51 ` Vinayak Menon
0 siblings, 0 replies; 7+ messages in thread
From: Vinayak Menon @ 2018-03-28 6:51 UTC (permalink / raw)
To: Catalin Marinas; +Cc: Andrew Morton, linux-mm
On 3/27/2018 11:19 PM, Catalin Marinas wrote:
> On Tue, Mar 27, 2018 at 10:59:31AM +0530, Vinayak Menon wrote:
>> On 3/27/2018 12:56 AM, Andrew Morton wrote:
>>> On Mon, 26 Mar 2018 16:44:21 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
>>>> On Mon, Mar 26, 2018 at 04:53:49PM +0530, Vinayak Menon wrote:
>>>>> A crash is observed when kmemleak_scan accesses the
>>>>> object->pointer, likely due to the following race.
>>>>>
>>>>> TASK A TASK B TASK C
>>>>> kmemleak_write
>>>>> (with "scan" and
>>>>> NOT "scan=on")
>>>>> kmemleak_scan()
>>>>> create_object
>>>>> kmem_cache_alloc fails
>>>>> kmemleak_disable
>>>>> kmemleak_do_cleanup
>>>>> kmemleak_free_enabled = 0
>>>>> kfree
>>>>> kmemleak_free bails out
>>>>> (kmemleak_free_enabled is 0)
>>>>> slub frees object->pointer
>>>>> update_checksum
>>>>> crash - object->pointer
>>>>> freed (DEBUG_PAGEALLOC)
>>>>>
>>>>> kmemleak_do_cleanup waits for the scan thread to complete, but not for
>>>>> direct call to kmemleak_scan via kmemleak_write. So add a wait for
>>>>> kmemleak_scan completion before disabling kmemleak_free.
>>>>>
>>>>> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
>>>> It looks fine to me. Maybe Andrew can pick it up.
>>>>
>>>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>> Well, the comment says:
>>>
>>> /*
>>> * Stop the automatic memory scanning thread. This function must be called
>>> * with the scan_mutex held.
>>> */
>>> static void stop_scan_thread(void)
>>>
>>>
>>> So shouldn't we do it this way?
>> Earlier it was done the way you mentioned. But that was changed to fix
>> a deadlock by
>>
>> commit 5f369f374ba4889fe3c17883402db5ee8d254216
>> Author: Catalin Marinas <catalin.marinas@arm.com>
>> Date:A A Wed Jun 24 16:58:31 2015 -0700
>>
>> A A A mm: kmemleak: do not acquire scan_mutex in kmemleak_do_cleanup()
>>
>> Not able to see a reason why stop_scan_thread must be called with
>> scan_mutex held. The comment needs a fix ?
> Indeed, the comment needs fixing as waiting on the mutex here may lead
> deadlock. Would you mind sending an updated patch? Feel free to keep my
> reviewed-by tag.
Sure. done.
>
> Thanks.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-28 6:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-26 11:23 [PATCH] mm: kmemleak: wait for scan completion before disabling free Vinayak Menon
2018-03-26 15:44 ` Catalin Marinas
2018-03-26 19:26 ` Andrew Morton
2018-03-26 19:27 ` Andrew Morton
2018-03-27 5:29 ` Vinayak Menon
2018-03-27 17:49 ` Catalin Marinas
2018-03-28 6:51 ` Vinayak Menon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.