Message ID | 20181022105944.GA1411@gmail.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | [GIT,PULL] Compiler Attributes for v4.20-rc1 | expand |
On Mon, Oct 22, 2018 at 3:59 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Here it is the Compiler Attributes series/tree, which tries to disentangle > the include/linux/compiler*.h headers and bring them up to date. I've finally emptied the "normal" pull queues, and am looking at the odd ones that I felt I needed to review more in-depth. This looked fine to me, and I already pulled, but when looking at the conflict for __no_sanitize_address_or_inline, I also noticed that you actually changed the gcc version logic. The logic for using __no_sanitize_address *used* to be #if GCC_VERSION >= 40902 but you have changed it to # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8) so now it's gcc-4.8+ rather than gcc-4.9.2+. I'm not sure how much that matters (maybe the original check for 4.9.2 was just a random pick by Andrey? Added to cc), but together with the movement to <linux/compiler_attributes.h> that looks like it also wouldn't want the CONFIG_KASAN tests, I wonder what the right merge resolution would be. Yes, I see the resolution in linux-next, and I think that one is odd and dubious, and now it *mixes* that old test of gcc-4.9.2 with the different test in compiler-attributes. I'm _inclined_ to just do #ifdef CONFIG_KASAN #define __no_sanitize_address_or_inline \ __no_sanitize_address __maybe_unused notrace #else #define __no_sanitize_address_or_inline inline #endif without any compiler versions at all, because I don't think it matters (maybe we won't get address sanitation, and we will not get inlining either, but do we care?). But I'm also unsure whether you meant for the "__has_attribute()" test to be usable outside the linux/compiler_attributes.h file, in which case I could just do #if defined(CONFIG_KASAN) && __has_attribute(__no_sanitize_address__) instead. End result: it's not the merge resolution itself that raises questions, it's just semantics in general. So I've dropped this for now, in the hope that you/Andrey can answer these questions. Linus
On Thu, Nov 1, 2018 at 6:06 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I'm not sure how much that matters (maybe the original check for 4.9.2 > was just a random pick by Andrey? Added to cc), but together with the > movement to <linux/compiler_attributes.h> that looks like it also > wouldn't want the CONFIG_KASAN tests, I wonder what the right merge > resolution would be. Good catch. I don't recall any special logic when I did that change, so most likely I simply did like for the rest of the attributes and took a look at when it was first supported (and documentation in gcc's docs) in order to implement __has_attribute by hand. But indeed, it *may* be that there is an (undocumented) problem between 4.8 <= gcc < 4.9.2 with it. If so, we should document it down and fix it. Andrey? > Yes, I see the resolution in linux-next, and I think that one is odd > and dubious, and now it *mixes* that old test of gcc-4.9.2 with the > different test in compiler-attributes. I missed that conflict completely, my bad (I did not miss all of them, at least; one required fixing). Hm.... at a quick look, why is it only on compiler-gcc.h? It should either have a corresponding #define elsewhere or just be put directly in another common header, no? (Adding Vasily & Martin to CC.) > But I'm also unsure whether you meant for the "__has_attribute()" test > to be usable outside the linux/compiler_attributes.h file, in which > case I could just do > > #if defined(CONFIG_KASAN) && __has_attribute(__no_sanitize_address__) > > instead. I think that (using __has_attribute() outside) may be a good idea: I wanted to keep compiler_attributes.h as simple as possible by avoiding #ifdefs inside that header (except for __has_attribute itself), as an attempt to avoid going back to the mess of #ifdefs we had previously. Basically, keeping the attributes in compiler_attributes.h that do not depend on complex logic. So using __has_attribute *outside* the header actually goes well with that principle, because it helps keeping stuff out of it if they depend on other config options; without having to rely on GCC_VERSION either. [By the way, in case it clarifies: note that "optional" in that file actually is a bit of a misnomer. I meant to say "optional" as in "not supported by all compilers, so conditionally defined" ("optionally" defined); rather than "optional" in the sense of "code still works without the attribute". It caught Rasmus in one of his patches sent a few days ago on top of this tree, so I want to change it or explain it to avoid confusion.] Cheers, Miguel
On Thu, Nov 1, 2018 at 10:06 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > The logic for using __no_sanitize_address *used* to be > > #if GCC_VERSION >= 40902 Ok, looking around, I think this has less to do with the attribute being recognized, and simply just being because KASAN itself wants gcc-4.9.2. I'm actually not seeing that KASAN dependency in the Kconfig scripts (and it probably _should_ be now that we can just add compiler version dependencies there), but that explains why the gcc version check is different from "gcc supports the attribute". Anyway, I decided to do the merge by just getting rid of the GCC_VERSION check around __no_sanitize_address_or_inline entirely. If you enable KASAN, then a function with that marking just won't be marked inline. End result: pulled. I'm as confused as you are as to why __no_sanitize_address_or_inline is in the gcc header, but I guess it ends up being the same issue: KASAN depends on gcc even if that dependency doesn't seem to be spelled out in lib/Kconfig.kasan. So I _think_ the KASAN config should have a depends on CC_IS_GCC && GCC_VERSION >= 40902 on it, but maybe there is something I'm missing. But from a pull standpoint, I don't want to mess with those (unrelated) issues, so I just kept the merge resolution as simple and straightforward as possible. Miguel, please do double-check the merge (it's not pushed out yet, I'm doing the usual build tests etc first). Linus
On 11/01/2018 08:06 PM, Linus Torvalds wrote: > On Mon, Oct 22, 2018 at 3:59 AM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: >> >> Here it is the Compiler Attributes series/tree, which tries to disentangle >> the include/linux/compiler*.h headers and bring them up to date. > > I've finally emptied the "normal" pull queues, and am looking at the > odd ones that I felt I needed to review more in-depth. > > This looked fine to me, and I already pulled, but when looking at the > conflict for __no_sanitize_address_or_inline, I also noticed that you > actually changed the gcc version logic. > > The logic for using __no_sanitize_address *used* to be > > #if GCC_VERSION >= 40902 > > but you have changed it to > > # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8) > > so now it's gcc-4.8+ rather than gcc-4.9.2+. As you said in other email - "this has less to do with the attribute being recognized, and simply just being because KASAN itself wants gcc-4.9.2." gcc < 4.9.2 simply doesn't have -fsanitize=kernel-address. But no_sanitize attribute originally comes with user space ASAN (-fsanitize=address) which exist in earlier GCCs like 4.8. Checking against 4.8 gcc should be fine. If we compile the kernel with gcc 4.8 it will be compiled without instrumentation, so the attribute won't have any effect. > > I'm not sure how much that matters (maybe the original check for 4.9.2 > was just a random pick by Andrey? Added to cc), but together with the > movement to <linux/compiler_attributes.h> that looks like it also > wouldn't want the CONFIG_KASAN tests, I wonder what the right merge > resolution would be. > > Yes, I see the resolution in linux-next, and I think that one is odd > and dubious, and now it *mixes* that old test of gcc-4.9.2 with the > different test in compiler-attributes. > > I'm _inclined_ to just do > > #ifdef CONFIG_KASAN > #define __no_sanitize_address_or_inline \ > __no_sanitize_address __maybe_unused notrace > #else > #define __no_sanitize_address_or_inline inline > #endif > > without any compiler versions at all, because I don't think it matters > (maybe we won't get address sanitation, and we will not get inlining > either, but do we care?). You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline shouldn't have been added in the first place, because we already have almost the same __no_kasan_or_inline: #ifdef CONFIG_KASAN /* * We can't declare function 'inline' because __no_sanitize_address confilcts * with inlining. Attempt to inline it may cause a build failure. * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 * '__maybe_unused' allows us to avoid defined-but-not-used warnings. */ # define __no_kasan_or_inline __no_sanitize_address __maybe_unused #else # define __no_kasan_or_inline __always_inline #endif There are small differences like absence of notrace, but notrace could be added to the function together with __no_kasan_or_inline. And inline is *not* redefined to __always_inline only on x86 when CONFIG_OPTIMIZE_INLINING=y So there is absolutely no reason to have both __no_kasan_or_inline and __no_sanitize_address_or_inline.
On Fri, Nov 2, 2018 at 2:52 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Anyway, I decided to do the merge by just getting rid of the > GCC_VERSION check around __no_sanitize_address_or_inline entirely. If > you enable KASAN, then a function with that marking just won't be > marked inline. I was a bit confused when reading the gcc bug reports, i.e. why gcc did *not* complain in 4.9 but it did in 5.1 (when it was supposed to complain also in 4.9). It turns out that gcc 5.1 takes into account who is the actual caller due to this change: + cgraph_node *caller = e->caller->global.inlined_to + ? e->caller->global.inlined_to : e->caller; ... - else if (!sanitize_attrs_match_for_inline_p (e->caller->decl, callee->decl)) + else if (!sanitize_attrs_match_for_inline_p (caller->decl, callee->decl)) change; e.g. this: #define really_inline inline __attribute__((always_inline)) #define no_sanitize __attribute__((no_sanitize_address)) really_inline void f() {} really_inline void g() { f(); } no_sanitize void h() { g(); } Complains in gcc 4.9 -O0, 5.1 -O0 and 5.1 -O2; but *not* in 4.9 -O2. https://godbolt.org/z/kNApqk Anyway, this is orthogonal but in case it clarifies that for someone else... > Miguel, please do double-check the merge (it's not pushed out yet, I'm > doing the usual build tests etc first). I was sleeping, didn't manage to see it (in your GitHub, I guess?). I did the merge myself, and arrived at the same thing as you. I quickly inspected the rest and seems fine. By the way, I spotted an extra space at: + * we do one or the other. Cheers, Miguel
On 11/02/2018 04:46 AM, Linus Torvalds wrote: > On Thu, Nov 1, 2018 at 10:06 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> The logic for using __no_sanitize_address *used* to be >> >> #if GCC_VERSION >= 40902 > > Ok, looking around, I think this has less to do with the attribute > being recognized, and simply just being because KASAN itself wants > gcc-4.9.2. > > I'm actually not seeing that KASAN dependency in the Kconfig scripts > (and it probably _should_ be now that we can just add compiler version > dependencies there), but that explains why the gcc version check is > different from "gcc supports the attribute". > > Anyway, I decided to do the merge by just getting rid of the > GCC_VERSION check around __no_sanitize_address_or_inline entirely. If > you enable KASAN, then a function with that marking just won't be > marked inline. > > End result: pulled. I'm as confused as you are as to why > __no_sanitize_address_or_inline is in the gcc header, but I guess it > ends up being the same issue: KASAN depends on gcc even if that > dependency doesn't seem to be spelled out in lib/Kconfig.kasan. > > So I _think_ the KASAN config should have a > > depends on CC_IS_GCC && GCC_VERSION >= 40902 > > on it, but maybe there is something I'm missing. > I'd rather use cc-option instead of version check, since we also support clang. It should be possible to express compiler requirements for subfeatures like stack/inline instrumentation in Kconfig. But that would be not that trivial, see the scripts/Makefile.kasan > But from a pull standpoint, I don't want to mess with those > (unrelated) issues, so I just kept the merge resolution as simple and > straightforward as possible. > > Miguel, please do double-check the merge (it's not pushed out yet, I'm > doing the usual build tests etc first). > > Linus >
On Fri, Nov 2, 2018 at 2:43 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline > shouldn't have been added in the first place, because we already have almost the same >__no_kasan_or_inline: Ahh, very good. Vasily, Martin - since __no_sanitize_address_or_inline was added just for s390, would you mind replacing it with __no_kasan_or_inline instead, and testing that in whatever failed before? Then we can just remove that unnecessary symbol #define entirely.. Thanks, Linus
On Fri, Nov 2, 2018 at 6:16 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > On 11/02/2018 04:46 AM, Linus Torvalds wrote: > > > > So I _think_ the KASAN config should have a > > > > depends on CC_IS_GCC && GCC_VERSION >= 40902 > > > > on it, but maybe there is something I'm missing. > > I'd rather use cc-option instead of version check, since we also support clang. That would be even better, but I thought the requirement for 4.9.2 came not from the option existing, but because of some bugs getting fixed? But if we can do it without version checks, that would be lovely. Linus
On 11/02/2018 07:11 PM, Linus Torvalds wrote: > On Fri, Nov 2, 2018 at 6:16 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: >> >> On 11/02/2018 04:46 AM, Linus Torvalds wrote: >>> >>> So I _think_ the KASAN config should have a >>> >>> depends on CC_IS_GCC && GCC_VERSION >= 40902 >>> >>> on it, but maybe there is something I'm missing. >> >> I'd rather use cc-option instead of version check, since we also support clang. > > That would be even better, but I thought the requirement for 4.9.2 > came not from the option existing, but because of some bugs getting > fixed? It looks unusual but -fsanitize=kernel-address really showed up only in 4.9.2. It was actually backported from 5.0 branch to 4.9 for whatever reason. > > But if we can do it without version checks, that would be lovely. > > Linus >
On Fri, 2 Nov 2018 09:09:32 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Nov 2, 2018 at 2:43 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > > > You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline > > shouldn't have been added in the first place, because we already have almost the same > >__no_kasan_or_inline: > > Ahh, very good. > > Vasily, Martin - since __no_sanitize_address_or_inline was added just > for s390, would you mind replacing it with __no_kasan_or_inline > instead, and testing that in whatever failed before? > > Then we can just remove that unnecessary symbol #define entirely.. Ok, will do.
On Mon, 5 Nov 2018 07:02:56 +0100 Martin Schwidefsky <schwidefsky@de.ibm.com> wrote: > On Fri, 2 Nov 2018 09:09:32 -0700 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > On Fri, Nov 2, 2018 at 2:43 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > > > > > You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline > > > shouldn't have been added in the first place, because we already have almost the same > > >__no_kasan_or_inline: > > > > Ahh, very good. > > > > Vasily, Martin - since __no_sanitize_address_or_inline was added just > > for s390, would you mind replacing it with __no_kasan_or_inline > > instead, and testing that in whatever failed before? > > > > Then we can just remove that unnecessary symbol #define entirely.. > > Ok, will do. Follow-up question: the __no_sanitize_address_or_inline define has the 'notrace' option that is missing for __no_kasan_or_inline. We need that option for __load_psw_mask, if we do the replacement all users of __no_kasan_or_inline would get the option as well. This affects __read_once_size_nocheck and read_word_at_a_time. Do these function have to be traceable ? This patch would work for me: -- From 4aaa09fe4b54e930edabac86606dee979b12647c Mon Sep 17 00:00:00 2001 From: Martin Schwidefsky <schwidefsky@de.ibm.com> Date: Mon, 5 Nov 2018 07:36:28 +0100 Subject: [PATCH] compiler: remove __no_sanitize_address_or_inline again The __no_sanitize_address_or_inline and __no_kasan_or_inline defines are almost identical. The only difference is that __no_kasan_or_inline does not have the 'notrace' attribute. To be able to replace __no_sanitize_address_or_inline with the older definition, add 'notrace' to __no_kasan_or_inline and change to two users of __no_sanitize_address_or_inline in the s390 code. The 'notrace' option is necessary for e.g. the __load_psw_mask function in arch/s390/include/asm/processor.h. Without the option it is possible to trace __load_psw_mask which leads to kernel stack overflow. Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> --- arch/s390/include/asm/processor.h | 4 ++-- include/linux/compiler-gcc.h | 12 ------------ include/linux/compiler.h | 2 +- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h index 302795c47c06..81038ab357ce 100644 --- a/arch/s390/include/asm/processor.h +++ b/arch/s390/include/asm/processor.h @@ -236,7 +236,7 @@ static inline unsigned long current_stack_pointer(void) return sp; } -static __no_sanitize_address_or_inline unsigned short stap(void) +static __no_kasan_or_inline unsigned short stap(void) { unsigned short cpu_address; @@ -330,7 +330,7 @@ static inline void __load_psw(psw_t psw) * Set PSW mask to specified value, while leaving the * PSW addr pointing to the next instruction. */ -static __no_sanitize_address_or_inline void __load_psw_mask(unsigned long mask) +static __no_kasan_or_inline void __load_psw_mask(unsigned long mask) { unsigned long addr; psw_t psw; diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index c0f5db3a9621..2010493e1040 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -143,18 +143,6 @@ #define KASAN_ABI_VERSION 3 #endif -/* - * Because __no_sanitize_address conflicts with inlining: - * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 - * we do one or the other. - */ -#ifdef CONFIG_KASAN -#define __no_sanitize_address_or_inline \ - __no_sanitize_address __maybe_unused notrace -#else -#define __no_sanitize_address_or_inline inline -#endif - #if GCC_VERSION >= 50100 #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 #endif diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 18c80cfa4fc4..06396c1cf127 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -189,7 +189,7 @@ void __read_once_size(const volatile void *p, void *res, int size) * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 * '__maybe_unused' allows us to avoid defined-but-not-used warnings. */ -# define __no_kasan_or_inline __no_sanitize_address __maybe_unused +# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused #else # define __no_kasan_or_inline __always_inline #endif
On Mon, 5 Nov 2018 14:15:35 +0100 Martin Schwidefsky <schwidefsky@de.ibm.com> wrote: > > Follow-up question: the __no_sanitize_address_or_inline define has the 'notrace' > option that is missing for __no_kasan_or_inline. We need that option for > __load_psw_mask, if we do the replacement all users of __no_kasan_or_inline > would get the option as well. This affects __read_once_size_nocheck and > read_word_at_a_time. Do these function have to be traceable ? Some history on why I added notrace to inline. It was to make things more consistent. Since gcc wont add a fentry/mcount call to inlined functions, having those functions show up in the trace depending on whether or not gcc honored the "inline" tag made things confusing. And it even once caused a crash when local_irq_save() stopped being inlined. I added notrace to inline so that if you mark something as inline, it would not show up in the trace regardless of gcc deciding to inline the function or not. > > This patch would work for me: > -- > >From 4aaa09fe4b54e930edabac86606dee979b12647c Mon Sep 17 00:00:00 2001 > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > Date: Mon, 5 Nov 2018 07:36:28 +0100 > Subject: [PATCH] compiler: remove __no_sanitize_address_or_inline again > > The __no_sanitize_address_or_inline and __no_kasan_or_inline defines > are almost identical. The only difference is that __no_kasan_or_inline > does not have the 'notrace' attribute. > > To be able to replace __no_sanitize_address_or_inline with the older > definition, add 'notrace' to __no_kasan_or_inline and change to two > users of __no_sanitize_address_or_inline in the s390 code. > > The 'notrace' option is necessary for e.g. the __load_psw_mask function > in arch/s390/include/asm/processor.h. Without the option it is possible > to trace __load_psw_mask which leads to kernel stack overflow. Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > --- > arch/s390/include/asm/processor.h | 4 ++-- > include/linux/compiler-gcc.h | 12 ------------ > include/linux/compiler.h | 2 +- > 3 files changed, 3 insertions(+), 15 deletions(-) > > diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h > index 302795c47c06..81038ab357ce 100644 > --- a/arch/s390/include/asm/processor.h > +++ b/arch/s390/include/asm/processor.h > @@ -236,7 +236,7 @@ static inline unsigned long current_stack_pointer(void) > return sp; > } > > -static __no_sanitize_address_or_inline unsigned short stap(void) > +static __no_kasan_or_inline unsigned short stap(void) > { > unsigned short cpu_address; > > @@ -330,7 +330,7 @@ static inline void __load_psw(psw_t psw) > * Set PSW mask to specified value, while leaving the > * PSW addr pointing to the next instruction. > */ > -static __no_sanitize_address_or_inline void __load_psw_mask(unsigned long mask) > +static __no_kasan_or_inline void __load_psw_mask(unsigned long mask) > { > unsigned long addr; > psw_t psw; > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index c0f5db3a9621..2010493e1040 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -143,18 +143,6 @@ > #define KASAN_ABI_VERSION 3 > #endif > > -/* > - * Because __no_sanitize_address conflicts with inlining: > - * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 > - * we do one or the other. > - */ > -#ifdef CONFIG_KASAN > -#define __no_sanitize_address_or_inline \ > - __no_sanitize_address __maybe_unused notrace > -#else > -#define __no_sanitize_address_or_inline inline > -#endif > - > #if GCC_VERSION >= 50100 > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > #endif > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index 18c80cfa4fc4..06396c1cf127 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -189,7 +189,7 @@ void __read_once_size(const volatile void *p, void *res, int size) > * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 > * '__maybe_unused' allows us to avoid defined-but-not-used warnings. > */ > -# define __no_kasan_or_inline __no_sanitize_address __maybe_unused > +# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused > #else > # define __no_kasan_or_inline __always_inline > #endif
On Mon, Nov 5, 2018 at 5:15 AM Martin Schwidefsky <schwidefsky@de.ibm.com> wrote: > > This patch would work for me: Thanks, applied, Linus