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/[email protected]/
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
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 <[email protected]>
Suggested-by: Vlastimil Babka <[email protected]>
---
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
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 <[email protected]>
Suggested-by: Vlastimil Babka <[email protected]>
---
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
+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 <[email protected]>
> Suggested-by: Vlastimil Babka <[email protected]>
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);
>
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 <[email protected]>
> Suggested-by: Vlastimil Babka <[email protected]>
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
> *
>
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 <[email protected]>
>> Suggested-by: Vlastimil Babka <[email protected]>
>
> 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
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 <[email protected]>
>> Suggested-by: Vlastimil Babka <[email protected]>
>
> 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
On 9/2/21 02:11, [email protected] 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 <[email protected]>
>>> Suggested-by: Vlastimil Babka <[email protected]>
>>
>> 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.