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

[Vinayak Menon]

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

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

[Greg KH]

On Mon, May 07, 2018 at 01:22:36PM +0530, Vinayak Menon wrote:
> 
> 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.

All was dropped, so no worries anymore :)

thanks,

greg k-h