From: Lai Jiangshan <[email protected]>
traps.c handles works for entries. It is very close to the code in
arch/x86/entry/. So we move traps.c to arch/x86/entry/.
It is also prepared for later patches that implements C version of
the entry code in traps.c.
Signed-off-by: Lai Jiangshan <[email protected]>
---
arch/x86/entry/Makefile | 5 ++++-
arch/x86/{kernel => entry}/traps.c | 0
arch/x86/kernel/Makefile | 5 +----
3 files changed, 5 insertions(+), 5 deletions(-)
rename arch/x86/{kernel => entry}/traps.c (100%)
diff --git a/arch/x86/entry/Makefile b/arch/x86/entry/Makefile
index 7fec5dcf6438..4e26248cea33 100644
--- a/arch/x86/entry/Makefile
+++ b/arch/x86/entry/Makefile
@@ -11,8 +11,11 @@ CFLAGS_REMOVE_common.o = $(CC_FLAGS_FTRACE)
CFLAGS_common.o += -fno-stack-protector
+CFLAGS_REMOVE_traps.o = -fstack-protector -fstack-protector-strong
+CFLAGS_traps.o += -fno-stack-protector
+
obj-y := entry_$(BITS).o thunk_$(BITS).o syscall_$(BITS).o
-obj-y += common.o
+obj-y += common.o traps.o
obj-y += vdso/
obj-y += vsyscall/
diff --git a/arch/x86/kernel/traps.c b/arch/x86/entry/traps.c
similarity index 100%
rename from arch/x86/kernel/traps.c
rename to arch/x86/entry/traps.c
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 21aa164cece2..5ba80522f5da 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -48,14 +48,11 @@ KCOV_INSTRUMENT := n
CFLAGS_head$(BITS).o += -fno-stack-protector
-CFLAGS_REMOVE_traps.o = -fstack-protector -fstack-protector-strong
-CFLAGS_traps.o += -fno-stack-protector
-
CFLAGS_irq.o := -I $(srctree)/$(src)/../include/asm/trace
obj-y := process_$(BITS).o signal.o
obj-$(CONFIG_COMPAT) += signal_compat.o
-obj-y += traps.o idt.o irq.o irq_$(BITS).o dumpstack_$(BITS).o
+obj-y += idt.o irq.o irq_$(BITS).o dumpstack_$(BITS).o
obj-y += time.o ioport.o dumpstack.o nmi.o
obj-$(CONFIG_MODIFY_LDT_SYSCALL) += ldt.o
obj-y += setup.o x86_init.o i8259.o irqinit.o
--
2.19.1.6.gb485710b
On 2021/9/2 16:09, Joerg Roedel wrote:
> On Wed, Sep 01, 2021 at 01:50:03AM +0800, Lai Jiangshan wrote:
>> arch/x86/entry/Makefile | 5 ++++-
>> arch/x86/{kernel => entry}/traps.c | 0
>> arch/x86/kernel/Makefile | 5 +----
>> 3 files changed, 5 insertions(+), 5 deletions(-)
>> rename arch/x86/{kernel => entry}/traps.c (100%)
>
> From looking over the patch-set I didn't spot the reason for putting the
> entry C code into traps.c. Can you explain that please? Otherwise I'd
> prefer to create a new file, like arch/x86/entry/entry64.c.
I agree, and I think Peter is handling it.
Thanks
Lai
On Wed, Sep 01, 2021 at 01:50:03AM +0800, Lai Jiangshan wrote:
> arch/x86/entry/Makefile | 5 ++++-
> arch/x86/{kernel => entry}/traps.c | 0
> arch/x86/kernel/Makefile | 5 +----
> 3 files changed, 5 insertions(+), 5 deletions(-)
> rename arch/x86/{kernel => entry}/traps.c (100%)
From looking over the patch-set I didn't spot the reason for putting the
entry C code into traps.c. Can you explain that please? Otherwise I'd
prefer to create a new file, like arch/x86/entry/entry64.c.
Thanks,
Joerg
On Thu, Sep 02, 2021 at 05:21:51PM +0800, Lai Jiangshan wrote:
>
>
> On 2021/9/2 16:09, Joerg Roedel wrote:
> > On Wed, Sep 01, 2021 at 01:50:03AM +0800, Lai Jiangshan wrote:
> > > arch/x86/entry/Makefile | 5 ++++-
> > > arch/x86/{kernel => entry}/traps.c | 0
> > > arch/x86/kernel/Makefile | 5 +----
> > > 3 files changed, 5 insertions(+), 5 deletions(-)
> > > rename arch/x86/{kernel => entry}/traps.c (100%)
> >
> > From looking over the patch-set I didn't spot the reason for putting the
> > entry C code into traps.c. Can you explain that please? Otherwise I'd
> > prefer to create a new file, like arch/x86/entry/entry64.c.
>
>
> I agree, and I think Peter is handling it.
I don't think I said that. I said I like the patches but there's a lot
of scary code and details to review, which takes time.
I've now done a second reading of the patches and provided some more
feedback, but I'm very sure I didn't get to the scary details yet.
One thing that got pointed out (by Andrew Cooper) is that by moving the
whole SWAPGS trainwreck to C it becomes entirely 'trivial' to sneak in
an 'accidental' per-cpu reference before having done the SWAPGS dance.
I'm myself not (yet) convinced that's a good enough reason to leave it
in ASM, but it does certainly need consideration.
On 2021/9/2 18:50, Peter Zijlstra wrote:
> On Thu, Sep 02, 2021 at 05:21:51PM +0800, Lai Jiangshan wrote:
>>
>>
>> On 2021/9/2 16:09, Joerg Roedel wrote:
>>> On Wed, Sep 01, 2021 at 01:50:03AM +0800, Lai Jiangshan wrote:
>>>> arch/x86/entry/Makefile | 5 ++++-
>>>> arch/x86/{kernel => entry}/traps.c | 0
>>>> arch/x86/kernel/Makefile | 5 +----
>>>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>>> rename arch/x86/{kernel => entry}/traps.c (100%)
>>>
>>> From looking over the patch-set I didn't spot the reason for putting the
>>> entry C code into traps.c. Can you explain that please? Otherwise I'd
>>> prefer to create a new file, like arch/x86/entry/entry64.c.
>>
>>
>> I agree, and I think Peter is handling it.
>
> I don't think I said that. I said I like the patches but there's a lot
> of scary code and details to review, which takes time.
>
> I've now done a second reading of the patches and provided some more
> feedback, but I'm very sure I didn't get to the scary details yet.
>
> One thing that got pointed out (by Andrew Cooper) is that by moving the
> whole SWAPGS trainwreck to C it becomes entirely 'trivial' to sneak in
> an 'accidental' per-cpu reference before having done the SWAPGS dance.
>
> I'm myself not (yet) convinced that's a good enough reason to leave it
> in ASM, but it does certainly need consideration.
>
It is real concern and it proves that my having put the C code in traps.c
was totally wrong.
To relieve the concern, I think the C code can be put into a single file, like
arch/x86/entry/entry64.c, and be documented that it is as critical, dangerous as
entry_64.S and any one should take no less care on modifying/reviewing it than on
modifying/reviewing entry_64.S.
And all the other users of native_swapgs can be moved to this file too, such as
__[rd|wr]gsbase_inactive().
A noninstr function can sometimes have 'accidental' instrument to sneak in.
For example, stack-protector is instrumenting many noninstr functions now
if the CONFIG is yes. It is normally Ok and gcc is adding per-function control
on it.
But the C code can not be instrumented by any way. For example stack-protector
would add per-cpu reference before having done the SWAPGS dance.) Entry C code
required a stronger limitation than noninstr code.
By the way, can objtool check the per-cpu reference?
On Thu, Sep 02, 2021 at 07:54:25PM +0800, Lai Jiangshan wrote:
> For example, stack-protector is instrumenting many noninstr functions now
> if the CONFIG is yes. It is normally Ok and gcc is adding per-function control
> on it.
IIRC the latest compiler have an attribute for this too, that really
should be added to noinstr. Lemme go find.
On Thu, Sep 02, 2021 at 02:05:41PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 02, 2021 at 07:54:25PM +0800, Lai Jiangshan wrote:
> > For example, stack-protector is instrumenting many noninstr functions now
> > if the CONFIG is yes. It is normally Ok and gcc is adding per-function control
> > on it.
>
> IIRC the latest compiler have an attribute for this too, that really
> should be added to noinstr. Lemme go find.
Something like so... Nick ?
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 49b0ac8b6fd3..6a15c3d8df5c 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -62,6 +62,12 @@
#define __no_sanitize_coverage
#endif
+#if defined(CONFIG_STACKPROTECTOR)
+#define __no_stack_protector __attribute__((no_stack_protector))
+#else
+#define __no_stack_protector
+#endif
+
/*
* Not all versions of clang implement the type-generic versions
* of the builtin overflow checkers. Fortunately, clang implements
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index cb9217fc60af..7ac0a3f11ba9 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -128,6 +128,12 @@
#define __no_sanitize_coverage
#endif
+#if defined(CONFIG_STACKPROTECTOR) && __has_attribute__(__no_stack_protector__)
+#define __no_stack_protector __attribute__((no_stack_protector))
+#else
+#define __no_stack_protector
+#endif
+
#if GCC_VERSION >= 50100
#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
#endif
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index e4ea86fc584d..5ae1c08dba8d 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -210,7 +210,8 @@ struct ftrace_likely_data {
/* Section for code which can't be instrumented at all */
#define noinstr \
noinline notrace __attribute((__section__(".noinstr.text"))) \
- __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage
+ __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage \
+ __no_stack_protector
#endif /* __KERNEL__ */
On Thu, Sep 2, 2021 at 7:05 PM Nick Desaulniers <[email protected]> wrote:
>
> The above 2 hunks should go in include/linux/compiler_attributes.h,
> but yes. I'd been meaning to send such a patch; it's nice to have
Note that `compiler_attributes.h` does not keep attributes that depend
on config options.
On one hand, I feel we should put them there. On the other hand, I
want to avoid making a mess again since the purpose of the file is to
keep things clean for the majority of attributes.
Perhaps we should do a middle-ground, and only allow those that depend
on a single config option, i.e. no nesting `#if`s or complex
conditions.
Cheers,
Miguel
On Thu, Sep 2, 2021 at 6:36 AM Peter Zijlstra <[email protected]> wrote:
>
> On Thu, Sep 02, 2021 at 02:05:41PM +0200, Peter Zijlstra wrote:
> > On Thu, Sep 02, 2021 at 07:54:25PM +0800, Lai Jiangshan wrote:
> > > For example, stack-protector is instrumenting many noninstr functions now
> > > if the CONFIG is yes. It is normally Ok and gcc is adding per-function control
> > > on it.
> >
> > IIRC the latest compiler have an attribute for this too, that really
> > should be added to noinstr. Lemme go find.
>
> Something like so... Nick ?
>
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 49b0ac8b6fd3..6a15c3d8df5c 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -62,6 +62,12 @@
> #define __no_sanitize_coverage
> #endif
>
> +#if defined(CONFIG_STACKPROTECTOR)
> +#define __no_stack_protector __attribute__((no_stack_protector))
> +#else
> +#define __no_stack_protector
> +#endif
> +
> /*
> * Not all versions of clang implement the type-generic versions
> * of the builtin overflow checkers. Fortunately, clang implements
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index cb9217fc60af..7ac0a3f11ba9 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -128,6 +128,12 @@
> #define __no_sanitize_coverage
> #endif
>
> +#if defined(CONFIG_STACKPROTECTOR) && __has_attribute__(__no_stack_protector__)
> +#define __no_stack_protector __attribute__((no_stack_protector))
> +#else
> +#define __no_stack_protector
> +#endif
> +
The above 2 hunks should go in include/linux/compiler_attributes.h,
but yes. I'd been meaning to send such a patch; it's nice to have
finer grain function-level control over -fno-stack-protector which
significantly hurts ergonomics for things like LTO. IIRC GCC only
added the attribute recently in the 10.X release, so it might be too
new to rely on quite yet.
> #if GCC_VERSION >= 50100
> #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> #endif
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index e4ea86fc584d..5ae1c08dba8d 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -210,7 +210,8 @@ struct ftrace_likely_data {
> /* Section for code which can't be instrumented at all */
> #define noinstr \
> noinline notrace __attribute((__section__(".noinstr.text"))) \
> - __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage
> + __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage \
> + __no_stack_protector
>
> #endif /* __KERNEL__ */
>
--
Thanks,
~Nick Desaulniers
On Thu, Sep 2, 2021 at 10:19 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, Sep 2, 2021 at 7:05 PM Nick Desaulniers <[email protected]> wrote:
> >
> > The above 2 hunks should go in include/linux/compiler_attributes.h,
> > but yes. I'd been meaning to send such a patch; it's nice to have
>
> Note that `compiler_attributes.h` does not keep attributes that depend
> on config options.
Sure, I'd drop the config check and define it conditionally on the
__has_attribute check alone. Does it hurt to mark functions as
__attribute__((no_stack_protector)) when we're not building with
-fstack-protector*? Nope!
> On one hand, I feel we should put them there. On the other hand, I
> want to avoid making a mess again since the purpose of the file is to
> keep things clean for the majority of attributes.
>
> Perhaps we should do a middle-ground, and only allow those that depend
> on a single config option, i.e. no nesting `#if`s or complex
> conditions.
--
Thanks,
~Nick Desaulniers
On 9/2/21 19:05, Nick Desaulniers wrote:
> IIRC GCC only
> added the attribute recently in the 10.X release, so it might be too
> new to rely on quite yet.
The no_stack_protector attribute was actually added in the GCC 11.x release:
https://gcc.gnu.org/gcc-11/changes.html
Note the compiler is definitely used by Fedora, openSUSE Tumbleweed
and other cutting edge distributions.
Cheers,
Martin
On Fri, Sep 3, 2021 at 12:36 AM Martin Liška <[email protected]> wrote:
>
> On 9/2/21 19:05, Nick Desaulniers wrote:
> > IIRC GCC only
> > added the attribute recently in the 10.X release, so it might be too
> > new to rely on quite yet.
>
> The no_stack_protector attribute was actually added in the GCC 11.x release:
> https://gcc.gnu.org/gcc-11/changes.html
Ah right, that lays more weight though that this feature is still too
new to rely on quite yet. Martin, do you know if what happens with
regards to inlining when the callee and caller mismatch on this
function attribute in GCC? This is very much a problem for the
kernel.
> Note the compiler is definitely used by Fedora, openSUSE Tumbleweed
> and other cutting edge distributions.
Kernel supports GCC 4.9+ currently. This feature can only be emulated
with the coarse grain -fno-stack-protector (or gnu_inline with out of
line assembler definition...:( ).
--
Thanks,
~Nick Desaulniers
On 9/7/21 23:12, Nick Desaulniers wrote:
> On Fri, Sep 3, 2021 at 12:36 AM Martin Liška <[email protected]> wrote:
>>
>> On 9/2/21 19:05, Nick Desaulniers wrote:
>>> IIRC GCC only
>>> added the attribute recently in the 10.X release, so it might be too
>>> new to rely on quite yet.
>>
>> The no_stack_protector attribute was actually added in the GCC 11.x release:
>> https://gcc.gnu.org/gcc-11/changes.html
>
> Ah right, that lays more weight though that this feature is still too
> new to rely on quite yet.
Sure.
> Martin, do you know if what happens with
> regards to inlining when the callee and caller mismatch on this
> function attribute in GCC? This is very much a problem for the
> kernel.
That's a know issue that was already discusses both in a Kernel mailing and
GCC bugzilla:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722#c9
Martin
>
>> Note the compiler is definitely used by Fedora, openSUSE Tumbleweed
>> and other cutting edge distributions.
>
> Kernel supports GCC 4.9+ currently. This feature can only be emulated
> with the coarse grain -fno-stack-protector (or gnu_inline with out of
> line assembler definition...:( ).
>