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

Groups > linux.kernel > #1240205 > unrolled thread

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

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

  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

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

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