[PATCH] mm: kmemleak: wait for scan completion before disabling free

[PATCH] mm: kmemleak: wait for scan completion before disabling free

[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

Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free

[Catalin Marinas]

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.

Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free

[Andrew Morton]

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
_

Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free

[Andrew Morton]

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
> _
> 

Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free

[Vinayak Menon]

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
> _
>

Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free

[Catalin Marinas]

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

Re: [PATCH] mm: kmemleak: wait for scan completion before disabling free

[Vinayak Menon]

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.
>