x86/entry: Entry/exception code rework, preparatory patches

From: Andy Lutomirski <[email protected]>

A data breakpoint near the top of an IST stack will cause unresoverable

recursion. A data breakpoint on the GDT, IDT, or TSS is terrifying.

Prevent either of these from happening.

Co-developed-by Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/kernel/hw_breakpoint.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

From: Andy Lutomirski <[email protected]>

A data breakpoint near the top of an IST stack will cause unresoverable

recursion. A data breakpoint on the GDT, IDT, or TSS is terrifying. Prevent either of these from happening.

Co-developed-by Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/kernel/hw_breakpoint.c |   25 +++++++++++++++++++++++++
  1 file changed, 25 insertions(+)

arch/x86/kernel/hw_breakpoint.c +25 -0

@@ -227,10 +227,35 @@ int arch_check_bp_in_kernelspace(struct
  	return (va >= TASK_SIZE_MAX) || ((va + len - 1) >= TASK_SIZE_MAX);
  }
 
+/*
+ * Checks whether the range from addr to end, inclusive, overlaps the CPU
+ * entry area range.
+ */
+static inline bool within_cpu_entry_area(unsigned long addr, unsigned long end)
+{
+	return end >= CPU_ENTRY_AREA_PER_CPU &&
+	       addr < (CPU_ENTRY_AREA_PER_CPU + CPU_ENTRY_AREA_TOTAL_SIZE);
+}
+
  static int arch_build_bp_info(struct perf_event *bp,
  			      const struct perf_event_attr *attr,
  			      struct arch_hw_breakpoint *hw)
  {
+	unsigned long bp_end;
+
+	bp_end = attr->bp_addr + attr->bp_len - 1;
+	if (bp_end < attr->bp_addr)
+		return -EINVAL;
+
+	/*
+	 * Prevent any breakpoint of any type that overlaps the
+	 * cpu_entry_area.  This protects the IST stacks and also
+	 * reduces the chance that we ever find out what happens if
+	 * there's a data breakpoint on the GDT, IDT, or TSS.
+	 */
+	if (within_cpu_entry_area(attr->bp_addr, bp_end))
+		return -EINVAL;
+
  	hw->address = attr->bp_addr;
  	hw->mask = 0;
 

From: Andy Lutomirski <[email protected]>

A data breakpoint near the top of an IST stack will cause unresoverable recursion. A data breakpoint on the GDT, IDT, or TSS is terrifying. Prevent either of these from happening.

Co-developed-by Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/kernel/hw_breakpoint.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

arch/x86/kernel/hw_breakpoint.c +0 -0

@@ -227,10 +227,35 @@ int arch_check_bp_in_kernelspace(struct
        return (va >= TASK_SIZE_MAX) || ((va + len - 1) >= TASK_SIZE_MAX);
 }

arch/x86/kernel/hw_breakpoint.c +16 -0

+
 static int arch_build_bp_info(struct perf_event *bp,
                              const struct perf_event_attr *attr,
                              struct arch_hw_breakpoint *hw)
 {
+       unsigned long bp_end;
+
+       bp_end = attr->bp_addr + attr->bp_len - 1;
+       if (bp_end < attr->bp_addr)
+               return -EINVAL;
+
+       /*
+        * Prevent any breakpoint of any type that overlaps the
+        * cpu_entry_area.  This protects the IST stacks and also
+        * reduces the chance that we ever find out what happens if
+        * there's a data breakpoint on the GDT, IDT, or TSS.
+        */
+       if (within_cpu_entry_area(attr->bp_addr, bp_end))
+               return -EINVAL;
+
        hw->address = attr->bp_addr;
        hw->mask = 0;
 

From: Andy Lutomirski <[email protected]>

A data breakpoint near the top of an IST stack will cause unresoverable recursion. A data breakpoint on the GDT, IDT, or TSS is terrifying. Prevent either of these from happening.

Quoted 3 lines · Thomas Gleixner

From: Andy Lutomirski <[email protected]>

Quoted 4 lines

A data breakpoint near the top of an IST stack will cause unresoverable recursion. A data breakpoint on the GDT, IDT, or TSS is terrifying. Prevent either of these from happening.

What happen when a data breakpoint on the direct GDT (load_direct_gdt()) and the debug IDT (load_debug_idt()) which are not considered in this patch?

The scheduler IPI has grown weird and wonderful over the years, time for spring cleaning.

Move all the non-trivial stuff out of it and into a regular smp function call IPI. This then reduces the schedule_ipi() to most of it's former MOP

Quoted 5 lines · Thomas Gleixner

The scheduler IPI has grown weird and wonderful over the years, time for spring cleaning.

Move all the non-trivial stuff out of it and into a regular smp function call IPI. This then reduces the schedule_ipi() to most of it's former MOP

s/MOP/NOP/ after clarifying it on IRC. :)

It was NOP long ago. Now it's Minimal OPeration :)

The scheduler IPI has grown weird and wonderful over the years, time for spring cleaning.

Move all the non-trivial stuff out of it and into a regular smp function call IPI. This then reduces the schedule_ipi() to most of it's former MOP glory and ensures to keep the interrupt vector lean and mean.

Aside of that avoiding the full irq_enter() in the x86 IPI implementation is incorrect as scheduler_ipi() can be instrumented. To work around that scheduler_ipi() had an irq_enter/exit() hack when heavy work was pending. This is gone now.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
  kernel/sched/core.c  |   64 +++++++++++++++++++++++----------------------------
  kernel/sched/fair.c  |    5 +--
  kernel/sched/sched.h |    6 +++-
  3 files changed, 36 insertions(+), 39 deletions(-)

kernel/sched/core.c +30 -34

@@ -219,6 +219,13 @@ void update_rq_clock(struct rq *rq)
  	update_rq_clock_task(rq, delta);
  }
 
+static inline void
+rq_csd_init(struct rq *rq, call_single_data_t *csd, smp_call_func_t func)
+{
+	csd->flags = 0;
+	csd->func = func;
+	csd->info = rq;
+}
 
  #ifdef CONFIG_SCHED_HRTICK
  /*

@@ -314,16 +321,14 @@ void hrtick_start(struct rq *rq, u64 del
  	hrtimer_start(&rq->hrtick_timer, ns_to_ktime(delay),
  		      HRTIMER_MODE_REL_PINNED_HARD);
  }
+
  #endif /* CONFIG_SMP */
 
  static void hrtick_rq_init(struct rq *rq)
  {
  #ifdef CONFIG_SMP
-	rq->hrtick_csd.flags = 0;
-	rq->hrtick_csd.func = __hrtick_start;
-	rq->hrtick_csd.info = rq;
+	rq_csd_init(rq, &rq->hrtick_csd, __hrtick_start);
  #endif
-
  	hrtimer_init(&rq->hrtick_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
  	rq->hrtick_timer.function = hrtick;
  }

@@ -650,6 +655,16 @@ static inline bool got_nohz_idle_kick(vo
  	return false;
  }
 
+static void nohz_csd_func(void *info)
+{
+	struct rq *rq = info;
+
+	if (got_nohz_idle_kick()) {
+		rq->idle_balance = 1;
+		raise_softirq_irqoff(SCHED_SOFTIRQ);
+	}
+}
+
  #else /* CONFIG_NO_HZ_COMMON */
 
  static inline bool got_nohz_idle_kick(void)

@@ -2292,6 +2307,11 @@ void sched_ttwu_pending(void)
  	rq_unlock_irqrestore(rq, &rf);
  }
 
+static void wake_csd_func(void *info)
+{
+	sched_ttwu_pending();
+}
+
  void scheduler_ipi(void)
  {
  	/*

@@ -2300,34 +2320,6 @@ void scheduler_ipi(void)
  	 * this IPI.
  	 */
  	preempt_fold_need_resched();
-
-	if (llist_empty(&this_rq()->wake_list) && !got_nohz_idle_kick())
-		return;
-
-	/*
-	 * Not all reschedule IPI handlers call irq_enter/irq_exit, since
-	 * traditionally all their work was done from the interrupt return
-	 * path. Now that we actually do some work, we need to make sure
-	 * we do call them.
-	 *
-	 * Some archs already do call them, luckily irq_enter/exit nest
-	 * properly.
-	 *
-	 * Arguably we should visit all archs and update all handlers,
-	 * however a fair share of IPIs are still resched only so this would
-	 * somewhat pessimize the simple resched case.
-	 */
-	irq_enter();
-	sched_ttwu_pending();
-
-	/*
-	 * Check if someone kicked us for doing the nohz idle load balance.
-	 */
-	if (unlikely(got_nohz_idle_kick())) {
-		this_rq()->idle_balance = 1;
-		raise_softirq_irqoff(SCHED_SOFTIRQ);
-	}
-	irq_exit();
  }
 
  static void ttwu_queue_remote(struct task_struct *p, int cpu, int wake_flags)

@@ -2336,9 +2328,9 @@ static void ttwu_queue_remote(struct tas
 
  	p->sched_remote_wakeup = !!(wake_flags & WF_MIGRATED);
 
-	if (llist_add(&p->wake_entry, &cpu_rq(cpu)->wake_list)) {
+	if (llist_add(&p->wake_entry, &rq->wake_list)) {
  		if (!set_nr_if_polling(rq->idle))
-			smp_send_reschedule(cpu);
+			smp_call_function_single_async(cpu, &rq->wake_csd);
  		else
  			trace_sched_wake_idle_without_ipi(cpu);
  	}

@@ -6685,12 +6677,16 @@ void __init sched_init(void)
  		rq->avg_idle = 2*sysctl_sched_migration_cost;
  		rq->max_idle_balance_cost = sysctl_sched_migration_cost;
 
+		rq_csd_init(rq, &rq->wake_csd, wake_csd_func);
+
  		INIT_LIST_HEAD(&rq->cfs_tasks);
 
  		rq_attach_root(rq, &def_root_domain);
  #ifdef CONFIG_NO_HZ_COMMON
  		rq->last_blocked_load_update_tick = jiffies;
  		atomic_set(&rq->nohz_flags, 0);
+
+		rq_csd_init(rq, &rq->nohz_csd, nohz_csd_func);
  #endif
  #endif /* CONFIG_SMP */
  		hrtick_rq_init(rq);

kernel/sched/fair.c +0 -0

@@ -10009,12 +10009,11 @@ static void kick_ilb(unsigned int flags)
  		return;

kernel/sched/fair.c +0 -0

  }
 
  /*

kernel/sched/sched.h +4 -2

@@ -890,9 +890,10 @@ struct rq {
  #ifdef CONFIG_SMP
  	unsigned long		last_blocked_load_update_tick;
  	unsigned int		has_blocked_load;
+	call_single_data_t	nohz_csd;
  #endif /* CONFIG_SMP */
  	unsigned int		nohz_tick_stopped;
-	atomic_t nohz_flags;
+	atomic_t		nohz_flags;
  #endif /* CONFIG_NO_HZ_COMMON */
 
  	unsigned long		nr_load_updates;

@@ -979,7 +980,7 @@ struct rq {
 
  	/* This is used to determine avg_idle's max value */
  	u64			max_idle_balance_cost;
-#endif
+#endif /* CONFIG_SMP */
 
  #ifdef CONFIG_IRQ_TIME_ACCOUNTING
  	u64			prev_irq_time;

@@ -1021,6 +1022,7 @@ struct rq {
  #endif
 
  #ifdef CONFIG_SMP
+	call_single_data_t	wake_csd;
  	struct llist_head	wake_list;
  #endif
 

kernel/sched/fair.c static void kick_ilb(unsigned int flags) · line 10016

Quoted 12 lines · Thomas Gleixner

kernel/sched/fair.c +2 -3

@@ -10009,12 +10009,11 @@ static void kick_ilb(unsigned int flags)
  		return;
 
  	/*
-	 * Use smp_send_reschedule() instead of resched_cpu().
-	 * This way we generate a sched IPI on the target CPU which
+	 * This way we generate an IPI on the target CPU which
  	 * is idle. And the softirq performing nohz idle load balance
  	 * will be run before returning from the IPI.
  	 */
-	smp_send_reschedule(ilb_cpu);
+	smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->wake_csd);

This should be nohz_csd instead of wake_csd, no? I.e.:

        smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);

Quoted 27 lines · Thomas Gleixner

kernel/sched/core.c +10 -0

@@ -650,6 +655,16 @@ static inline bool got_nohz_idle_kick(vo
  	return false;
  }
+static void nohz_csd_func(void *info)
+{
+	struct rq *rq = info;
+
+	if (got_nohz_idle_kick()) {
+		rq->idle_balance = 1;
+		raise_softirq_irqoff(SCHED_SOFTIRQ);
+	}
+}
+
  #else /* CONFIG_NO_HZ_COMMON */
  static inline bool got_nohz_idle_kick(void)
 
  #ifdef CONFIG_NO_HZ_COMMON

  		rq->last_blocked_load_update_tick = jiffies;
  		atomic_set(&rq->nohz_flags, 0);

kernel/sched/core.c +2 -0

+
+		rq_csd_init(rq, &rq->nohz_csd, nohz_csd_func);
  #endif
  #endif /* CONFIG_SMP */
  		hrtick_rq_init(rq);

kernel/sched/fair.c static void kick_ilb(unsigned int flags) · line 10015

Quoted 10 lines

kernel/sched/fair.c +2 -3

@@ -10009,12 +10009,11 @@ static void kick_ilb(unsigned int flags)
  		return;
  	/*
-	 * Use smp_send_reschedule() instead of resched_cpu().
-	 * This way we generate a sched IPI on the target CPU which
+	 * This way we generate an IPI on the target CPU which
  	 * is idle. And the softirq performing nohz idle load balance
  	 * will be run before returning from the IPI.
  	 */
-	smp_send_reschedule(ilb_cpu);
+	smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->wake_csd);

This should be nohz_csd instead of wake_csd, no? I.e.:

       smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);

Quoted 41 lines · Alexandre Chartre

Quoted 27 lines · Thomas Gleixner

kernel/sched/core.c +10 -0

@@ -650,6 +655,16 @@ static inline bool got_nohz_idle_kick(vo
  	return false;
  }
+static void nohz_csd_func(void *info)
+{
+	struct rq *rq = info;
+
+	if (got_nohz_idle_kick()) {
+		rq->idle_balance = 1;
+		raise_softirq_irqoff(SCHED_SOFTIRQ);
+	}
+}
+
  #else /* CONFIG_NO_HZ_COMMON */
  static inline bool got_nohz_idle_kick(void)
 
  #ifdef CONFIG_NO_HZ_COMMON

  		rq->last_blocked_load_update_tick = jiffies;
  		atomic_set(&rq->nohz_flags, 0);

kernel/sched/core.c +2 -0

+
+		rq_csd_init(rq, &rq->nohz_csd, nohz_csd_func);
  #endif
  #endif /* CONFIG_SMP */
  		hrtick_rq_init(rq);

kernel/sched/fair.c static void kick_ilb(unsigned int flags) · line 10015

Quoted 10 lines

kernel/sched/fair.c +2 -3

@@ -10009,12 +10009,11 @@ static void kick_ilb(unsigned int flags)
  		return;
  	/*
-	 * Use smp_send_reschedule() instead of resched_cpu().
-	 * This way we generate a sched IPI on the target CPU which
+	 * This way we generate an IPI on the target CPU which
  	 * is idle. And the softirq performing nohz idle load balance
  	 * will be run before returning from the IPI.
  	 */
-	smp_send_reschedule(ilb_cpu);
+	smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->wake_csd);

This should be nohz_csd instead of wake_csd, no? I.e.:

Quoted 1 line

       smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);

You figured correctly. Thanks!

I still see warnings of the form "leave instruction with modified stack frame" from older complilers and of the form "undefined stack state" from newer compilers. I am running stock objtool versions, so I am guessing that this is at least one reason for these warnings.

Quoted 4 lines · Paul E. McKenney

I still see warnings of the form "leave instruction with modified stack frame" from older complilers and of the form "undefined stack state" from newer compilers. I am running stock objtool versions, so I am guessing that this is at least one reason for these warnings.

Part 5, patch 2, might be responsible. I still have to look at curing that.

Now that the scheduler IPI is trivial and simple again there is no point to have the little function out of line. This simplifies the effort of constraining the instrumentation nicely.

Signed-off-by Thomas Gleixner <[email protected]>

---
  include/linux/sched.h |   10 +++++++++-
  kernel/sched/core.c   |   10 ----------
  2 files changed, 9 insertions(+), 11 deletions(-)

include/linux/sched.h +9 -1

@@ -1719,7 +1719,15 @@ extern char *__get_task_comm(char *to, s
  })
 
  #ifdef CONFIG_SMP
-void scheduler_ipi(void);
+static __always_inline void scheduler_ipi(void)
+{
+	/*
+	 * Fold TIF_NEED_RESCHED into the preempt_count; anybody setting
+	 * TIF_NEED_RESCHED remotely (for the first time) will also send
+	 * this IPI.
+	 */
+	preempt_fold_need_resched();
+}
  extern unsigned long wait_task_inactive(struct task_struct *, long match_state);
  #else
  static inline void scheduler_ipi(void) { }

kernel/sched/core.c +0 -10

@@ -2312,16 +2312,6 @@ static void wake_csd_func(void *info)
  	sched_ttwu_pending();
  }
 
-void scheduler_ipi(void)
-{
-	/*
-	 * Fold TIF_NEED_RESCHED into the preempt_count; anybody setting
-	 * TIF_NEED_RESCHED remotely (for the first time) will also send
-	 * this IPI.
-	 */
-	preempt_fold_need_resched();
-}
-
  static void ttwu_queue_remote(struct task_struct *p, int cpu, int wake_flags)
  {
  	struct rq *rq = cpu_rq(cpu);

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

Quoted 2 lines · Thomas Gleixner

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

I might be missing something, but isn't this guaranteed by do_signal()->get_signal()->task_work_run() path?

Quoted 2 lines · Thomas Gleixner

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

I might be missing something, but isn't this guaranteed by do_signal()->get_signal()->task_work_run() path?

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

Suggested-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c |    8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/entry/common.c +4 -4

@@ -156,16 +156,16 @@ static void exit_to_usermode_loop(struct
  		if (cached_flags & _TIF_PATCH_PENDING)
  			klp_update_patch_state(current);
 
-		/* deal with pending signal delivery */
-		if (cached_flags & _TIF_SIGPENDING)
-			do_signal(regs);
-
  		if (cached_flags & _TIF_NOTIFY_RESUME) {
  			clear_thread_flag(TIF_NOTIFY_RESUME);
  			tracehook_notify_resume(regs);
  			rseq_handle_notify_resume(NULL, regs);
  		}
 
+		/* deal with pending signal delivery */
+		if (cached_flags & _TIF_SIGPENDING)
+			do_signal(regs);
+
  		if (cached_flags & _TIF_USER_RETURN_NOTIFY)
  			fire_user_return_notifiers();
 

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

Suggested-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

Suggested-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
arch/x86/entry/common.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/entry/common.c +4 -4

@@ -156,16 +156,16 @@ static void exit_to_usermode_loop(struct
 		if (cached_flags & _TIF_PATCH_PENDING)
 			klp_update_patch_state(current);
 
-		/* deal with pending signal delivery */
-		if (cached_flags & _TIF_SIGPENDING)
-			do_signal(regs);
-
 		if (cached_flags & _TIF_NOTIFY_RESUME) {
 			clear_thread_flag(TIF_NOTIFY_RESUME);
 			tracehook_notify_resume(regs);
 			rseq_handle_notify_resume(NULL, regs);
 		}
 
+		/* deal with pending signal delivery */
+		if (cached_flags & _TIF_SIGPENDING)
+			do_signal(regs);
+
 		if (cached_flags & _TIF_USER_RETURN_NOTIFY)
 			fire_user_return_notifiers();

Quoted 2 lines · ----- On May 5, 2020, at 9:16 AM, Thomas Gleixner [email protected] wrote:

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

What is missing from this patch description is: _why_ is this deemed useful ?

Also, color me confused: is "do_signal()" actually running any user-space, or just setting up the user-space stack for eventual return to signal handler ?

Also, it might be OK, but we're changing the order of two things which have effects on each other: restartable sequences abort fixup for preemption and do_signal(), which also have effects on rseq abort.

Because those two will cause the abort to trigger, I suspect changing the order might be OK, but we really need to think this through.

Thanks,

Mathieu

Quoted 24 lines

Suggested-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
arch/x86/entry/common.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/entry/common.c +0 -4

@@ -156,16 +156,16 @@ static void exit_to_usermode_loop(struct
 		if (cached_flags & _TIF_PATCH_PENDING)
 			klp_update_patch_state(current);
 
-		/* deal with pending signal delivery */
-		if (cached_flags & _TIF_SIGPENDING)
-			do_signal(regs);
-
 		if (cached_flags & _TIF_NOTIFY_RESUME) {
 			clear_thread_flag(TIF_NOTIFY_RESUME);
 			tracehook_notify_resume(regs);
 			rseq_handle_notify_resume(NULL, regs);
 		}
 

arch/x86/entry/common.c static void exit_to_usermode_loop(struct · line 167

Quoted 3 lines

arch/x86/entry/common.c +3 -0

+		/* deal with pending signal delivery */
+		if (cached_flags & _TIF_SIGPENDING)
+			do_signal(regs);

Quoted 3 lines

arch/x86/entry/common.c +1 -0

+
 		if (cached_flags & _TIF_USER_RETURN_NOTIFY)
 			fire_user_return_notifiers();  

Quoted 46 lines · On Wed, 13 May 2020 16:56:41 -0400 (EDT) Mathieu Desnoyers…

Quoted 2 lines · ----- On May 5, 2020, at 9:16 AM, Thomas Gleixner [email protected] wrote:

Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked.

What is missing from this patch description is: _why_ is this deemed useful ?

Also, color me confused: is "do_signal()" actually running any user-space, or just setting up the user-space stack for eventual return to signal handler ?

Also, it might be OK, but we're changing the order of two things which have effects on each other: restartable sequences abort fixup for preemption and do_signal(), which also have effects on rseq abort.

Because those two will cause the abort to trigger, I suspect changing the order might be OK, but we really need to think this through.

Thanks,

Mathieu

Quoted 24 lines

Suggested-by Andy Lutomirski <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
arch/x86/entry/common.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/entry/common.c +0 -4

@@ -156,16 +156,16 @@ static void exit_to_usermode_loop(struct
 		if (cached_flags & _TIF_PATCH_PENDING)
 			klp_update_patch_state(current);
 
-		/* deal with pending signal delivery */
-		if (cached_flags & _TIF_SIGPENDING)
-			do_signal(regs);
-
 		if (cached_flags & _TIF_NOTIFY_RESUME) {
 			clear_thread_flag(TIF_NOTIFY_RESUME);
 			tracehook_notify_resume(regs);
 			rseq_handle_notify_resume(NULL, regs);
 		}
 

Quoted 3 lines

arch/x86/entry/common.c static void exit_to_usermode_loop(struct · line 167

Quoted 3 lines

arch/x86/entry/common.c +3 -0

+		/* deal with pending signal delivery */
+		if (cached_flags & _TIF_SIGPENDING)
+			do_signal(regs);

Looking deeper into this, it appears that do_signal() can freeze or kill the task.

That is, it wont go back to user space here, but simply schedule out (being traced) or even exit (killed).

Before the resume hooks would never be called in such cases, and now they are.

-- Steve

Quoted 3 lines

Quoted 3 lines

arch/x86/entry/common.c +1 -0

+
 		if (cached_flags & _TIF_USER_RETURN_NOTIFY)
 			fire_user_return_notifiers();

On Wed, 13 May 2020 16:56:41 -0400 (EDT)

Quoted 3 lines

arch/x86/entry/common.c static void exit_to_usermode_loop(struct · line 167

Quoted 3 lines

arch/x86/entry/common.c +3 -0

+		/* deal with pending signal delivery */
+		if (cached_flags & _TIF_SIGPENDING)
+			do_signal(regs);

Looking deeper into this, it appears that do_signal() can freeze or kill the task.

That is, it wont go back to user space here, but simply schedule out (being traced) or even exit (killed).

Before the resume hooks would never be called in such cases, and now they are.

Quoted 5 lines

Mathieu Desnoyers <[email protected]> wrote:

Also, color me confused: is "do_signal()" actually running any user-space, or just setting up the user-space stack for eventual return to signal handler ?

Quoted 6 lines

Also, it might be OK, but we're changing the order of two things which have effects on each other: restartable sequences abort fixup for preemption and do_signal(), which also have effects on rseq abort.

Because those two will cause the abort to trigger, I suspect changing the order might be OK, but we really need to think this through.

I should have thought about this before, so thanks to both of you for making me look at it again for the very wrong reasons.

Consider the patch dropped.

Quoted 4 lines · On Thu, 14 May 2020 02:12:21 +0200 Thomas Gleixner <[email protected]> wrote:

I should have thought about this before, so thanks to both of you for making me look at it again for the very wrong reasons.

Consider the patch dropped.

I'm glad our incompetence could be of service to you :-/

Steven, Mathieu

(combo reply)

Quoted 5 lines · Steven Rostedt

On Wed, 13 May 2020 16:56:41 -0400 (EDT)

Quoted 3 lines

arch/x86/entry/common.c static void exit_to_usermode_loop(struct · line 167

Quoted 3 lines

arch/x86/entry/common.c +3 -0

+        /* deal with pending signal delivery */
+        if (cached_flags & _TIF_SIGPENDING)
+            do_signal(regs);

Quoted 8 lines

Looking deeper into this, it appears that do_signal() can freeze or kill the task.

That is, it wont go back to user space here, but simply schedule out (being traced) or even exit (killed).

Before the resume hooks would never be called in such cases, and now they are.

It theoretically matters because pending task work might kill the task. That's the concern Andy and Peter had. Assume the following:

usermode

-> exception
set not fatal signal

-> exception
queue task work to kill task
<- return

<- return

The same could happen when the non fatal signal is set from a remote CPU.

So in theory that would result in:

handle non fatal signal first

handle task work which kills task

which would be the wrong order.

But that's just illusion.

Quoted 5 lines

Quoted 5 lines

Mathieu Desnoyers <[email protected]> wrote:

Also, color me confused: is "do_signal()" actually running any user-space, or just setting up the user-space stack for eventual return to signal handler ?

I'm surprised that you can't answer that question yourself. How did you ever make rseq work and how did rseq_signal_deliver() end up in setup_rt_frame()?

Hint: Tracing might answer that question

And to cut it short:

Exit to user space happnes only through ONE channel, i.e. leaving prepare_exit_to usermode().

     exit_to_usermode_loop <-prepare_exit_to_usermode
     do_signal <-exit_to_usermode_loop
     get_signal <-do_signal
     setup_sigcontext <-do_signal
     do_syscall_64 <-entry_SYSCALL_64_after_hwframe
     syscall_trace_enter <-do_syscall_64

     sys_rt_sigreturn()
     restore_altstack <-__ia32_sys_rt_sigreturn
     syscall_slow_exit_work <-do_syscall_64
     exit_to_usermode_loop <-do_syscall_64

Quoted 6 lines

Quoted 6 lines

Also, it might be OK, but we're changing the order of two things which have effects on each other: restartable sequences abort fixup for preemption and do_signal(), which also have effects on rseq abort.

Because those two will cause the abort to trigger, I suspect changing the order might be OK, but we really need to think this through.

That's a purely academic problem. The order is completely irrelevant. You have to handle any order anyway:

usermode

-> exception / syscall
sets signal

<- return

prepare_exit_to_usemode()

     cached_flags = READ_ONCE(t->flags);
     exit_to_user_mode_loop(regs, cached_flags) {
       while (cached_flags) {
          local_irq_enable();

          handle(cached_flags & RESCHED);
          handle(cached_flags & UPROBE);
          handle(cached_flags & PATCHING);
          handle(cached_flags & SIGNAL);
          handle(cached_flags & NOTIFY_RESUME);
          handle(cached_flags & RETURN_NOTIFY);

          local_irq_disable();

          cached_flags = READ_ONCE(t->flags);
        }

cached_flag is a momentary snapshot when attempting to return to user space.

But after reenabling interrupts any of the relevant flag bits can be set by an exception/interrupt or from remote. If preemption is enabled the task can be scheduled out, migrated at any point before disabling interrupts again. Even after disabling interrupts and before re-reading cached flags there might be a remote change of flags.

That said, even for the case Andy and Peter were looking at (MCE) the ordering is completely irrelevant.

I should have thought about this before, so thanks to both of you for making me look at it again for the very wrong reasons.

Consider the patch dropped.

Thanks,

tglx

Quoted 6 lines

Quoted 6 lines

Mathieu Desnoyers <[email protected]> wrote:

Also, color me confused: is "do_signal()" actually running any user-space, or just setting up the user-space stack for eventual return to signal handler ?

I'm surprised that you can't answer that question yourself. How did you ever make rseq work and how did rseq_signal_deliver() end up in setup_rt_frame()?

Hint: Tracing might answer that question

And to cut it short:

Exit to user space happnes only through ONE channel, i.e. leaving prepare_exit_to usermode().

Quoted 6 lines

Quoted 6 lines

Also, it might be OK, but we're changing the order of two things which have effects on each other: restartable sequences abort fixup for preemption and do_signal(), which also have effects on rseq abort.

Because those two will cause the abort to trigger, I suspect changing the order might be OK, but we really need to think this through.

That's a purely academic problem. The order is completely irrelevant. You have to handle any order anyway:

That said, even for the case Andy and Peter were looking at (MCE) the ordering is completely irrelevant.

Yes, I'm very well aware of this. But the patch commit message states:

"Make sure task_work runs before any kind of userspace -- very much including signals -- is invoked."

which seems to imply that "userspace" can be "invoked" before the task_work runs. Which makes no sense whatsoever. Hence my confused state.

READ/WRITE_ONCE_NOCHECK() is required for atomics in code which cannot be instrumented like the x86 int3 text poke code. As READ/WRITE_ONCE() is undergoing a rewrite, provide __{READ,WRITE}_ONCE_SCALAR().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
  include/linux/compiler.h |    8 ++++++++
  1 file changed, 8 insertions(+)

include/linux/compiler.h +8 -0

@@ -313,6 +313,14 @@ unsigned long read_word_at_a_time(const
  	__u.__val;					\
  })
 
+#define __READ_ONCE_SCALAR(x)				\
+	(*(const volatile typeof(x) *)&(x))
+
+#define __WRITE_ONCE_SCALAR(x, val)			\
+do {							\
+	*(volatile typeof(x) *)&(x) = val;		\
+} while (0)
+
  /**
   * data_race - mark an expression as containing intentional data races
   *

READ/WRITE_ONCE_NOCHECK() is required for atomics in code which cannot be instrumented like the x86 int3 text poke code. As READ/WRITE_ONCE() is undergoing a rewrite, provide __{READ,WRITE}_ONCE_SCALAR().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/compiler.h |    8 ++++++++
 1 file changed, 8 insertions(+)

Quoted 13 lines · Thomas Gleixner

READ/WRITE_ONCE_NOCHECK() is required for atomics in code which cannot be instrumented like the x86 int3 text poke code. As READ/WRITE_ONCE() is undergoing a rewrite, provide __{READ,WRITE}_ONCE_SCALAR().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/linux/compiler.h |    8 ++++++++
 1 file changed, 8 insertions(+)

include/linux/compiler.h unsigned long read_word_at_a_time(const · line 322

Quoted 10 lines

include/linux/compiler.h +7 -0

@@ -313,6 +313,14 @@ unsigned long read_word_at_a_time(const
 	__u.__val;					\
 })
 
+#define __READ_ONCE_SCALAR(x)				\
+	(*(const volatile typeof(x) *)&(x))
+
+#define __WRITE_ONCE_SCALAR(x, val)			\
+do {							\
+	*(volatile typeof(x) *)&(x) = val;		\
+} while (0)

FWIW, these end up being called __READ_ONCE() and __WRITE_ONCE() after the rewrite; the *_SCALAR() variants will call into kcsan_check_atomic_*().

If you go with that naming now, then any later conflict should fall out in the wash.

READ/WRITE_ONCE_NOCHECK() is required for atomics in code which cannot be instrumented like the x86 int3 text poke code. As READ/WRITE_ONCE() is undergoing a rewrite, provide __{READ,WRITE}_ONCE_SCALAR().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Currently instrumentation of atomic primitives is done at the architecture level, while composites or fallbacks are provided at the generic level.

The result is that there are no uninstrumented variants of the fallbacks. Since there is now need of such (see the next patch), invert this ordering.

Doing this means moving the instrumentation into the generic code as well as having (for now) two variants of the fallbacks.

Notes:

- the various *cond_read* primitives are not proper fallbacks and got moved into linux/atomic.c. No arch_ variants are generated because the base primitives smp_cond_load*() are instrumented.

- once all architectures are moved over to arch_atomic_ we can remove one of the fallback variants and reclaim some 2300 lines.

- atomic_{read,set}*() are no longer double-instrumented

Reported-by Thomas Gleixner <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc Mark Rutland <[email protected]>

Currently instrumentation of atomic primitives is done at the architecture level, while composites or fallbacks are provided at the generic level.

The result is that there are no uninstrumented variants of the fallbacks. Since there is now need of such (see the next patch),

invert this ordering.

Doing this means moving the instrumentation into the generic code as well as having (for now) two variants of the fallbacks.

Notes:

- the various *cond_read* primitives are not proper fallbacks and got moved into linux/atomic.c. No arch_ variants are generated because the base primitives smp_cond_load*() are instrumented.

- once all architectures are moved over to arch_atomic_ we can remove one of the fallback variants and reclaim some 2300 lines.

- atomic_{read,set}*() are no longer double-instrumented

Reported-by Thomas Gleixner <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc Mark Rutland <[email protected]>

Quoted 4 lines · On Tue, 05 May 2020 15:16:09 +0200 Thomas Gleixner <[email protected]> wrote:

Currently instrumentation of atomic primitives is done at the architecture level, while composites or fallbacks are provided at the generic level.

Quoted 2 lines

The result is that there are no uninstrumented variants of the fallbacks. Since there is now need of such (see the next patch),

Just a comment on the change log. Can we avoid saying "see the next patch"? A few years from now, if we stumble on changes in this commit and need to see that next patch, if something happened to lore, it may be difficult to find what that next patch was.

But saying that patch's subject, would be
just a simple search in the git history.

That said, looking at "the next patch" which is "x86/doublefault: Remove memmove() call", does that patch really have a need for such?

Use of memmove() in #DF is problematic considered tracing and other instrumentation.

Remove the memmove() call and simply write out what needs doing; this even clarifies the code, win-win! The code copies from the espfix64 stack to the normal task stack, there is no possible way for that to overlap.

Survives selftests/x86, specifically sigreturn_64.

Suggested-by Borislav Petkov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Acked-by Andy Lutomirski <[email protected]>

Link https://lkml.kernel.org/r/[email protected] open here

---
  arch/x86/kernel/traps.c |    7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

arch/x86/kernel/traps.c +6 -1

@@ -278,6 +278,7 @@ dotraplinkage void do_double_fault(struc
  		regs->ip == (unsigned long)native_irq_return_iret)
  	{
  		struct pt_regs *gpregs = (struct pt_regs *)this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
+		unsigned long *p = (unsigned long *)regs->sp;
 
  		/*
  		 * regs->sp points to the failing IRET frame on the

@@ -285,7 +286,11 @@ dotraplinkage void do_double_fault(struc
  		 * in gpregs->ss through gpregs->ip.
  		 *
  		 */
-		memmove(&gpregs->ip, (void *)regs->sp, 5*8);
+		gpregs->ip	= p[0];
+		gpregs->cs	= p[1];
+		gpregs->flags	= p[2];
+		gpregs->sp	= p[3];
+		gpregs->ss	= p[4];
  		gpregs->orig_ax = 0;  /* Missing (lost) #GP error code */
 
  		/*