Re: Patch "mm/kmemleak.c: wait for scan completion before disabling free" has been added to the 3.18-stable tree

* Re: Patch "mm/kmemleak.c: wait for scan completion before disabling free" has been added to the 3.18-stable tree
       [not found] <152529381722755@kroah.com>
@ 2018-05-07  7:52 ` Vinayak Menon
  2018-05-08  7:20   ` Greg KH
  0 siblings, 1 reply; 2+ messages in thread
From: Vinayak Menon @ 2018-05-07  7:52 UTC (permalink / raw)
  To: gregkh, stable; +Cc: akpm, alexander.levin, catalin.marinas, torvalds

On 5/3/2018 2:13 AM, gregkh@linuxfoundation.org wrote:
> This is a note to let you know that I've just added the patch titled
>
>     mm/kmemleak.c: wait for scan completion before disabling free
>
> to the 3.18-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
>      mm-kmemleak.c-wait-for-scan-completion-before-disabling-free.patch
> and it can be found in the queue-3.18 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
>
>
> From foo@baz Wed May  2 13:21:44 PDT 2018
> From: Vinayak Menon <vinmenon@codeaurora.org>
> Date: Wed, 28 Mar 2018 16:01:16 -0700
> Subject: mm/kmemleak.c: wait for scan completion before disabling free
>
> From: Vinayak Menon <vinmenon@codeaurora.org>
>
> [ Upstream commit 914b6dfff790544d9b77dfd1723adb3745ec9700 ]
>
> 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, and while at it
> fix the comment on stop_scan_thread.
>
> [vinmenon@codeaurora.org: fix stop_scan_thread comment]
>   Link: http://lkml.kernel.org/r/1522219972-22809-1-git-send-email-vinmenon@codeaurora.org
> Link: http://lkml.kernel.org/r/1522063429-18992-1-git-send-email-vinmenon@codeaurora.org
> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  mm/kmemleak.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -1481,8 +1481,7 @@ static void start_scan_thread(void)
>  }
>  
>  /*
> - * Stop the automatic memory scanning thread. This function must be called
> - * with the scan_mutex held.
> + * Stop the automatic memory scanning thread.
>   */
>  static void stop_scan_thread(void)
>  {
> @@ -1746,12 +1745,15 @@ static void kmemleak_do_cleanup(struct w
>  	mutex_lock(&scan_mutex);
>  	stop_scan_thread();
>  
> +	mutex_lock(&scan_mutex);

The lock is taken once aboveï¿½ stop_scan_thread() and can result in a deadlock.

The lock was removed from kmemleak_do_cleanup by following commit which is not present on 3.18

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()

So we may have to pick the above patch too.

>  	/*
> -	 * 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();
>
>
> Patches currently in stable-queue which might be from vinmenon@codeaurora.org are
>
> queue-3.18/mm-kmemleak.c-wait-for-scan-completion-before-disabling-free.patch

^ permalink raw reply	[flat|nested] 2+ messages in thread