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

  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().



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@windriver.com>
---
 mm/kmemleak.c | 2 --
 1 file changed, 2 deletions(-)
  

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?

> 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@windriver.com>
> ---
>  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);
>
  

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?

>> 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@windriver.com>
>> ---
>>  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);
>>  
>
  

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);'


I think there is still some memory not recorded on the backtrace before

stack_depot_early_init(), does anyone have a better suggestion?


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@windriver.com>
>>> ---
>>>   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);
>>>
  

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@windriver.com>
>>>> ---
>>>>   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);
>>>>
  

On Fri, Aug 11, 2023 at 10:09:08AM +0200, Vlastimil Babka wrote:
> On 8/11/23 04:03, wang xiaolei wrote:
> > On 8/10/23 9:16 PM, Vlastimil Babka wrote:
> >> 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.

Indeed, otherwise we'd get a lot of false positives.

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

It's not a functional problem, just a reporting one. There are
rare early leaks (usually false positives) so identifying them would
help. That said, I think set_track_prepare() is too conservative in
waiting for kmemleak_initialized to be set in kmemleak_late_init().
That's a late_initcall() meant for the scanning thread etc. not the core
kmemleak functionality (which is on from early boot).

We can instead use a different variable to check in set_track_prepare(),
e.g. object_cache. stack_depot_early_init() is called prior to
kmemleak_init(), so it should be fine.

If "kmemleak_initialized" is confusing, we could rename it to
"kmemleak_late_initialized" or "kmemleak_fully_initialized". I'm not too
fussed about this as long as we add some comment on why we check
object_cache instead of kmemleak_initialized.