* [RFC PATCH 0/2] lib, stackdepot: check stackdepot handle before accessing slabs @ 2021-09-01 5:19 Imran Khan 2021-09-01 5:19 ` [RFC PATCH 1/2] " Imran Khan 2021-09-01 5:19 ` [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries Imran Khan 0 siblings, 2 replies; 8+ messages in thread From: Imran Khan @ 2021-09-01 5:19 UTC (permalink / raw) To: geert, vbabka, akpm; +Cc: linux-kernel This RFC patch series addresses suggestion discussed in an earlier RFC [1]. Since earlier RFC was about SLUB subsystem, and current changes only involve stackdepot, I am submitting the patches in a new thread. The changes of this patch set are as follows: PATCH-1: Checks validity of a stackdepot handle before proceeding to access stackdepot slab/objects. PATCH-2: Adds a helper in stackdepot, to allow users to print stack entries just by specifying the stackdepot handle. [1] https://lore.kernel.org/lkml/2772cf56-4183-857f-d070-c54bceb5c8d9@suse.cz/ Imran Khan (2): lib, stackdepot: check stackdepot handle before accessing slabs. lib, stackdepot: Add helper to print stack entries. lib/stackdepot.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) -- 2.30.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH 1/2] lib, stackdepot: check stackdepot handle before accessing slabs. 2021-09-01 5:19 [RFC PATCH 0/2] lib, stackdepot: check stackdepot handle before accessing slabs Imran Khan @ 2021-09-01 5:19 ` Imran Khan 2021-09-01 8:58 ` Vlastimil Babka 2021-09-01 5:19 ` [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries Imran Khan 1 sibling, 1 reply; 8+ messages in thread From: Imran Khan @ 2021-09-01 5:19 UTC (permalink / raw) To: geert, vbabka, akpm; +Cc: linux-kernel stack_depot_save allocates slabs that will be used for storing objects in future.If this slab allocation fails we may get to a situation where space allocation for a new stack_record fails, causing stack_depot_save to return 0 as handle. If user of this handle ends up invoking stack_depot_fetch with this handle value, current implementation of stack_depot_fetch will end up using slab from wrong index. To avoid this check handle value at the beginning. Also issue a warning for nil handle values and when slab allocation for stackdepot fails for the first time. Signed-off-by: Imran Khan <imran.f.khan@oracle.com> Suggested-by: Vlastimil Babka <vbabka@suse.cz> --- lib/stackdepot.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/stackdepot.c b/lib/stackdepot.c index 0a2e417f83cb..1d42ef9ef766 100644 --- a/lib/stackdepot.c +++ b/lib/stackdepot.c @@ -232,6 +232,10 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle, struct stack_record *stack; *entries = NULL; + if (!handle) { + WARN(1, "stack depot handle is absent.\n"); + return 0; + } if (parts.slabindex > depot_index) { WARN(1, "slab index %d out of bounds (%d) for stack id %08x\n", parts.slabindex, depot_index, handle); @@ -303,6 +307,8 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries, page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER); if (page) prealloc = page_address(page); + else + WARN_ONCE(1, "slab allocation for stack depot failed.\n"); } raw_spin_lock_irqsave(&depot_lock, flags); -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] lib, stackdepot: check stackdepot handle before accessing slabs. 2021-09-01 5:19 ` [RFC PATCH 1/2] " Imran Khan @ 2021-09-01 8:58 ` Vlastimil Babka 2021-09-02 0:12 ` imran.f.khan 0 siblings, 1 reply; 8+ messages in thread From: Vlastimil Babka @ 2021-09-01 8:58 UTC (permalink / raw) To: Imran Khan, geert, akpm; +Cc: linux-kernel, linux-mm, Alexander Potapenko +CC linux-mm, Alex On 9/1/21 07:19, Imran Khan wrote: > stack_depot_save allocates slabs that will be used for storing > objects in future.If this slab allocation fails we may get to > a situation where space allocation for a new stack_record fails, > causing stack_depot_save to return 0 as handle. > If user of this handle ends up invoking stack_depot_fetch with > this handle value, current implementation of stack_depot_fetch > will end up using slab from wrong index. > To avoid this check handle value at the beginning. > Also issue a warning for nil handle values and when slab allocation > for stackdepot fails for the first time. > > Signed-off-by: Imran Khan <imran.f.khan@oracle.com> > Suggested-by: Vlastimil Babka <vbabka@suse.cz> Agree but without the warnings please, especially the "stack depot handle is absent" one. It's just something that can happen e.g. in GFP_NOWAIT contexts and no need to spam dmesg. > --- > lib/stackdepot.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/lib/stackdepot.c b/lib/stackdepot.c > index 0a2e417f83cb..1d42ef9ef766 100644 > --- a/lib/stackdepot.c > +++ b/lib/stackdepot.c > @@ -232,6 +232,10 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle, > struct stack_record *stack; > > *entries = NULL; > + if (!handle) { > + WARN(1, "stack depot handle is absent.\n"); > + return 0; > + } > if (parts.slabindex > depot_index) { > WARN(1, "slab index %d out of bounds (%d) for stack id %08x\n", > parts.slabindex, depot_index, handle); > @@ -303,6 +307,8 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries, > page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER); > if (page) > prealloc = page_address(page); > + else > + WARN_ONCE(1, "slab allocation for stack depot failed.\n"); > } > > raw_spin_lock_irqsave(&depot_lock, flags); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] lib, stackdepot: check stackdepot handle before accessing slabs. 2021-09-01 8:58 ` Vlastimil Babka @ 2021-09-02 0:12 ` imran.f.khan 0 siblings, 0 replies; 8+ messages in thread From: imran.f.khan @ 2021-09-02 0:12 UTC (permalink / raw) To: Vlastimil Babka, geert, akpm; +Cc: linux-kernel, linux-mm, Alexander Potapenko On 1/9/21 6:58 pm, Vlastimil Babka wrote: > +CC linux-mm, Alex > > On 9/1/21 07:19, Imran Khan wrote: >> stack_depot_save allocates slabs that will be used for storing >> objects in future.If this slab allocation fails we may get to >> a situation where space allocation for a new stack_record fails, >> causing stack_depot_save to return 0 as handle. >> If user of this handle ends up invoking stack_depot_fetch with >> this handle value, current implementation of stack_depot_fetch >> will end up using slab from wrong index. >> To avoid this check handle value at the beginning. >> Also issue a warning for nil handle values and when slab allocation >> for stackdepot fails for the first time. >> >> Signed-off-by: Imran Khan <imran.f.khan@oracle.com> >> Suggested-by: Vlastimil Babka <vbabka@suse.cz> > > Agree but without the warnings please, especially the "stack depot handle is > absent" one. It's just something that can happen e.g. in GFP_NOWAIT contexts > and no need to spam dmesg. > Okay. I have removed warnings in v2 of patch set. >> --- >> lib/stackdepot.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/lib/stackdepot.c b/lib/stackdepot.c >> index 0a2e417f83cb..1d42ef9ef766 100644 >> --- a/lib/stackdepot.c >> +++ b/lib/stackdepot.c >> @@ -232,6 +232,10 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle, >> struct stack_record *stack; >> >> *entries = NULL; >> + if (!handle) { >> + WARN(1, "stack depot handle is absent.\n"); >> + return 0; >> + } >> if (parts.slabindex > depot_index) { >> WARN(1, "slab index %d out of bounds (%d) for stack id %08x\n", >> parts.slabindex, depot_index, handle); >> @@ -303,6 +307,8 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries, >> page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER); >> if (page) >> prealloc = page_address(page); >> + else >> + WARN_ONCE(1, "slab allocation for stack depot failed.\n"); >> } >> >> raw_spin_lock_irqsave(&depot_lock, flags); >> > Thanks for reviewing this. --Imran ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries. 2021-09-01 5:19 [RFC PATCH 0/2] lib, stackdepot: check stackdepot handle before accessing slabs Imran Khan 2021-09-01 5:19 ` [RFC PATCH 1/2] " Imran Khan @ 2021-09-01 5:19 ` Imran Khan 2021-09-01 9:07 ` Vlastimil Babka 1 sibling, 1 reply; 8+ messages in thread From: Imran Khan @ 2021-09-01 5:19 UTC (permalink / raw) To: geert, vbabka, akpm; +Cc: linux-kernel To print a stack entries, users of stackdepot, first use stack_depot_fetch to get a list of stack entries and then use stack_trace_print to print this list. Provide a helper in stackdepot to print stack entries based on stackdepot handle. Signed-off-by: Imran Khan <imran.f.khan@oracle.com> Suggested-by: Vlastimil Babka <vbabka@suse.cz> --- lib/stackdepot.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/stackdepot.c b/lib/stackdepot.c index 1d42ef9ef766..eab4476b060b 100644 --- a/lib/stackdepot.c +++ b/lib/stackdepot.c @@ -214,6 +214,23 @@ static inline struct stack_record *find_stack(struct stack_record *bucket, return NULL; } +/** + * stack_depot_print - print stack entries from a depot + * + * @handle: Stack depot handle which was returned from + * stack_depot_save(). + * + */ +void stack_depot_print(depot_stack_handle_t stack) +{ + unsigned long *entries; + unsigned int nr_entries; + + nr_entries = stack_depot_fetch(stack, &entries); + stack_trace_print(entries, nr_entries, 0); +} +EXPORT_SYMBOL_GPL(stack_depot_print); + /** * stack_depot_fetch - Fetch stack entries from a depot * -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries. 2021-09-01 5:19 ` [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries Imran Khan @ 2021-09-01 9:07 ` Vlastimil Babka 2021-09-02 0:11 ` imran.f.khan 0 siblings, 1 reply; 8+ messages in thread From: Vlastimil Babka @ 2021-09-01 9:07 UTC (permalink / raw) To: Imran Khan, geert, akpm; +Cc: linux-kernel, linux-mm, Alexander Potapenko On 9/1/21 07:19, Imran Khan wrote: > To print a stack entries, users of stackdepot, first > use stack_depot_fetch to get a list of stack entries > and then use stack_trace_print to print this list. > Provide a helper in stackdepot to print stack entries > based on stackdepot handle. > > Signed-off-by: Imran Khan <imran.f.khan@oracle.com> > Suggested-by: Vlastimil Babka <vbabka@suse.cz> You should convert existing users together with the patch that introduces the helper. I think print_stack() in mm/kasan/report.c, and __dump_page_owner() could use this. > --- > lib/stackdepot.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) It's missing a declaration in include/linux/stackdepot.h Perhaps it could be all be a static inline there anyway. > diff --git a/lib/stackdepot.c b/lib/stackdepot.c > index 1d42ef9ef766..eab4476b060b 100644 > --- a/lib/stackdepot.c > +++ b/lib/stackdepot.c > @@ -214,6 +214,23 @@ static inline struct stack_record *find_stack(struct stack_record *bucket, > return NULL; > } > > +/** > + * stack_depot_print - print stack entries from a depot > + * > + * @handle: Stack depot handle which was returned from > + * stack_depot_save(). > + * > + */ > +void stack_depot_print(depot_stack_handle_t stack) > +{ > + unsigned long *entries; > + unsigned int nr_entries; > + > + nr_entries = stack_depot_fetch(stack, &entries); > + stack_trace_print(entries, nr_entries, 0); > +} > +EXPORT_SYMBOL_GPL(stack_depot_print); > + > /** > * stack_depot_fetch - Fetch stack entries from a depot > * > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries. 2021-09-01 9:07 ` Vlastimil Babka @ 2021-09-02 0:11 ` imran.f.khan 2021-09-02 8:00 ` Vlastimil Babka 0 siblings, 1 reply; 8+ messages in thread From: imran.f.khan @ 2021-09-02 0:11 UTC (permalink / raw) To: Vlastimil Babka, geert, akpm; +Cc: linux-kernel, linux-mm, Alexander Potapenko Hi Vlastimil, On 1/9/21 7:07 pm, Vlastimil Babka wrote: > On 9/1/21 07:19, Imran Khan wrote: >> To print a stack entries, users of stackdepot, first >> use stack_depot_fetch to get a list of stack entries >> and then use stack_trace_print to print this list. >> Provide a helper in stackdepot to print stack entries >> based on stackdepot handle. >> >> Signed-off-by: Imran Khan <imran.f.khan@oracle.com> >> Suggested-by: Vlastimil Babka <vbabka@suse.cz> > > You should convert existing users together with the patch that introduces > the helper. I think print_stack() in mm/kasan/report.c, and > __dump_page_owner() could use this. > Okay. I have done this in v2 of the patch set. BTW I also see some users (one place in page owner, rest all in some gpu drivers) of stack_depot_fetch + stack_trace_snprintf. Could you please let me know if it would be okay to add a helper corresponding to stack_trace_snprintf as well. >> --- >> lib/stackdepot.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) > > It's missing a declaration in include/linux/stackdepot.h > Perhaps it could be all be a static inline there anyway. > Agree. I have added missing declaration now. I have not made it inline though. >> diff --git a/lib/stackdepot.c b/lib/stackdepot.c >> index 1d42ef9ef766..eab4476b060b 100644 >> --- a/lib/stackdepot.c >> +++ b/lib/stackdepot.c >> @@ -214,6 +214,23 @@ static inline struct stack_record *find_stack(struct stack_record *bucket, >> return NULL; >> } >> >> +/** >> + * stack_depot_print - print stack entries from a depot >> + * >> + * @handle: Stack depot handle which was returned from >> + * stack_depot_save(). >> + * >> + */ >> +void stack_depot_print(depot_stack_handle_t stack) >> +{ >> + unsigned long *entries; >> + unsigned int nr_entries; >> + >> + nr_entries = stack_depot_fetch(stack, &entries); >> + stack_trace_print(entries, nr_entries, 0); >> +} >> +EXPORT_SYMBOL_GPL(stack_depot_print); >> + >> /** >> * stack_depot_fetch - Fetch stack entries from a depot >> * >> > Thanks again for the review. --Imran ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries. 2021-09-02 0:11 ` imran.f.khan @ 2021-09-02 8:00 ` Vlastimil Babka 0 siblings, 0 replies; 8+ messages in thread From: Vlastimil Babka @ 2021-09-02 8:00 UTC (permalink / raw) To: imran.f.khan, geert, akpm; +Cc: linux-kernel, linux-mm, Alexander Potapenko On 9/2/21 02:11, imran.f.khan@oracle.com wrote: > Hi Vlastimil, > > On 1/9/21 7:07 pm, Vlastimil Babka wrote: >> On 9/1/21 07:19, Imran Khan wrote: >>> To print a stack entries, users of stackdepot, first >>> use stack_depot_fetch to get a list of stack entries >>> and then use stack_trace_print to print this list. >>> Provide a helper in stackdepot to print stack entries >>> based on stackdepot handle. >>> >>> Signed-off-by: Imran Khan <imran.f.khan@oracle.com> >>> Suggested-by: Vlastimil Babka <vbabka@suse.cz> >> >> You should convert existing users together with the patch that introduces >> the helper. I think print_stack() in mm/kasan/report.c, and >> __dump_page_owner() could use this. >> > > Okay. I have done this in v2 of the patch set. BTW I also see some users > (one place in page owner, rest all in some gpu drivers) of stack_depot_fetch > + stack_trace_snprintf. Could you please let me know if it would be okay to > add a helper corresponding to stack_trace_snprintf as well. Yeah looks like that would make sense too, i915 even has this as __print_depot_stack(). Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-09-02 8:00 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-01 5:19 [RFC PATCH 0/2] lib, stackdepot: check stackdepot handle before accessing slabs Imran Khan 2021-09-01 5:19 ` [RFC PATCH 1/2] " Imran Khan 2021-09-01 8:58 ` Vlastimil Babka 2021-09-02 0:12 ` imran.f.khan 2021-09-01 5:19 ` [RFC PATCH 2/2] lib, stackdepot: Add helper to print stack entries Imran Khan 2021-09-01 9:07 ` Vlastimil Babka 2021-09-02 0:11 ` imran.f.khan 2021-09-02 8:00 ` Vlastimil Babka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).