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

On Tue, Mar 27, 2018 at 10:59:31AM +0530, Vinayak Menon wrote:

On 3/27/2018 12:56 AM, Andrew Morton wrote:

quoted

On Mon, 26 Mar 2018 16:44:21 +0100 Catalin Marinas [off-list ref] wrote:

quoted

On Mon, Mar 26, 2018 at 04:53:49PM +0530, Vinayak Menon wrote:

quoted

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

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 [off-list ref]
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