Re: [RFC PATCH v2 4/6] arm64: signal: Allocate extra sigcontext space as needed

[Catalin Marinas <catalin.marinas@arm.com>]

On Mon, May 15, 2017 at 02:24:45PM +0100, Dave P Martin wrote:
> On Fri, May 12, 2017 at 05:57:24PM +0100, Catalin Marinas wrote:
> > On Wed, Apr 12, 2017 at 06:01:13PM +0100, Dave P Martin wrote:
> > > --- a/arch/arm64/include/uapi/asm/sigcontext.h
> > > +++ b/arch/arm64/include/uapi/asm/sigcontext.h
> > > @@ -80,4 +80,31 @@ struct esr_context {
> > >  	__u64 esr;
> > >  };
> > >  
> > > +/*
> > > + * Pointer to extra space for additional structures that don't fit in
> > > + * sigcontext.__reserved[].  Note:
> > > + *
> > > + * 1) fpsimd_context, esr_context and extra_context must be placed in
> > > + * sigcontext.__reserved[] if present.  They cannot be placed in the
> > > + * extra space.  Any other record can be placed either in the extra
> > > + * space or in sigcontext.__reserved[].
> > > + *
> > > + * 2) There must not be more than one extra_context.
> > > + *
> > > + * 3) If extra_context is present, it must be followed immediately in
> > > + * sigcontext.__reserved[] by the terminating null _aarch64_ctx (i.e.,
> > > + * extra_context must be the last record in sigcontext.__reserved[]
> > > + * except for the terminator).
> > > + *
> > > + * 4) The extra space must itself be terminated with a null
> > > + * _aarch64_ctx.
> > > + */
> > 
> > IIUC, if we need to save some state that doesn't fit in what's left of
> > sigcontext.__reserved[] (e.g. SVE with 1024-bit vector length), we
> > ignore the available space and go for a memory block following the end
> > of sigcontext.__reserved[] + 16. Is there a reason we can't store the
> > new state across the end of sigcontext.__reserved[] and move fp/lr at
> > the end of the new frame? I'm not sure the fp/lr position immediately
> > after __reserved[] counts as ABI.
> 
> This was my original view.
> 
> Originally I preferred not to waste the space and did move fp/lr to the
> end, but someone (I think you or Will) expressed concern that the fp/lr
> position relative to the signal frame _might_ count as ABI.
> 
> I think it's not that likely that software will be relying on this,
> since it appears easier just to follow the frame chain than to treat
> this as a special case.
> 
> But it's hard to be certain.  It comes down to a judgement call.

I would not consider this ABI. The ABI part is that the fp register
points to where fp/lr were saved.

> > > +#define EXTRA_MAGIC	0x45585401
> > > +
> > > +struct extra_context {
> > > +	struct _aarch64_ctx head;
> > > +	void __user *data;	/* 16-byte aligned pointer to extra space */
> > "__user" is a kernel-only attribute, we shouldn't expose it in a uapi
> > header.
> 
> This is filtered out by headers_install, just like #ifdef __KERNEL__.

Ah, ok, I missed this.

> > > +	__u32 size;		/* size in bytes of the extra space */
> > > +};
> > 
> > Do we need the size of the extra space? Can we not infer it anyway by
> > walking the contexts save there? Surely we don't expect more than one
> > extra context.
> 
> Strictly speaking we don't need it.  When userspace parses a signal
> frame generated by the kernel, it can trust the kernel to write a well-
> formed signal frame.
> 
> In sigreturn it allows us to retain a sanity-check on overall size
> similar to what sizeof(__reserved) gives us.  This "feels cleaner"
> to me, but the value of it is debatable, since we can still apply
> SIGFRAME_MAXSZ and uaccess should protect us against gross overruns.

I'm not keen on the size information, it seems superfluous.

BTW, does SIGFRAME_MAXSZ now become ABI? Or the user only needs to
interrogate the frame size and we keep this internal to the kernel?

-- 
Catalin