[PATCH v2] stackdepot: ignore junk last entry in case of switch from user mode.

[PATCH v2] stackdepot: ignore junk last entry in case of switch from user mode.

[Maninder Singh]

Issue observed on ARM.

Whenever there is switch from user mode, we end up with invalid last entry
with some user space address as below:-

 save_stack+0x40/0xec
 __set_page_owner+0x2c/0x64
....
....
 __handle_domain_irq+0x9c/0x130
 gic_handle_irq+0x40/0x80
 __irq_usr+0x4c/0x60
 0xb6507818

So in this case last entry is not valid, which leads to allocated one more
new frame for stackdepot although having all above frames exactly same.

(It increases depot_index drastically)

So its better to ignore that last frame in case of switch.
 save_stack+0x40/0xec
 __set_page_owner+0x2c/0x64
....
....
 __handle_domain_irq+0x9c/0x130
 gic_handle_irq+0x40/0x80
 __irq_usr+0x4c/0x60

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
v1 -> v2: use generic API to check whether address belongs to user space or not.

 lib/stackdepot.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index f87d138..a045748 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -214,6 +214,13 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
 	if (unlikely(trace->nr_entries == 0))
 		goto fast_exit;
 
+	/* 
+	 * Ignore last entry if it belongs to user space
+	 * in case of switch from user mode.
+	 */
+	if (!kernel_text_address(trace->entries[trace->nr_entries - 1]))
+		trace->nr_entries--;
+
 	hash = hash_stack(trace->entries, trace->nr_entries);
 	bucket = &stack_table[hash & STACK_HASH_MASK];
 
-- 
1.9.1

RE: [PATCH v2] stackdepot: ignore junk last entry in case of switch from user mode.

[Vaneet Narang]

Hi Dmitry,

>diff --git a/lib/stackdepot.c b/lib/stackdepot.c
>index f87d138..a045748 100644
>--- a/lib/stackdepot.c
>+++ b/lib/stackdepot.c
>@@ -214,6 +214,13 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
> 	if (unlikely(trace->nr_entries == 0))
> 		goto fast_exit;
> 
>+	/* 
>+	 * Ignore last entry if it belongs to user space
>+	 * in case of switch from user mode.
>+	 */
>+	if (!kernel_text_address(trace->entries[trace->nr_entries - 1]))

can we also add this check here:-
if (!kernel_text_address(trace->entries[trace->nr_entries - 1]) || in_interrupt())

Because on further checking , last frame in case of svc to interrupt mode is some random core kernel / Kernel Module address
like below:-

irq_exit+0xe4/0x140
__handle_domain_irq+0x9c/0x130
gic_handle_irq+0x40/0x80
__irq_svc+0x44/0x7c
hdmi_ocm_rx_get_scdc_sts+0x160/0x170 [sdp_hdmi]   >> we need this frme??

This generates mismatch with existing entires and new entries get created and depot_index
value keep on increasing slowly and reaches max value (STACK_ALLOC_SLABS_CAP) but after this change 
depot_index gets a stable value after some time. (Tested on ARM)

>+		trace->nr_entries--;
>+
> 	hash = hash_stack(trace->entries, trace->nr_entries);
> 	bucket = &stack_table[hash & STACK_HASH_MASK];
> 
>-- 
>1.9.1

Regards,
Vaneet Narang

Re: [PATCH v2] stackdepot: ignore junk last entry in case of switch from user mode.

[Dmitry Vyukov]

On Thu, Oct 12, 2017 at 1:10 PM, Vaneet Narang <v.narang@samsung.com> wrote:
> Hi Dmitry,
>
>>diff --git a/lib/stackdepot.c b/lib/stackdepot.c
>>index f87d138..a045748 100644
>>--- a/lib/stackdepot.c
>>+++ b/lib/stackdepot.c
>>@@ -214,6 +214,13 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
>>       if (unlikely(trace->nr_entries == 0))
>>               goto fast_exit;
>>
>>+      /*
>>+       * Ignore last entry if it belongs to user space
>>+       * in case of switch from user mode.
>>+       */
>>+      if (!kernel_text_address(trace->entries[trace->nr_entries - 1]))
>
> can we also add this check here:-
> if (!kernel_text_address(trace->entries[trace->nr_entries - 1]) || in_interrupt())
>
> Because on further checking , last frame in case of svc to interrupt mode is some random core kernel / Kernel Module address
> like below:-
>
> irq_exit+0xe4/0x140
> __handle_domain_irq+0x9c/0x130
> gic_handle_irq+0x40/0x80
> __irq_svc+0x44/0x7c
> hdmi_ocm_rx_get_scdc_sts+0x160/0x170 [sdp_hdmi]   >> we need this frme??
>
> This generates mismatch with existing entires and new entries get created and depot_index
> value keep on increasing slowly and reaches max value (STACK_ALLOC_SLABS_CAP) but after this change
> depot_index gets a stable value after some time. (Tested on ARM)


I think you need to piggy-back on irq stack support in kasan. See
filter_irq_stacks in mm/kasan/kasan.c. If you put __irq_svc (and
potentially other irq entry points) into irq/softirq section, then
they will be filtered automatically.
Also note that at least on x86 stack unwinder unwinds whole irq stack
and then whole kernel thread stack, so generally we need to filter not
just the last entry, but everything below irq entry point.

And I think this whole change need to be moved into kasan's save_stack
function. There is already a similar workarounds for x86. Currently
stackdepot merely memorizes the input data, regardless of what it is
(theoretically, not necessary even a stack trace).

Re: [PATCH v2] stackdepot: ignore junk last entry in case of switch from user mode.

[Andrey Ryabinin]

On 10/11/2017 03:43 PM, Maninder Singh wrote:
> Issue observed on ARM.
> 
> Whenever there is switch from user mode, we end up with invalid last entry
> with some user space address as below:-
> 
>  save_stack+0x40/0xec
>  __set_page_owner+0x2c/0x64
> ....
> ....
>  __handle_domain_irq+0x9c/0x130
>  gic_handle_irq+0x40/0x80
>  __irq_usr+0x4c/0x60
>  0xb6507818
> 

Sounds like an ARM issue to me.

...
 
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index f87d138..a045748 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -214,6 +214,13 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
>  	if (unlikely(trace->nr_entries == 0))
>  		goto fast_exit;
>  
> +	/* 
> +	 * Ignore last entry if it belongs to user space

And if it belongs to module too. NACK.


> +	 * in case of switch from user mode.
> +	 */
> +	if (!kernel_text_address(trace->entries[trace->nr_entries - 1]))
> +		trace->nr_entries--;
> +
>  	hash = hash_stack(trace->entries, trace->nr_entries);
>  	bucket = &stack_table[hash & STACK_HASH_MASK];
>  
>