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

2017-10-14 Thread kbuild test robot 
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2017-10-14 Thread kbuild test robot 
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2017-10-11 Thread Dmitry Vyukov 
On Wed, Oct 11, 2017 at 1:31 PM, Dmitry Vyukov  wrote:
> On Wed, Oct 11, 2017 at 1:24 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
>>
>> 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 
>> Signed-off-by: Maninder Singh 
>> ---
>>  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 = _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.

2017-10-11 Thread Dmitry Vyukov 
On Wed, Oct 11, 2017 at 1:31 PM, Dmitry Vyukov  wrote:
> On Wed, Oct 11, 2017 at 1:24 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
>>
>> 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 
>> Signed-off-by: Maninder Singh 
>> ---
>>  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 = _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.

2017-10-11 Thread Dmitry Vyukov 
On Wed, Oct 11, 2017 at 1:24 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
>
> 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 
> Signed-off-by: Maninder Singh 
> ---
>  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 = _table[hash & STACK_HASH_MASK];
>
> --
> 1.9.1
>

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

2017-10-11 Thread Dmitry Vyukov 
On Wed, Oct 11, 2017 at 1:24 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
>
> 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 
> Signed-off-by: Maninder Singh 
> ---
>  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 = _table[hash & STACK_HASH_MASK];
>
> --
> 1.9.1
>

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

2017-10-11 Thread 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 
Signed-off-by: Maninder Singh 
---
 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 = _table[hash & STACK_HASH_MASK];
 
-- 
1.9.1

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

2017-10-11 Thread 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 
Signed-off-by: Maninder Singh 
---
 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 = _table[hash & STACK_HASH_MASK];
 
-- 
1.9.1