[PATCH] arm64: signal: Ensure signal delivery failure is recoverable

[Dave Martin]

On Tue, Dec 03, 2024 at 05:48:10PM +0100, Kevin Brodsky wrote:
> On 03/12/2024 16:37, Dave Martin wrote:
> > Hi,
> >
> > On Tue, Dec 03, 2024 at 04:06:24PM +0100, Kevin Brodsky wrote:
> >> On 03/12/2024 12:56, Dave Martin wrote:
> >>> On Tue, Dec 03, 2024 at 10:03:22AM +0000, Kevin Brodsky wrote:
> >>>> [...]
> >>>> @@ -1462,10 +1462,33 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
> >>>>  			 struct rt_sigframe_user_layout *user, int usig)
> >>>>  {

[...]

> >>>> +	/*
> >>>> +	 * We must not fail from this point onwards. We are going to update
> >>>> +	 * registers, including SP, in order to invoke the signal handler. If
> >>>> +	 * we failed and attempted to deliver a nested SIGSEGV to a handler
> >>>> +	 * after that point, the subsequent sigreturn would end up restoring
> >>>> +	 * the (partial) state for the original signal handler.
> >>>> +	 */
> >>> The same is true at the call site of this function; could problems
> >>> creep back in there?
> >> That's a fair point, I could add a short comment after the call to
> >> setup_return() to clarify this.
> > That would make sense.
> >
> > (Or refactor so that the point of no return is at a function boundary;
> > but since this is a Fixes: patch, it's probably best to keep things
> > simple.)
> 
> I did consider this, in fact this is what I did initially. However I
> then realised that gcs_signal_entry() itself modifies thread state that
> affects control flow. In other words, if it succeeds, then we are
> already in a situation where failure should be avoided, so pulling it
> out of setup_return() wouldn't necessarily be clearer. There may be more
> operations that can fail like gcs_signal_entry() in the future, meaning
> we'd have to "revert" such operations if any fails. I think it makes
> sense for this to be handled in setup_return(). At least we don't need
> to complicate things for now.

That seems fair.  A one-line comment at the call to setup_return()
should be enough.  I guess we can live with the slightly confusing
function naming (since it was already a bit confusing, and this isn't
trivial to sort out).

So, with the comment, feel free to add:

Reviewed-by: Dave Martin <Dave.Martin at arm.com>

Cheers
---Dave