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

Groups > linux.kernel > #1239437 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-05 12:40 +0200
    Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-10-05 13:20 +0200
    Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Ingo Molnar <mingo@kernel.org> - 2015-10-05 13:30 +0200
      Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-10-05 13:50 +0200
        Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-05 13:50 +0200
          Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-10-05 15:20 +0200
            Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-10-05 20:50 +0200
          Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andi Kleen <ak@linux.intel.com> - 2015-10-05 18:40 +0200
            Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-05 19:00 +0200
              Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andi Kleen <ak@linux.intel.com> - 2015-10-05 21:00 +0200
              Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Ingo Molnar <mingo@kernel.org> - 2015-10-06 09:30 +0200
                Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-06 09:40 +0200
                Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-10-06 20:20 +0200
                  Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Ingo Molnar <mingo@kernel.org> - 2015-10-07 09:30 +0200
                  Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-07 11:00 +0200
                    Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-07 11:20 +0200
                    Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andi Kleen <ak@linux.intel.com> - 2015-10-07 18:30 +0200
                      Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-07 20:50 +0200

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

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 kasan_disable_current()/kasan_enable_current() to silence
these warnings.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---

 Perhaps it would be better to add something like this:
	READ_ONCE_NOCHECK()
	{
		kasan_disable_current();
		READ_ONCE();
		kasan_enable_current();
	}
  ?

 arch/x86/kernel/process.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 39e585a..0488eb9 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -13,6 +13,7 @@
 #include <linux/random.h>
 #include <linux/user-return-notifier.h>
 #include <linux/dmi.h>
+#include <linux/kasan.h>
 #include <linux/utsname.h>
 #include <linux/stackprotector.h>
 #include <linux/tick.h>
@@ -514,7 +515,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
  */
 unsigned long get_wchan(struct task_struct *p)
 {
-	unsigned long start, bottom, top, sp, fp, ip;
+	unsigned long start, bottom, top, sp, fp, ip, ret = 0;
 	int count = 0;
 
 	if (!p || p == current || p->state == TASK_RUNNING)
@@ -550,14 +551,21 @@ unsigned long get_wchan(struct task_struct *p)
 	if (sp < bottom || sp > top)
 		return 0;
 
+	kasan_disable_current();
 	fp = READ_ONCE(*(unsigned long *)sp);
 	do {
 		if (fp < bottom || fp > top)
-			return 0;
+			goto out;
+
 		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
-		if (!in_sched_functions(ip))
-			return ip;
+		if (!in_sched_functions(ip)) {
+			ret = ip;
+			goto out;
+		}
 		fp = READ_ONCE(*(unsigned long *)fp);
 	} while (count++ < 16 && p->state != TASK_RUNNING);
-	return 0;
+
+out:
+	kasan_enable_current();
+	return ret;
 }
-- 
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]

#1239468

On Mon, Oct 05, 2015 at 01:28:26PM +0300, Andrey Ryabinin 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 kasan_disable_current()/kasan_enable_current() to silence
> these warnings.

So we're going to be sprinkling those around code which kasan doesn't
like? Can't we do better, without touching all code everywhere?

Probably not but let me ask that just in case...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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]

#1239473

* 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 kasan_disable_current()/kasan_enable_current() to silence
> these warnings.
> 
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> 
>  Perhaps it would be better to add something like this:
> 	READ_ONCE_NOCHECK()
> 	{
> 		kasan_disable_current();
> 		READ_ONCE();
> 		kasan_enable_current();
> 	}
>   ?
> 
>  arch/x86/kernel/process.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 39e585a..0488eb9 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -13,6 +13,7 @@
>  #include <linux/random.h>
>  #include <linux/user-return-notifier.h>
>  #include <linux/dmi.h>
> +#include <linux/kasan.h>
>  #include <linux/utsname.h>
>  #include <linux/stackprotector.h>
>  #include <linux/tick.h>
> @@ -514,7 +515,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>   */
>  unsigned long get_wchan(struct task_struct *p)
>  {
> -	unsigned long start, bottom, top, sp, fp, ip;
> +	unsigned long start, bottom, top, sp, fp, ip, ret = 0;
>  	int count = 0;
>  
>  	if (!p || p == current || p->state == TASK_RUNNING)
> @@ -550,14 +551,21 @@ unsigned long get_wchan(struct task_struct *p)
>  	if (sp < bottom || sp > top)
>  		return 0;
>  
> +	kasan_disable_current();
>  	fp = READ_ONCE(*(unsigned long *)sp);
>  	do {
>  		if (fp < bottom || fp > top)
> -			return 0;
> +			goto out;

a break would do just fine too.

> +
>  		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
> -		if (!in_sched_functions(ip))
> -			return ip;
> +		if (!in_sched_functions(ip)) {
> +			ret = ip;
> +			goto out;

ditto.

> +		}
>  		fp = READ_ONCE(*(unsigned long *)fp);
>  	} while (count++ < 16 && p->state != TASK_RUNNING);
> -	return 0;
> +
> +out:

and then the label would not be needed.

> +	kasan_enable_current();
> +	return ret;

But that's all pretty disgusting really.

Cannot we do better, such as annotating the function and then KASAN sorting out 
its false positives, or something like that?

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]

#1239483

On Mon, Oct 5, 2015 at 1:23 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 kasan_disable_current()/kasan_enable_current() to silence
>> these warnings.
>>
>> Reported-by: Sasha Levin <sasha.levin@oracle.com>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>>
>>  Perhaps it would be better to add something like this:
>>       READ_ONCE_NOCHECK()
>>       {
>>               kasan_disable_current();
>>               READ_ONCE();
>>               kasan_enable_current();
>>       }
>>   ?
>>
>>  arch/x86/kernel/process.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 39e585a..0488eb9 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/random.h>
>>  #include <linux/user-return-notifier.h>
>>  #include <linux/dmi.h>
>> +#include <linux/kasan.h>
>>  #include <linux/utsname.h>
>>  #include <linux/stackprotector.h>
>>  #include <linux/tick.h>
>> @@ -514,7 +515,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>>   */
>>  unsigned long get_wchan(struct task_struct *p)
>>  {
>> -     unsigned long start, bottom, top, sp, fp, ip;
>> +     unsigned long start, bottom, top, sp, fp, ip, ret = 0;
>>       int count = 0;
>>
>>       if (!p || p == current || p->state == TASK_RUNNING)
>> @@ -550,14 +551,21 @@ unsigned long get_wchan(struct task_struct *p)
>>       if (sp < bottom || sp > top)
>>               return 0;
>>
>> +     kasan_disable_current();
>>       fp = READ_ONCE(*(unsigned long *)sp);
>>       do {
>>               if (fp < bottom || fp > top)
>> -                     return 0;
>> +                     goto out;
>
> a break would do just fine too.
>
>> +
>>               ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>> -             if (!in_sched_functions(ip))
>> -                     return ip;
>> +             if (!in_sched_functions(ip)) {
>> +                     ret = ip;
>> +                     goto out;
>
> ditto.
>
>> +             }
>>               fp = READ_ONCE(*(unsigned long *)fp);
>>       } while (count++ < 16 && p->state != TASK_RUNNING);
>> -     return 0;
>> +
>> +out:
>
> and then the label would not be needed.
>
>> +     kasan_enable_current();
>> +     return ret;
>
> But that's all pretty disgusting really.
>
> Cannot we do better, such as annotating the function and then KASAN sorting out
> its false positives, or something like that?


We also plug __attribute__((no_sanitize_address)) on the function.
--
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]

#1239484

On 10/05/2015 02:39 PM, Dmitry Vyukov wrote:
>>
>> But that's all pretty disgusting really.
>>
>> Cannot we do better, such as annotating the function and then KASAN sorting out
>> its false positives, or something like that?
> 
> 
> We also plug __attribute__((no_sanitize_address)) on the function.
> 

It's absolutely unusable:( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
If we add it here, it won't built because of: '__always_inline __read_once_size()'

But, I think I have the solution.
We could have some blacklist - list of function names which we should be ignored.
In kasan_report() we could resolve return address to function name and compare it with name in list.
If name in list -> ignore report.


--
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]

#1239545

On Mon, Oct 05, 2015 at 02:46:30PM +0300, Andrey Ryabinin wrote:
> It's absolutely unusable:( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> If we add it here, it won't built because of: '__always_inline __read_once_size()'
> 
> But, I think I have the solution.
> We could have some blacklist - list of function names which we should be ignored.
> In kasan_report() we could resolve return address to function name and compare it with name in list.
> If name in list -> ignore report.

Sounds better. :)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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]

#1239837

On Mon, Oct 5, 2015 at 6:15 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Oct 05, 2015 at 02:46:30PM +0300, Andrey Ryabinin wrote:
>> It's absolutely unusable:( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
>> If we add it here, it won't built because of: '__always_inline __read_once_size()'
>>
>> But, I think I have the solution.
>> We could have some blacklist - list of function names which we should be ignored.
>> In kasan_report() we could resolve return address to function name and compare it with name in list.
>> If name in list -> ignore report.
>
> Sounds better. :)

These ought to be rare.  probe_kernel_xyz are hopefully already
covered, which leaves strange accesses to things that are known to be
out of bounds, which ought to be just the memory allocation stuff
(already covered?) and stack walking code (this stuff).

I think that READ_ONCE_NOCHECK is a decent solution that isn't too messy.

--Andy
--
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]

#1239730

> But, I think I have the solution.
> We could have some blacklist - list of function names which we should be ignored.
> In kasan_report() we could resolve return address to function name and compare it with name in list.
> If name in list -> ignore report.

I think annotating statements is cleaner than functions, even if it
is more code. Much better documentation

But if you really want to annotate on the function level:

It's better to annotate the function directly than some hidden away list.
This way there is some indication that there are races in there, which is
generally useful documentation.

__racy_function or similar.

Also central lists are generally annoying as they cause patch conflicts.

If disabling with an attribute doesn't work, you could put it into a special section
with __attribute__((section ...)) and check the start/end symbol before reporting. 
That's how kprobes solves similar issues. It also has the advantage
that it stops inlining.

-Andi
--
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]

#1239748

On 10/05/2015 07:39 PM, Andi Kleen wrote:
>> But, I think I have the solution.
>> We could have some blacklist - list of function names which we should be ignored.
>> In kasan_report() we could resolve return address to function name and compare it with name in list.
>> If name in list -> ignore report.
> 
> I think annotating statements is cleaner than functions, even if it
> is more code. Much better documentation
> 

I agree with that, that's why I suggested to add READ_ONCE_NOCHECK():
	READ_ONCE_NOCHECK()
	{
		kasan_disable_current();
		READ_ONCE();
		kasan_enable_current();
	}

Anywone objects?

> But if you really want to annotate on the function level:
> 
> It's better to annotate the function directly than some hidden away list.
> This way there is some indication that there are races in there, which is
> generally useful documentation.
> 
> __racy_function or similar.
> 
> Also central lists are generally annoying as they cause patch conflicts.
> 
> If disabling with an attribute doesn't work, you could put it into a special section
> with __attribute__((section ...)) and check the start/end symbol before reporting. 
> That's how kprobes solves similar issues. It also has the advantage
> that it stops inlining.


Yes, it might be better. Although, because of broken -fconserve-stack, this may
not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533
Function splitter may split original function into two parts and it always puts one split
part in default .text section.

--
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]

#1239843

> Yes, it might be better. Although, because of broken -fconserve-stack, this may
> not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533
> Function splitter may split original function into two parts and it always puts one split
> part in default .text section.

Interesting.

I guess could just add noinline too. It only happens with inlining, right?

Probably that needs to be added to all the existing section users too,
like __kprobes.

-Andi
--
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]

#1240205

* Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:

> On 10/05/2015 07:39 PM, Andi Kleen wrote:
> >> But, I think I have the solution.
> >> We could have some blacklist - list of function names which we should be ignored.
> >> In kasan_report() we could resolve return address to function name and compare it with name in list.
> >> If name in list -> ignore report.
> > 
> > I think annotating statements is cleaner than functions, even if it
> > is more code. Much better documentation
> > 
> 
> I agree with that, that's why I suggested to add READ_ONCE_NOCHECK():
> 	READ_ONCE_NOCHECK()
> 	{
> 		kasan_disable_current();
> 		READ_ONCE();
> 		kasan_enable_current();
> 	}
> 
> Anywone objects?

Sounds good to me! As long as it's hidden from plain .c files I'm a happy camper.

This should probably also be faster for KASAN than triggering a warning and having 
to parse a blacklist, right?

> > If disabling with an attribute doesn't work, you could put it into a special 
> > section with __attribute__((section ...)) and check the start/end symbol 
> > before reporting. That's how kprobes solves similar issues. It also has the 
> > advantage that it stops inlining.
> 
> Yes, it might be better. Although, because of broken -fconserve-stack, this may 
> not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533 
> Function splitter may split original function into two parts and it always puts 
> one split part in default .text section.

We do a _ton_ of such section tricks in the kernel (all of exception handling is 
based on that) - if that's broken by -fconserve-stack then the kernel is broken 
much more widely.

So unless KASAN wants to do something special here you can rely on sections just 
fine.

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]

#1240215

On 10/06/2015 10:26 AM, Ingo Molnar wrote:
> 
> * Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> 
>> On 10/05/2015 07:39 PM, Andi Kleen wrote:
>>>> But, I think I have the solution.
>>>> We could have some blacklist - list of function names which we should be ignored.
>>>> In kasan_report() we could resolve return address to function name and compare it with name in list.
>>>> If name in list -> ignore report.
>>>
>>> I think annotating statements is cleaner than functions, even if it
>>> is more code. Much better documentation
>>>
>>
>> I agree with that, that's why I suggested to add READ_ONCE_NOCHECK():
>> 	READ_ONCE_NOCHECK()
>> 	{
>> 		kasan_disable_current();
>> 		READ_ONCE();
>> 		kasan_enable_current();
>> 	}
>>
>> Anywone objects?
> 
> Sounds good to me! As long as it's hidden from plain .c files I'm a happy camper.
> 
> This should probably also be faster for KASAN than triggering a warning and having 
> to parse a blacklist, right?
> 

Sure.

>>> If disabling with an attribute doesn't work, you could put it into a special 
>>> section with __attribute__((section ...)) and check the start/end symbol 
>>> before reporting. That's how kprobes solves similar issues. It also has the 
>>> advantage that it stops inlining.
>>
>> Yes, it might be better. Although, because of broken -fconserve-stack, this may 
>> not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533 
>> Function splitter may split original function into two parts and it always puts 
>> one split part in default .text section.
> 
> We do a _ton_ of such section tricks in the kernel (all of exception handling is 
> based on that) - if that's broken by -fconserve-stack then the kernel is broken 
> much more widely.
> 

I'm mistaken here. It was broken once, at some point of development of gcc 5, but this was fixed
eventually. I just checked gcc 5.2, 4.9.2, 4.8.4, all of them are ok.

> So unless KASAN wants to do something special here you can rely on sections just 
> fine.
> 
> 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]

#1240842

On Tue, Oct 6, 2015 at 12:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>> On 10/05/2015 07:39 PM, Andi Kleen wrote:
>> >> But, I think I have the solution.
>> >> We could have some blacklist - list of function names which we should be ignored.
>> >> In kasan_report() we could resolve return address to function name and compare it with name in list.
>> >> If name in list -> ignore report.
>> >
>> > I think annotating statements is cleaner than functions, even if it
>> > is more code. Much better documentation
>> >
>>
>> I agree with that, that's why I suggested to add READ_ONCE_NOCHECK():
>>       READ_ONCE_NOCHECK()
>>       {
>>               kasan_disable_current();
>>               READ_ONCE();
>>               kasan_enable_current();
>>       }
>>
>> Anywone objects?
>
> Sounds good to me! As long as it's hidden from plain .c files I'm a happy camper.
>
> This should probably also be faster for KASAN than triggering a warning and having
> to parse a blacklist, right?
>
>> > If disabling with an attribute doesn't work, you could put it into a special
>> > section with __attribute__((section ...)) and check the start/end symbol
>> > before reporting. That's how kprobes solves similar issues. It also has the
>> > advantage that it stops inlining.
>>
>> Yes, it might be better. Although, because of broken -fconserve-stack, this may
>> not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533
>> Function splitter may split original function into two parts and it always puts
>> one split part in default .text section.
>
> We do a _ton_ of such section tricks in the kernel (all of exception handling is
> based on that) - if that's broken by -fconserve-stack then the kernel is broken
> much more widely.
>
> So unless KASAN wants to do something special here you can rely on sections just
> fine.

Kprobes is moving away from a section approach for some reason (not
sure why), but the kprobe approach should work, too.

But what's wrong with the GCC attribute mechanism?  Surely GCC ought
to be able to generate the code, at least in the simple cases, and the
attribute already exists.  The attribute and READ_ONCE_NOCHECK seem
like the least messy in the C code.

--Andy
--
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]

#1241192

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Tue, Oct 6, 2015 at 12:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> >
> >> On 10/05/2015 07:39 PM, Andi Kleen wrote:
> >> >> But, I think I have the solution.
> >> >> We could have some blacklist - list of function names which we should be ignored.
> >> >> In kasan_report() we could resolve return address to function name and compare it with name in list.
> >> >> If name in list -> ignore report.
> >> >
> >> > I think annotating statements is cleaner than functions, even if it
> >> > is more code. Much better documentation
> >> >
> >>
> >> I agree with that, that's why I suggested to add READ_ONCE_NOCHECK():
> >>       READ_ONCE_NOCHECK()
> >>       {
> >>               kasan_disable_current();
> >>               READ_ONCE();
> >>               kasan_enable_current();
> >>       }
> >>
> >> Anywone objects?
> >
> > Sounds good to me! As long as it's hidden from plain .c files I'm a happy camper.
> >
> > This should probably also be faster for KASAN than triggering a warning and having
> > to parse a blacklist, right?
> >
> >> > If disabling with an attribute doesn't work, you could put it into a special
> >> > section with __attribute__((section ...)) and check the start/end symbol
> >> > before reporting. That's how kprobes solves similar issues. It also has the
> >> > advantage that it stops inlining.
> >>
> >> Yes, it might be better. Although, because of broken -fconserve-stack, this may
> >> not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533
> >> Function splitter may split original function into two parts and it always puts
> >> one split part in default .text section.
> >
> > We do a _ton_ of such section tricks in the kernel (all of exception handling is
> > based on that) - if that's broken by -fconserve-stack then the kernel is broken
> > much more widely.
> >
> > So unless KASAN wants to do something special here you can rely on sections just
> > fine.
> 
> Kprobes is moving away from a section approach for some reason (not
> sure why), but the kprobe approach should work, too.

Do you mean NOKPROBE_SYMBOL() vs __kprobes?

So one concern is with functions being in multiple blacklists, so yeah, the 
NOKPROBE_SYMBOL() approach might be more robust than __kprobes.

But note that NOKPROBE_SYMBOL() itself is still section based:

#define __NOKPROBE_SYMBOL(fname)                        \
static unsigned long __used                             \
        __attribute__((section("_kprobe_blacklist")))   \
        _kbl_addr_##fname = (unsigned long)fname;

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]

#1241246

On 10/06/2015 09:11 PM, Andy Lutomirski wrote:
> 
> But what's wrong with the GCC attribute mechanism?  Surely GCC ought
> to be able to generate the code, at least in the simple cases, and the
> attribute already exists.  The attribute and READ_ONCE_NOCHECK seem
> like the least messy in the C code.

The problem with 'no_sanitize_address' attribute is incompatibility with inlining. 
GCC can't inline function with that attribute into function without it.
And the contrary is also true - GCC can't inline function without attribute into function with such attribute.

Failure to inline always_inline function leads to build failure.
And under CONFIG_OPTIMIZE=n 'inline' means 'always_inline'.

include/linux/compiler-gcc.h:

#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
    !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
#define inline		inline		__attribute__((always_inline)) notrace





--
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]

#1241275

On 10/07/2015 11:54 AM, Andrey Ryabinin wrote:
> On 10/06/2015 09:11 PM, Andy Lutomirski wrote:
>>
>> But what's wrong with the GCC attribute mechanism?  Surely GCC ought
>> to be able to generate the code, at least in the simple cases, and the
>> attribute already exists.  The attribute and READ_ONCE_NOCHECK seem
>> like the least messy in the C code.
> 
> The problem with 'no_sanitize_address' attribute is incompatibility with inlining. 
> GCC can't inline function with that attribute into function without it.
> And the contrary is also true - GCC can't inline function without attribute into function with such attribute.
> 
> Failure to inline always_inline function leads to build failure.
> And under CONFIG_OPTIMIZE=n 'inline' means 'always_inline'.
> 
> include/linux/compiler-gcc.h:
> 
> #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||		\
>     !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
> #define inline		inline		__attribute__((always_inline)) notrace
> 

Huh, 'inline' effectively means 'always_inline' on every arch, except x86.
This looks like a bug IMO.
Allowing gcc to uninline functions marked 'inline' could be beneficial for some arches/configs.

$ git grep ARCH_SUPPORTS_OPTIMIZED_INLINING
	arch/tile/Kconfig:config ARCH_SUPPORTS_OPTIMIZED_INLINING
	arch/x86/Kconfig:config ARCH_SUPPORTS_OPTIMIZED_INLINING
	include/linux/compiler-gcc.h:#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||           \

$ git grep OPTIMIZE_INLINING
	arch/x86/Kconfig.debug:config OPTIMIZE_INLINING
	arch/x86/configs/i386_defconfig:CONFIG_OPTIMIZE_INLINING=y
	arch/x86/configs/x86_64_defconfig:CONFIG_OPTIMIZE_INLINING=y
	arch/x86/entry/vdso/vdso32/vclock_gettime.c:#undef CONFIG_OPTIMIZE_INLINING
	include/linux/compiler-gcc.h:    !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
	kernel/configs/tiny.config:CONFIG_OPTIMIZE_INLINING=y
--
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]

#1241666

On Wed, Oct 07, 2015 at 11:54:42AM +0300, Andrey Ryabinin wrote:
> On 10/06/2015 09:11 PM, Andy Lutomirski wrote:
> > 
> > But what's wrong with the GCC attribute mechanism?  Surely GCC ought
> > to be able to generate the code, at least in the simple cases, and the
> > attribute already exists.  The attribute and READ_ONCE_NOCHECK seem
> > like the least messy in the C code.
> 
> The problem with 'no_sanitize_address' attribute is incompatibility with inlining. 
> GCC can't inline function with that attribute into function without it.
> And the contrary is also true - GCC can't inline function without attribute into function with such attribute.
> 
> Failure to inline always_inline function leads to build failure.

So just don't do that?  Don't set the attribute on functions marked inline.
Where do you see this anyways?


-Andi
--
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]

#1241729

2015-10-07 19:27 GMT+03:00 Andi Kleen <ak@linux.intel.com>:
> On Wed, Oct 07, 2015 at 11:54:42AM +0300, Andrey Ryabinin wrote:
>> On 10/06/2015 09:11 PM, Andy Lutomirski wrote:
>> >
>> > But what's wrong with the GCC attribute mechanism?  Surely GCC ought
>> > to be able to generate the code, at least in the simple cases, and the
>> > attribute already exists.  The attribute and READ_ONCE_NOCHECK seem
>> > like the least messy in the C code.
>>
>> The problem with 'no_sanitize_address' attribute is incompatibility with inlining.
>> GCC can't inline function with that attribute into function without it.
>> And the contrary is also true - GCC can't inline function without attribute into function with such attribute.
>>
>> Failure to inline always_inline function leads to build failure.
>
> So just don't do that?  Don't set the attribute on functions marked inline.
> Where do you see this anyways?

Besides that we can't set the attribute on functions that *call*
inline functions.
So we can't set it on get_wchan() because it calls __read_once_size().



>
> -Andi
--
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