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@xxxxxxx> 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@xxxxxxxxxxxxxx> >>>> It looks fine to me. Maybe Andrew can pick it up. >>>> >>>> Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx> >>> 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@xxxxxxx> >> 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. Sure. done. > > Thanks. >
