Re: [PATCH] rseq: x86: Fix rseq_cs get cleared when returning from signal handler

From: Alexander Mikhalitsyn
Date: Mon Jun 20 2022 - 11:47:18 EST


Hi Mathieu!

It comes from CRIU ;)

We have seen periodic failures of our ZDTM test (rseq02):
zdtm: add rseq02 transition test with NO_RESTART CS flag
https://github.com/checkpoint-restore/criu/commit/db9ec13616625f98db95b00438a370f8a660bfee
zdtm: temporary disable rseq02 test
https://github.com/checkpoint-restore/criu/commit/ead227994b4918422496e06d3f3802188c091a7a

Bui Quang Minh wanted to investigate this problem and come up with
this kernel patch.

Regards,
Alex

On Mon, Jun 20, 2022 at 6:39 PM Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> ----- On Jun 18, 2022, at 2:25 PM, Bui Quang Minh minhquangbui99@xxxxxxxxx wrote:
>
> > rseq.rseq_cs is not expected to be cleared when we are still in critical
> > section. When using rseq with RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL, if the
> > signal is delivered, we don't restart to the abort ip but go to signal
> > handler then return and continue the critical section execution. While in
> > signal handler, if a preemption, migration or another signal is delivered,
> > we will fall into rseq_ip_fixup again. At that moment, the IP is in signal
> > handler not in critical section so the rseq.rseq_cs is cleared. Later, when
> > we return back to critical section from signal handlers, we get the
> > rseq.rseq_cs incorrectly cleared.
> >
> > Another scenario is when a preemption or migration happens, then a signal
> > is delivered. When preparing to return to user mode, in
> > exit_to_user_mode_loop, we first handle the signal and set return IP to
> > signal handler. After that, rseq_handle_notify_resume is called to handle
> > the preemption/migration, sees the IP is the address of signal handler not
> > in critical section and clear rseq.rseq_cs.
> >
> > To handle this case, a RSEQ_CS_FLAG_IN_SIGNAL_HANDLER is added. This flag
> > is set in rseq.flags (it is easier to read and update rseq.flags than
> > rseq.rseq_cs.flags) by the kernel when rseq does not restart on signal.
> > When this flag is set, we don't clear rseq.rseq_cs when the IP is outside
> > of critical section. The flag is unset in sigreturn path when the last
> > signal handler returns back to the critical section.
>
> Hi,
>
> You raise valid points.
>
> May I first ask what is your use-case for RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL ?
>
> Considering the added per-arch complexity, I think we should consider whether
> it's worthwhile to keep supporting this flag, and whether there are actual users
> of it out there, or if we should handle this by deprecating this flag.
>
> Thanks,
>
> Mathieu
>
> >
> > Reviewed-by: Alexander Mikhalitsyn <alexander@xxxxxxxxxxxxx>
> > Signed-off-by: Bui Quang Minh <minhquangbui99@xxxxxxxxx>
> > ---
> > arch/x86/kernel/signal.c | 7 ++++
> > include/linux/sched.h | 5 +++
> > include/uapi/linux/rseq.h | 8 +++-
> > kernel/rseq.c | 80 ++++++++++++++++++++++++++++++++++++---
> > 4 files changed, 94 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> > index 9c7265b524c7..7aadf69d6849 100644
> > --- a/arch/x86/kernel/signal.c
> > +++ b/arch/x86/kernel/signal.c
> > @@ -646,6 +646,10 @@ SYSCALL_DEFINE0(sigreturn)
> > */
> > if (!restore_sigcontext(regs, &frame->sc, 0))
> > goto badframe;
> > +
> > + if (rseq_sigreturn(regs))
> > + goto badframe;
> > +
> > return regs->ax;
> >
> > badframe:
> > @@ -678,6 +682,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > if (restore_altstack(&frame->uc.uc_stack))
> > goto badframe;
> >
> > + if (rseq_sigreturn(regs))
> > + goto badframe;
> > +
> > return regs->ax;
> >
> > badframe:
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index c46f3a63b758..656b03232d0a 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -2340,6 +2340,8 @@ static inline void rseq_execve(struct task_struct *t)
> > t->rseq_event_mask = 0;
> > }
> >
> > +int rseq_sigreturn(struct pt_regs *regs);
> > +
> > #else
> >
> > static inline void rseq_set_notify_resume(struct task_struct *t)
> > @@ -2365,6 +2367,9 @@ static inline void rseq_fork(struct task_struct *t,
> > unsigned long clone_flags)
> > static inline void rseq_execve(struct task_struct *t)
> > {
> > }
> > +static inline int rseq_sigreturn(struct pt_regs *regs)
> > +{
> > +}
> >
> > #endif
> >
> > diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> > index 77ee207623a9..f946af8e21e6 100644
> > --- a/include/uapi/linux/rseq.h
> > +++ b/include/uapi/linux/rseq.h
> > @@ -26,6 +26,7 @@ enum rseq_cs_flags_bit {
> > RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT_BIT = 0,
> > RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT = 1,
> > RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT = 2,
> > + RSEQ_CS_FLAG_IN_SIGNAL_HANDLER_BIT = 3,
> > };
> >
> > enum rseq_cs_flags {
> > @@ -35,6 +36,8 @@ enum rseq_cs_flags {
> > (1U << RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT),
> > RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE =
> > (1U << RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT),
> > + RSEQ_CS_FLAG_IN_SIGNAL_HANDLER =
> > + (1U << RSEQ_CS_FLAG_IN_SIGNAL_HANDLER_BIT),
> > };
> >
> > /*
> > @@ -115,7 +118,7 @@ struct rseq {
> > * Restartable sequences flags field.
> > *
> > * This field should only be updated by the thread which
> > - * registered this data structure. Read by the kernel.
> > + * registered this data structure. Read and set by the kernel.
> > * Mainly used for single-stepping through rseq critical sections
> > * with debuggers.
> > *
> > @@ -128,6 +131,9 @@ struct rseq {
> > * - RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
> > * Inhibit instruction sequence block restart on migration for
> > * this thread.
> > + * - RSEQ_CS_FLAG_IN_SIGNAL_HANDLER
> > + * Mark that this thread is in critical section but currently in
> > + * signal handlers, so don't clear the rseq_cs field.
> > */
> > __u32 flags;
> > } __attribute__((aligned(4 * sizeof(__u64))));
> > diff --git a/kernel/rseq.c b/kernel/rseq.c
> > index 97ac20b4f738..a4aca612724b 100644
> > --- a/kernel/rseq.c
> > +++ b/kernel/rseq.c
> > @@ -172,14 +172,16 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct
> > rseq_cs *rseq_cs)
> >
> > static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
> > {
> > - u32 flags, event_mask;
> > + u32 old_flags, flags, event_mask;
> > int ret;
> >
> > /* Get thread flags. */
> > - ret = get_user(flags, &t->rseq->flags);
> > + ret = get_user(old_flags, &t->rseq->flags);
> > if (ret)
> > return ret;
> >
> > + flags = old_flags;
> > +
> > /* Take critical section flags into account. */
> > flags |= cs_flags;
> >
> > @@ -203,6 +205,21 @@ static int rseq_need_restart(struct task_struct *t, u32
> > cs_flags)
> > t->rseq_event_mask = 0;
> > preempt_enable();
> >
> > + /*
> > + * If we don't restart on signal event, set the
> > + * RSEQ_CS_FLAG_IN_SIGNAL_HANDLER flag so that later rseq_ip_fixup
> > + * doesn't clear rseq_cs pointer as the IP is outside of critical
> > + * section.
> > + */
> > + if ((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
> > + (event_mask & RSEQ_EVENT_SIGNAL) &&
> > + !(old_flags & RSEQ_CS_FLAG_IN_SIGNAL_HANDLER)) {
> > + old_flags |= RSEQ_CS_FLAG_IN_SIGNAL_HANDLER;
> > + ret = put_user(old_flags, &t->rseq->flags);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > return !!(event_mask & ~flags);
> > }
> >
> > @@ -248,10 +265,23 @@ static int rseq_ip_fixup(struct pt_regs *regs)
> > /*
> > * Handle potentially not being within a critical section.
> > * If not nested over a rseq critical section, restart is useless.
> > - * Clear the rseq_cs pointer and return.
> > + * In case the RSEQ_CS_FLAG_IN_SIGNAL_HANDLER is set, we are in
> > + * signal handlers and later return to critical section so don't
> > + * clear rseq_cs pointer.
> > + * Otherwise, clear the rseq_cs pointer and return.
> > */
> > - if (!in_rseq_cs(ip, &rseq_cs))
> > - return clear_rseq_cs(t);
> > + if (!in_rseq_cs(ip, &rseq_cs)) {
> > + u32 flags;
> > +
> > + ret = get_user(flags, &t->rseq->flags);
> > + if (ret)
> > + return ret;
> > + if (flags & RSEQ_CS_FLAG_IN_SIGNAL_HANDLER)
> > + return 0;
> > + else
> > + return clear_rseq_cs(t);
> > + }
> > +
> > ret = rseq_need_restart(t, rseq_cs.flags);
> > if (ret <= 0)
> > return ret;
> > @@ -322,6 +352,46 @@ void rseq_syscall(struct pt_regs *regs)
> >
> > #endif
> >
> > +#ifdef CONFIG_RSEQ
> > +
> > +int rseq_sigreturn(struct pt_regs *regs)
> > +{
> > + int ret;
> > + struct rseq_cs rseq_cs;
> > + struct task_struct *t = current;
> > +
> > + if (t->rseq) {
> > + u32 flags;
> > +
> > + ret = get_user(flags, &t->rseq->flags);
> > + if (ret)
> > + return ret;
> > +
> > + if (flags & RSEQ_CS_FLAG_IN_SIGNAL_HANDLER) {
> > + ret = rseq_get_rseq_cs(t, &rseq_cs);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * If the returned IP is in critical section, it
> > + * means we handle all the possible nested signal,
> > + * so unset the RSEQ_CS_FLAG_IN_SIGNAL_HANDLER.
> > + */
> > + if (in_rseq_cs(regs->ip, &rseq_cs)) {
> > + flags &= ~RSEQ_CS_FLAG_IN_SIGNAL_HANDLER;
> > +
> > + ret = put_user(flags, &t->rseq->flags);
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#endif
> > +
> > /*
> > * sys_rseq - setup restartable sequences for caller thread.
> > */
> > --
> > 2.25.1
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com