Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr()

[Uros Bizjak]

On Wed, Oct 18, 2023 at 2:14 PM Uros Bizjak <ubizjak@xxxxxxxxx> wrote:
>
> On Wed, Oct 18, 2023 at 12:54 PM Nadav Amit <namit@xxxxxxxxxx> wrote:
> >
> >
> >
> > > On Oct 18, 2023, at 12:04 PM, Uros Bizjak <ubizjak@xxxxxxxxx> wrote:
> > >
> > > Solved.
> > >
> > > All that is needed is to patch cpu_init() from
> > > arch/x86/kernel/cpu/common.c with:
> > >
> > > --cut here--
> > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > > index b14fc8c1c953..61b6fcdf6937 100644
> > > --- a/arch/x86/kernel/cpu/common.c
> > > +++ b/arch/x86/kernel/cpu/common.c
> > > @@ -2228,7 +2232,7 @@ void cpu_init_exception_handling(void)
> > > */
> > > void cpu_init(void)
> > > {
> > > -       struct task_struct *cur = current;
> > > +       struct task_struct *cur = this_cpu_read_stable(pcpu_hot.current_task);
> > >       int cpu = raw_smp_processor_id();
> >
> > Thanks for solving that, and sorry that I missed it.
> >
> > The reason I didn’t encounter it before is that in my original patch I created
> > a new compilation unit which only defined the alias.
> >
> > Since there might be additional problems (any “current” use in common.c is
> > dangerous, even in included files), I think that while there may be additional
> > solutions, defining the alias in a separate compilation unit - as I did before -
> > is the safest.
>
> What happens here can be illustrated with the following testcase:
>
> --cut here--
> int init_mm;
>
> struct task_struct
> {
>   int *active_mm;
> };
>
> struct task_struct init_task;
>
> struct pcpu_hot
> {
>   struct task_struct *current_task;
> };
>
> struct pcpu_hot pcpu_hot = { .current_task = &init_task };
>
> extern const struct pcpu_hot __seg_gs const_pcpu_hot
> __attribute__((alias("pcpu_hot")));
>
> void foo (void)
> {
>   struct task_struct *cur = const_pcpu_hot.current_task;
>
>   cur->active_mm = &init_mm;
> }
> --cut here--
>
> gcc -O2 -S:
>
> foo:
>        movq    $init_mm, init_task(%rip)
>        ret
>
> Here, gcc optimizes the access to generic address space, which is
> allowed to, since *we set the alias to pcpu_hot*, which is in the
> generic address space. The compiler doesn't care that we actually
> want:
>
> foo:
>        movq    %gs:const_pcpu_hot(%rip), %rax
>        movq    $init_mm, (%rax)
>
> So yes, to prevent the optimization, we have to hide the alias in another TU.
>
> BTW: Clang creates:
>
> foo:
>        movq    %gs:pcpu_hot(%rip), %rax
>        movq    $init_mm, (%rax)
>        retq
>
> It is a bit more conservative and retains the address space of the
> aliasing symbol.
>
> Looks like another case of underspecified functionality where both
> compilers differ. Luckily, both DTRT when aliases are hidden in
> another TU.

Attached is the prototype patch that works for me (together with
Linus' FPU switching patch).

Uros.
diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h
index a1168e7b69e5..21e8bd4ea44e 100644
--- a/arch/x86/include/asm/current.h
+++ b/arch/x86/include/asm/current.h
@@ -36,10 +36,23 @@ static_assert(sizeof(struct pcpu_hot) == 64);
 
 DECLARE_PER_CPU_ALIGNED(struct pcpu_hot, pcpu_hot);
 
+/*
+ *
+ */
+DECLARE_PER_CPU_ALIGNED(const struct pcpu_hot __percpu_seg_override,
+			const_pcpu_hot);
+
+#ifdef CONFIG_USE_X86_SEG_SUPPORT
+static __always_inline struct task_struct *get_current(void)
+{
+	return const_pcpu_hot.current_task;
+}
+#else
 static __always_inline struct task_struct *get_current(void)
 {
 	return this_cpu_read_stable(pcpu_hot.current_task);
 }
+#endif
 
 #define current get_current()
 
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 93eabf544031..a1d17578f5c1 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -19,6 +19,7 @@ KCSAN_SANITIZE_common.o := n
 
 obj-y			:= cacheinfo.o scattered.o topology.o
 obj-y			+= common.o
+obj-y			+= percpu-hot.o
 obj-y			+= rdrand.o
 obj-y			+= match.o
 obj-y			+= bugs.o
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index b14fc8c1c953..fb9e106467ba 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2043,13 +2043,6 @@ static __init int setup_clearcpuid(char *arg)
 }
 __setup("clearcpuid=", setup_clearcpuid);
 
-DEFINE_PER_CPU_ALIGNED(struct pcpu_hot, pcpu_hot) = {
-	.current_task	= &init_task,
-	.preempt_count	= INIT_PREEMPT_COUNT,
-	.top_of_stack	= TOP_OF_INIT_STACK,
-};
-EXPORT_PER_CPU_SYMBOL(pcpu_hot);
-
 #ifdef CONFIG_X86_64
 DEFINE_PER_CPU_FIRST(struct fixed_percpu_data,
 		     fixed_percpu_data) __aligned(PAGE_SIZE) __visible;
diff --git a/arch/x86/kernel/cpu/percpu-hot.c b/arch/x86/kernel/cpu/percpu-hot.c
new file mode 100644
index 000000000000..eca558a3845a
--- /dev/null
+++ b/arch/x86/kernel/cpu/percpu-hot.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/sched/task.h>
+#include <asm/current.h>
+
+DEFINE_PER_CPU_ALIGNED(struct pcpu_hot, pcpu_hot) = {
+	.current_task	= &init_task,
+	.preempt_count	= INIT_PREEMPT_COUNT,
+	.top_of_stack	= TOP_OF_INIT_STACK,
+};
+EXPORT_PER_CPU_SYMBOL(pcpu_hot);
+
+DECLARE_PER_CPU_ALIGNED(const struct pcpu_hot __percpu_seg_override,
+			const_pcpu_hot) __attribute__((alias("pcpu_hot")));
+EXPORT_PER_CPU_SYMBOL(const_pcpu_hot);
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d7779a18b24f..bf9815eaf4aa 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -212,7 +212,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
  */
 #define ___ADDRESSABLE(sym, __attrs) \
 	static void * __used __attrs \
-		__UNIQUE_ID(__PASTE(__addressable_,sym)) = (void *)&sym;
+	__UNIQUE_ID(__PASTE(__addressable_,sym)) = (void *)(uintptr_t)&sym;
 #define __ADDRESSABLE(sym) \
 	___ADDRESSABLE(sym, __section(".discard.addressable"))