| Message ID | 20230810074704.2042664-3-xiaolei.wang@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bail out in __stack_depot_save() if the stack_table is not allocated and delete the kmemleak_initialized judgment in set_track_prepare() | expand |
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.
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
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);
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(-)