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

[Vinayak Menon <vinmenon@codeaurora.org>]

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@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:A A  Wed Jun 24 16:58:31 2015 -0700
>>
>> A A A  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.
>