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

[PATCH 1/1] 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>
---
 lib/stackdepot.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index f87d138..bb35b2c 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -214,6 +214,11 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
 	if (unlikely(trace->nr_entries == 0))
 		goto fast_exit;
 
+	if (trace->entries[trace->nr_entries - 1] < MODULES_VADDR) {
+		trace->entries[trace->nr_entries - 1] = ULONG_MAX;
+		trace->nr_entries--;
+	}
+
 	hash = hash_stack(trace->entries, trace->nr_entries);
 	bucket = &stack_table[hash & STACK_HASH_MASK];
 
-- 
1.9.1

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

[Dmitry Vyukov]

On Wed, Oct 11, 2017 at 1:24 PM, Maninder Singh <maninder1.s@samsung.com> 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
>
> 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>
> ---
>  lib/stackdepot.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index f87d138..bb35b2c 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -214,6 +214,11 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
>         if (unlikely(trace->nr_entries == 0))
>                 goto fast_exit;
>
> +       if (trace->entries[trace->nr_entries - 1] < MODULES_VADDR) {

I agree with general approach. But isn't kernel text below
MODULES_VADDR on e.g. x86_64?

> +               trace->entries[trace->nr_entries - 1] = ULONG_MAX;

Do we need this?

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

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

[Dmitry Vyukov]

On Wed, Oct 11, 2017 at 1:31 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Oct 11, 2017 at 1:24 PM, Maninder Singh <maninder1.s@samsung.com> 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
>>
>> 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>
>> ---
>>  lib/stackdepot.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
>> index f87d138..bb35b2c 100644
>> --- a/lib/stackdepot.c
>> +++ b/lib/stackdepot.c
>> @@ -214,6 +214,11 @@ depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
>>         if (unlikely(trace->nr_entries == 0))
>>                 goto fast_exit;
>>
>> +       if (trace->entries[trace->nr_entries - 1] < MODULES_VADDR) {
>
> I agree with general approach. But isn't kernel text below
> MODULES_VADDR on e.g. x86_64?
>
>> +               trace->entries[trace->nr_entries - 1] = ULONG_MAX;
>
> Do we need this?
>
>> +               trace->nr_entries--;
>> +       }
>> +
>>         hash = hash_stack(trace->entries, trace->nr_entries);
>>         bucket = &stack_table[hash & STACK_HASH_MASK];


+kasan-dev mailing list

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2062 bytes --]

Hi Maninder,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc4 next-20171013]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Maninder-Singh/stackdepot-ignore-junk-last-entry-in-case-of-switch-from-user-mode/20171014-153544
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   lib/stackdepot.c: In function 'depot_save_stack':
>> lib/stackdepot.c:217:46: error: 'MODULES_VADDR' undeclared (first use in this function)
     if (trace->entries[trace->nr_entries - 1] < MODULES_VADDR) {
                                                 ^
   lib/stackdepot.c:217:46: note: each undeclared identifier is reported only once for each function it appears in

vim +/MODULES_VADDR +217 lib/stackdepot.c

   196	
   197	/**
   198	 * depot_save_stack - save stack in a stack depot.
   199	 * @trace - the stacktrace to save.
   200	 * @alloc_flags - flags for allocating additional memory if required.
   201	 *
   202	 * Returns the handle of the stack struct stored in depot.
   203	 */
   204	depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
   205					    gfp_t alloc_flags)
   206	{
   207		u32 hash;
   208		depot_stack_handle_t retval = 0;
   209		struct stack_record *found = NULL, **bucket;
   210		unsigned long flags;
   211		struct page *page = NULL;
   212		void *prealloc = NULL;
   213	
   214		if (unlikely(trace->nr_entries == 0))
   215			goto fast_exit;
   216	
 > 217		if (trace->entries[trace->nr_entries - 1] < MODULES_VADDR) {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51614 bytes --]