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

[Catalin Marinas <catalin.marinas@arm.com>]

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