Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1245672 > unrolled thread

[PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan()

Started byAndrey Ryabinin <aryabinin@virtuozzo.com>
First post2015-10-13 14:40 +0200
Last post2015-10-13 16:30 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-13 14:40 +0200
    Re: [PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan() Ingo Molnar <mingo@kernel.org> - 2015-10-13 15:50 +0200
      Re: [PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-13 16:00 +0200
      Re: [PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-10-13 16:00 +0200
        Re: [PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-13 16:20 +0200
        Re: [PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan() Ingo Molnar <mingo@kernel.org> - 2015-10-13 16:30 +0200

#1245672 — [PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan()

FromAndrey Ryabinin <aryabinin@virtuozzo.com>
Date2015-10-13 14:40 +0200
Subject[PATCH v2 2/2] x86/process: Silence KASAN warnings in get_wchan()
Message-ID<qj721-6Fq-15@gated-at.bofh.it>
get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use READ_ONCE_NOCHECK() to silence these warnings.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 arch/x86/kernel/process.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 39e585a..e28db18 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
 	if (sp < bottom || sp > top)
 		return 0;
 
-	fp = READ_ONCE(*(unsigned long *)sp);
+	fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
 	do {
 		if (fp < bottom || fp > top)
 			return 0;
-		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
+		ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
 		if (!in_sched_functions(ip))
 			return ip;
-		fp = READ_ONCE(*(unsigned long *)fp);
+		fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
 	} while (count++ < 16 && p->state != TASK_RUNNING);
 	return 0;
 }
-- 
2.4.9

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1245727

FromIngo Molnar <mingo@kernel.org>
Date2015-10-13 15:50 +0200
Message-ID<qj87M-8dT-17@gated-at.bofh.it>
In reply to#1245672
* Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:

> get_wchan() is racy by design, it may access volatile stack
> of running task, thus it may access redzone in a stack frame
> and cause KASAN to warn about this.
> 
> Use READ_ONCE_NOCHECK() to silence these warnings.
> 
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
>  arch/x86/kernel/process.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 39e585a..e28db18 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
>  	if (sp < bottom || sp > top)
>  		return 0;
>  
> -	fp = READ_ONCE(*(unsigned long *)sp);
> +	fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
>  	do {
>  		if (fp < bottom || fp > top)
>  			return 0;
> -		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
> +		ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
>  		if (!in_sched_functions(ip))
>  			return ip;
> -		fp = READ_ONCE(*(unsigned long *)fp);
> +		fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
>  	} while (count++ < 16 && p->state != TASK_RUNNING);
>  	return 0;
>  }

Hm, exactly how is the 'red zone' defined? Is this about the current task mostly, 
or when doing get_wchan() on other tasks?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245736

FromAndrey Ryabinin <aryabinin@virtuozzo.com>
Date2015-10-13 16:00 +0200
Message-ID<qj8hr-8pd-5@gated-at.bofh.it>
In reply to#1245727

On 10/13/2015 04:48 PM, Ingo Molnar wrote:
> 
> * Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> 
>> get_wchan() is racy by design, it may access volatile stack
>> of running task, thus it may access redzone in a stack frame
>> and cause KASAN to warn about this.
>>
>> Use READ_ONCE_NOCHECK() to silence these warnings.
>>
>> Reported-by: Sasha Levin <sasha.levin@oracle.com>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>>  arch/x86/kernel/process.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 39e585a..e28db18 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
>>  	if (sp < bottom || sp > top)
>>  		return 0;
>>  
>> -	fp = READ_ONCE(*(unsigned long *)sp);
>> +	fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
>>  	do {
>>  		if (fp < bottom || fp > top)
>>  			return 0;
>> -		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>> +		ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
>>  		if (!in_sched_functions(ip))
>>  			return ip;
>> -		fp = READ_ONCE(*(unsigned long *)fp);
>> +		fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
>>  	} while (count++ < 16 && p->state != TASK_RUNNING);
>>  	return 0;
>>  }
> 
> Hm, exactly how is the 'red zone' defined? Is this about the current task mostly, 
> or when doing get_wchan() on other tasks?

We doing get_whcan() *only* on other tasks:

520:	if (!p || p == current || p->state == TASK_RUNNING)
521:		return 0;


Current wouldn't be a problem for KASAN.

 


> Thanks,
> 
> 	Ingo
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245741

FromDmitry Vyukov <dvyukov@google.com>
Date2015-10-13 16:00 +0200
Message-ID<qj8hs-8pd-15@gated-at.bofh.it>
In reply to#1245727
On Tue, Oct 13, 2015 at 3:48 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>> get_wchan() is racy by design, it may access volatile stack
>> of running task, thus it may access redzone in a stack frame
>> and cause KASAN to warn about this.
>>
>> Use READ_ONCE_NOCHECK() to silence these warnings.
>>
>> Reported-by: Sasha Levin <sasha.levin@oracle.com>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>>  arch/x86/kernel/process.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 39e585a..e28db18 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
>>       if (sp < bottom || sp > top)
>>               return 0;
>>
>> -     fp = READ_ONCE(*(unsigned long *)sp);
>> +     fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
>>       do {
>>               if (fp < bottom || fp > top)
>>                       return 0;
>> -             ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>> +             ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
>>               if (!in_sched_functions(ip))
>>                       return ip;
>> -             fp = READ_ONCE(*(unsigned long *)fp);
>> +             fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
>>       } while (count++ < 16 && p->state != TASK_RUNNING);
>>       return 0;
>>  }
>
> Hm, exactly how is the 'red zone' defined? Is this about the current task mostly,
> or when doing get_wchan() on other tasks?


When code is compiled with AddressSanitizer, most variables on stack
have redzones around them, on entry function "poisons" these redzones
(any accesses to them will be flagged), on exit function "unpoisons"
these redzones.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245768

FromAndrey Ryabinin <aryabinin@virtuozzo.com>
Date2015-10-13 16:20 +0200
Message-ID<qj8AO-zI-15@gated-at.bofh.it>
In reply to#1245741

On 10/13/2015 04:57 PM, Dmitry Vyukov wrote:
> On Tue, Oct 13, 2015 at 3:48 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>
>>> get_wchan() is racy by design, it may access volatile stack
>>> of running task, thus it may access redzone in a stack frame
>>> and cause KASAN to warn about this.
>>>
>>> Use READ_ONCE_NOCHECK() to silence these warnings.
>>>
>>> Reported-by: Sasha Levin <sasha.levin@oracle.com>
>>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>> ---
>>>  arch/x86/kernel/process.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>>> index 39e585a..e28db18 100644
>>> --- a/arch/x86/kernel/process.c
>>> +++ b/arch/x86/kernel/process.c
>>> @@ -550,14 +550,14 @@ unsigned long get_wchan(struct task_struct *p)
>>>       if (sp < bottom || sp > top)
>>>               return 0;
>>>
>>> -     fp = READ_ONCE(*(unsigned long *)sp);
>>> +     fp = READ_ONCE_NOCHECK(*(unsigned long *)sp);
>>>       do {
>>>               if (fp < bottom || fp > top)
>>>                       return 0;
>>> -             ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>>> +             ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
>>>               if (!in_sched_functions(ip))
>>>                       return ip;
>>> -             fp = READ_ONCE(*(unsigned long *)fp);
>>> +             fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
>>>       } while (count++ < 16 && p->state != TASK_RUNNING);
>>>       return 0;
>>>  }
>>
>> Hm, exactly how is the 'red zone' defined? Is this about the current task mostly,
>> or when doing get_wchan() on other tasks?
> 
> 
> When code is compiled with AddressSanitizer, most variables on stack
> have redzones around them, on entry function "poisons" these redzones
> (any accesses to them will be flagged), on exit function "unpoisons"
> these redzones.
> 

An example bellow (stolen from slides - http://events.linuxfoundation.org/sites/events/files/slides/LinuxCon%20North%20America%202015%20KernelAddressSanitizer.pdf)

The following function:
void foo(void) {
	char a[328];
	...
	a[i] = 0;
}

will be transform by GCC to something like this:

void foo(void) {
       char redzone1[32];
       char a[328];
       char redzone2[24];
       char redzone3[32];

       int *shadow = (&redzone1 >> 3) + shadow_offset;
       shadow[0] = 0xf1f1f1f1; // poison redzone1
       shadow[11] = 0xf4f4f400; // poison redzone2
       shadow[12] = 0xf3f3f3f3; // poison redzone3

       ...
       __asan_store1(&a[i]); //check access to a[i]
       a[i] = 0;

       shadow[0] = shadow[11] = shadow[12] = 0; //unpoison redzones.
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245782

FromIngo Molnar <mingo@kernel.org>
Date2015-10-13 16:30 +0200
Message-ID<qj8Kv-KS-33@gated-at.bofh.it>
In reply to#1245741
* Dmitry Vyukov <dvyukov@google.com> wrote:

> > Hm, exactly how is the 'red zone' defined? Is this about the current task 
> > mostly, or when doing get_wchan() on other tasks?
> 
> When code is compiled with AddressSanitizer, most variables on stack have 
> redzones around them, on entry function "poisons" these redzones (any accesses 
> to them will be flagged), on exit function "unpoisons" these redzones.

I see, fair enough!

This series looks good to me, modulo the small documentation nit I had about the 
first patch.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web