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

[wangxiaolei <xiaolei.wang@windriver.com>]

On 8/15/23 12:20 AM, Catalin Marinas 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 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.

Ok, I will send v2 version, use object_cache instead of kmemleak_initialized

to check in set_track_prepare, and update kmemleak_initialized to 
kmemleak_late_initialized

thanks

xiaolei

>
> --
> Catalin