Received: by 2002:a6b:fb09:0:0:0:0:0 with SMTP id h9csp3109288iog; Mon, 20 Jun 2022 11:29:39 -0700 (PDT) X-Google-Smtp-Source: AGRyM1vjkAh6EbWG3miXI63BMn1stqVP0SwWQXT9DXpP6FRrzxN5e8SO+CrXyAeV6Mglrz4XBEga X-Received: by 2002:a17:90b:388d:b0:1e6:a0a4:c80f with SMTP id mu13-20020a17090b388d00b001e6a0a4c80fmr28667246pjb.39.1655749779026; Mon, 20 Jun 2022 11:29:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1655749779; cv=none; d=google.com; s=arc-20160816; b=b0mMlAIQxEzuIlV/etzd6inwAPeiQXVDpeiDIp5awHscNtfZ/ZwdfcPJMYMgY8LbJ7 8J097M43xG5WPe3oh/+f7Nx1SmDKUB6bfcbiklSekIl3ed2l0ixREI3gjVnzulZVPm5g VKtguUpt+UqW4EdjYtae3x45yXrzHMGY/14V47k15q4quTa05iKkjzej7fTKj7oGY5T6 ZJseuxuZq3JLzjIXju18CR6zMuPjLNVkj6jGwVbflFUXiQ+wnpuVbTLWm5j3U3GOiLF8 jYH5mU9paO4I7xZpplwdZKZdi1/gqK/BHwInIJ6xrNqqH1R0fTm7GiEzeaJtkNkhkaMj mu1g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:thread-index:thread-topic :content-transfer-encoding:mime-version:subject:references :in-reply-to:message-id:cc:to:from:date:dkim-signature:dkim-filter; bh=7RkjAJHH14LaDH5ne1pcjfolggbCqKjqHqQqr6OCNF8=; b=OziVcDvmBZg1vQSADo6L2wv/fYK3M0JBxE8pF8CJlC8/tQVyyP8nArAC+++QOpiefE CRdyUuRo98hQQN2o3Pbn90+GXhZq/UDQS5icp2ibCC5DcmGWyKXK1UU5eN7WmMiNN8ZI RfSvNyMDfthyizsMGCiqVR9Y679tYBsM9Tyj+W+JR9XJSfRwiORtLXWs0fYKkugaEi2n JU/Q3Qj6GriaPTdckJtZVeGHSzwcp+1i4dW6CsyiMp3Lys9ST2XzUpCJKLHZA6nn3tFn bdsSEKr3ZU0g4WiIASbObw4EMu7TW5gf4exll46o5O5iM0zB2Vmvb4j7RKQoBOz21aqP G5Rg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@efficios.com header.s=default header.b=cJV6Pcln; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j5-20020a056a00130500b00517cafbba6csi18413128pfu.145.2022.06.20.11.29.26; Mon, 20 Jun 2022 11:29:39 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@efficios.com header.s=default header.b=cJV6Pcln; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245045AbiFTSKu (ORCPT + 99 others); Mon, 20 Jun 2022 14:10:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56076 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238101AbiFTSKt (ORCPT ); Mon, 20 Jun 2022 14:10:49 -0400 Received: from mail.efficios.com (mail.efficios.com [167.114.26.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 65C351A384 for ; Mon, 20 Jun 2022 11:10:47 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 52C9F3CE49A; Mon, 20 Jun 2022 14:10:46 -0400 (EDT) Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id oc0BcvKIYAnY; Mon, 20 Jun 2022 14:10:45 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 57FDE3CE297; Mon, 20 Jun 2022 14:10:45 -0400 (EDT) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com 57FDE3CE297 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficios.com; s=default; t=1655748645; bh=7RkjAJHH14LaDH5ne1pcjfolggbCqKjqHqQqr6OCNF8=; h=Date:From:To:Message-ID:MIME-Version; b=cJV6Pcln0WAq7Kb0IBGuhvyLJF0BUx4S9LMmcjIUrPTXT1f61tCX8a5UDgNVKjW2p hG25qpUhBkpygCK2tKWl4193s/NogeoeFW6jHqPq63LpXh7cHnxwBwk7XAc8jD59mD mmz5Y6umqPf3FOswCdMJE3FIQyVkgWrgiSJWs5UY6lg002/jrbmjJbJhM3JOQf8oVj IPYhf4g6TZLWTb1gR4J8jhwmGupUtwSoFZZLHzxPTZZsm3DNA/WdBxi8SrW7ZNhbTW g+0oENydyVy0h0iOEiyLLXJ0zfplzKgX0xvYFAycLtoXM++bMrC3Mbsbi0SwXA1ZRk cXbaXX0TAZCuA== X-Virus-Scanned: amavisd-new at efficios.com Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id 2nxXJeJpvzQt; Mon, 20 Jun 2022 14:10:45 -0400 (EDT) Received: from mail03.efficios.com (mail03.efficios.com [167.114.26.124]) by mail.efficios.com (Postfix) with ESMTP id 2BBBC3CDCED; Mon, 20 Jun 2022 14:10:45 -0400 (EDT) Date: Mon, 20 Jun 2022 14:10:45 -0400 (EDT) From: Mathieu Desnoyers To: Alexander Mikhalitsyn , Florian Weimer , Carlos O'Donell , Paul Turner , Simon Marchi , Peter Oskolkov , Chris Kennelly , Kevin Malachowski , Pedro Alves Cc: Bui Quang Minh , linux-kernel , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86 , "H. Peter Anvin" , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Daniel Bristot de Oliveira , Valentin Schneider , "Paul E. McKenney" , Boqun Feng , Kees Cook , "Chang S. Bae" , "Eric W. Biederman" , Brian Gerst Message-ID: <648712158.13199.1655748645141.JavaMail.zimbra@efficios.com> In-Reply-To: References: <20220618182515.95831-1-minhquangbui99@gmail.com> <258546133.12151.1655739550814.JavaMail.zimbra@efficios.com> Subject: Re: [PATCH] rseq: x86: Fix rseq_cs get cleared when returning from signal handler MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [167.114.26.124] X-Mailer: Zimbra 8.8.15_GA_4304 (ZimbraWebClient - FF100 (Linux)/8.8.15_GA_4304) Thread-Topic: rseq: x86: Fix rseq_cs get cleared when returning from signal handler Thread-Index: jPQ8y5dBGIRhvQHRYh+bpExDAeaVOg== X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- On Jun 20, 2022, at 11:46 AM, Alexander Mikhalitsyn alexander@mihalicyn.com wrote: > 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. OK, so from the CRIU point of view, the only use of those flags is to set them again when they are observed in a process which is the target of a checkpoint-restore, right ? I'm just tying to figure out whether there are valid real-world use-cases for this flag. Initially, for RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT (not SIGNAL) rseq_cs flag, one use-case I had in mind was for a critical section for which it's only important that it stays on the same CPU, but can be preempted without abort. But I think the importance of not being restarted on preemption is limited, given that it is an optimization for a rare situation. For all 3 of no-restart on preempt/migrate/signal rseq flags, another use-case I had envisioned was to allow debuggers to single-step through rseq critical sections by taking control of the scheduling of all threads belonging to a process. I wonder how realistic it is to expect debuggers to implement this kind of feature, especially given that rseq could be used with data structures shared across processes over shared memory. AFAIK none of the debuggers implement this today, and I don't expect debuggers nor DynamoRIO (https://dynamorio.org/page_rseq.html) to implement this. The alternative way that debuggers can handle rseq critical sections is to skip over them using the entry/exit points defined within rseq entry/exit points array sections, in a fashion not completely unlike the handling of ll/sc "atomic" code by architecture-specific heuristics. It does not allow single-stepping through the rseq c.s. assembly, but it does prevent retrying forever when trying to single-step through an rseq c.s. I'm adding a few people concerned by those topics in CC. Feedback is very welcome. The key question here is: who would be sad to see the following flags be deprecated ? RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL Thanks, Mathieu > > Regards, > Alex > > On Mon, Jun 20, 2022 at 6:39 PM Mathieu Desnoyers > wrote: >> >> ----- On Jun 18, 2022, at 2:25 PM, Bui Quang Minh minhquangbui99@gmail.com >> 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 >> > Signed-off-by: Bui Quang Minh >> > --- >> > 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 -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com