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;
Date:   Tue, 26 Jul 2022 15:10:55 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Paul Turner <pjt@google.com>,
        linux-api <linux-api@vger.kernel.org>,
        Peter Oskolkov <posk@posk.io>
Message-ID: <790910676.83207.1658862655138.JavaMail.zimbra@efficios.com>
In-Reply-To: <20220622194617.1155957-1-mathieu.desnoyers@efficios.com>
References: <20220622194617.1155957-1-mathieu.desnoyers@efficios.com>
Subject: Re: [PATCH 1/2] rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Thread-Topic: rseq: Deprecate RSEQ_CS_FLAG_NO_RESTART_ON_* flags
Thread-Index: okUY3mzxLKojq7TSqc3xEuNzQtQsig==
Precedence: bulk

----- On Jun 22, 2022, at 3:46 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> The pretty much unused RSEQ_CS_FLAG_NO_RESTART_ON_* flags introduce
> complexity in rseq, and are subtly buggy [1]. Solving those issues
> requires introducing additional complexity in the rseq implementation
> for each supported architecture.
> 
> Considering that it complexifies the rseq ABI, I am proposing that we
> deprecate those flags. [2]
> 
> So far there appears to be consensus from maintainers of user-space
> projects impacted by this feature that its removal would be a welcome
> simplification. [3]
> 
> The deprecation approach proposed here is to issue WARN_ON_ONCE() when
> encountering those flags and kill the offending process with sigsegv.
> This should allow us to quickly identify whether anyone yells at us for
> removing this.

Hi Peter, would you consider pulling this patch through the tip tree ?

Thanks,

Mathieu

> 
> Link:
> https://lore.kernel.org/lkml/20220618182515.95831-1-minhquangbui99@gmail.com/
> [1]
> Link:
> https://lore.kernel.org/lkml/258546133.12151.1655739550814.JavaMail.zimbra@efficios.com/
> [2]
> Link:
> https://lore.kernel.org/lkml/87pmj1enjh.fsf@email.froward.int.ebiederm.org/ [3]
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> kernel/rseq.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 97ac20b4f738..81d7dc80787b 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -18,8 +18,9 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/rseq.h>
> 
> -#define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
> -				       RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
> +#define RSEQ_CS_NO_RESTART_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
> +				  RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
> +				  RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)
> 
> /*
>  *
> @@ -175,23 +176,15 @@ static int rseq_need_restart(struct task_struct *t, u32
> cs_flags)
> 	u32 flags, event_mask;
> 	int ret;
> 
> +	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS))
> +		return -EINVAL;
> +
> 	/* Get thread flags. */
> 	ret = get_user(flags, &t->rseq->flags);
> 	if (ret)
> 		return ret;
> 
> -	/* Take critical section flags into account. */
> -	flags |= cs_flags;
> -
> -	/*
> -	 * Restart on signal can only be inhibited when restart on
> -	 * preempt and restart on migrate are inhibited too. Otherwise,
> -	 * a preempted signal handler could fail to restart the prior
> -	 * execution context on sigreturn.
> -	 */
> -	if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
> -		     (flags & RSEQ_CS_PREEMPT_MIGRATE_FLAGS) !=
> -		     RSEQ_CS_PREEMPT_MIGRATE_FLAGS))
> +	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
> 		return -EINVAL;
> 
> 	/*
> @@ -203,7 +196,7 @@ static int rseq_need_restart(struct task_struct *t, u32
> cs_flags)
> 	t->rseq_event_mask = 0;
> 	preempt_enable();
> 
> -	return !!(event_mask & ~flags);
> +	return !!event_mask;
> }
> 
> static int clear_rseq_cs(struct task_struct *t)
> --
> 2.30.2

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com