Re: [PATCH 2/2] mm/kmemleak: No need to check kmemleak_initialized in set_track_prepare()

[Vlastimil Babka]

On 8/11/23 04:03, wang xiaolei wrote:
> 
> On 8/10/23 9:16 PM, Vlastimil Babka wrote:
>> CAUTION: This email comes from a non Wind River email account!
>> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>
>> On 8/10/23 12:03, Vlastimil Babka wrote:
>>> On 8/10/23 09:47, Xiaolei Wang wrote:
>>>> The kmemleak_late_init() is defined as a late_initcall. The current
>>>> implementation of set_track_prepare() depends on the kmemleak init.
>>>> That also means there is no call trace for the memory leak which object
>>>> is created before the kmemleak_late_init().
>>> So if I understand correctly, we have the following sequence of events durin
>>> boot
>>>
>>> ...
>>> A: stack_depot is initialized
>>> ...
>>> B: kmemleak is initialized
>>> ...
>>>
>>> before this patchset, we can miss allocations before B, aftewards only
>>> before A (which can't be helped), so we now have between A and B.
>>>
>>> That's nice, but it's weird that can record kmemleak when
>>> !kmemleak_initialized. Why can't it be initialized sooner in that case?
>> Looking closer, I think what you want could be achieved by kmemleak_init()
>> setting a variable that is checked in kmemleak_initialized() instead of the
>> kmemleak_initialized that's set too late.
>>
>> I think this should work because:
>> - I assume kmemleak can't record anything before kmemleak_init()
>> - stack depot early init is requested one way or the other
>> - mm_core_init() calls stack_depot_early_init() before kmemleak_init()
>>
>> But I also wonder how kmemleak can even reach set_track_prepare() before
>> kmemleak_init(), maybe that's the issue?
> 
> Before kmemleak_init, many places also need to allocate kmemleak_object,
> 
> and also need to save stack in advance, but kmemleak_object is allocated
> 
> in the form of an array, after kmemleak_init 'object_cache = 
> KMEM_CACHE(kmemleak_object, SLAB_NOLEAKTRACE);'

Hm I see, kmemleak has this static mempool so it really can record some
objects very early.

> I think there is still some memory not recorded on the backtrace before
> 
> stack_depot_early_init(), does anyone have a better suggestion?

No we can't record the backtrace earlier. But I don't think it's a problem
in practice. AFAIU kmemleak needs to record these very early allocations so
if they point to further objects, those are not suspected as orphans. But
the early allocations themselves also are very unlikely to be leaks, so does
it really matter that we don't have a backtrace for their allocation?
Because the backtrace is the only thing that's missing - the object is
otherwise recorded even if set_track_prepare() returns 0.

> thanks
> 
> xiaolei
> 
>>
>>>> In a previous patch, we have fixed a bug in stack_depot_save() so that
>>>> it can be invoked even before stack depot is initialized. So there is
>>>> no reason to check the kmemleak_initialized in set_track_prepare().
>>>> So delete the kmemleak_initialized judgment in set_track_prepare()
>>>>
>>>> unreferenced object 0xc674ca80 (size 64):
>>>>    comm "swapper/0", pid 1, jiffies 4294938337 (age 204.880s)
>>>>    hex dump (first 32 bytes):
>>>>      80 55 75 c6 80 54 75 c6 00 55 75 c6 80 52 75 c6 .Uu..Tu..Uu..Ru.
>>>>      00 53 75 c6 00 00 00 00 00 00 00 00 00 00 00 00 .Su..........
>>>>
>>>> Fixes: 56a61617dd22 ("mm: use stack_depot for recording kmemleak's backtrace")
>>>> Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
>>>> ---
>>>>   mm/kmemleak.c | 2 --
>>>>   1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>>>> index a2d34226e3c8..c9f2f816db19 100644
>>>> --- a/mm/kmemleak.c
>>>> +++ b/mm/kmemleak.c
>>>> @@ -610,8 +610,6 @@ static noinline depot_stack_handle_t set_track_prepare(void)
>>>>       unsigned long entries[MAX_TRACE];
>>>>       unsigned int nr_entries;
>>>>
>>>> -    if (!kmemleak_initialized)
>>>> -            return 0;
>>>>       nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 3);
>>>>       trace_handle = stack_depot_save(entries, nr_entries, GFP_NOWAIT);
>>>>