diff mbox series

[RFC,1/4] x86/signal: Introduce helpers to get the maximum signal frame size

Message ID 20200929205746.6763-2-chang.seok.bae@intel.com
State New
Headers show
Series x86: Improve Minimum Alternate Stack Size | expand

Commit Message

Chang S. Bae Sept. 29, 2020, 8:57 p.m. UTC
Signal frames do not have a fixed format and can vary in size when a number
of things change: support XSAVE features, 32 vs. 64-bit apps. Add the code
to support a runtime method for userspace to dynamically discover how large
a signal stack needs to be.

Introduce a new variable, max_frame_size, and helper functions for the
calculation to be used in a new user interface. Set max_frame_size to a
system-wide worst-case value, instead of storing multiple app-specific
values.

Locate the body of the helper function -- fpu__get_fpstate_sigframe_size()
in fpu/signal.c for its relevance.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/fpu/signal.h |  2 ++
 arch/x86/include/asm/sigframe.h   | 23 ++++++++++++++++
 arch/x86/kernel/cpu/common.c      |  3 +++
 arch/x86/kernel/fpu/signal.c      | 20 ++++++++++++++
 arch/x86/kernel/signal.c          | 45 +++++++++++++++++++++++++++++++
 5 files changed, 93 insertions(+)

Comments

Dave Martin Oct. 5, 2020, 1:42 p.m. UTC | #1
On Tue, Sep 29, 2020 at 01:57:43PM -0700, Chang S. Bae wrote:
> Signal frames do not have a fixed format and can vary in size when a number
> of things change: support XSAVE features, 32 vs. 64-bit apps. Add the code
> to support a runtime method for userspace to dynamically discover how large
> a signal stack needs to be.
> 
> Introduce a new variable, max_frame_size, and helper functions for the
> calculation to be used in a new user interface. Set max_frame_size to a
> system-wide worst-case value, instead of storing multiple app-specific
> values.
> 
> Locate the body of the helper function -- fpu__get_fpstate_sigframe_size()
> in fpu/signal.c for its relevance.
> 
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  arch/x86/include/asm/fpu/signal.h |  2 ++
>  arch/x86/include/asm/sigframe.h   | 23 ++++++++++++++++
>  arch/x86/kernel/cpu/common.c      |  3 +++
>  arch/x86/kernel/fpu/signal.c      | 20 ++++++++++++++
>  arch/x86/kernel/signal.c          | 45 +++++++++++++++++++++++++++++++
>  5 files changed, 93 insertions(+)

[...]

> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index be0d7d4152ec..239a0b23a4b0 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -663,6 +663,51 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	return 0;
>  }
>  
> +/*
> + * The FP state frame contains an XSAVE buffer which must be 64-byte aligned.
> + * If a signal frame starts at an unaligned address, extra space is required.
> + * This is the max alignment padding, conservatively.
> + */
> +#define MAX_XSAVE_PADDING	63UL
> +
> +/*
> + * The frame data is composed of the following areas and laid out as:
> + *
> + * -------------------------
> + * | alignment padding     |
> + * -------------------------
> + * | (f)xsave frame        |
> + * -------------------------
> + * | fsave header          |
> + * -------------------------
> + * | siginfo + ucontext    |
> + * -------------------------
> + */
> +
> +/* max_frame_size tells userspace the worst case signal stack size. */
> +static unsigned long __ro_after_init max_frame_size;
> +
> +void __init init_sigframe_size(void)
> +{
> +	/*
> +	 * Use the largest of possible structure formats. This might
> +	 * slightly oversize the frame for 64-bit apps.
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_X86_32) ||
> +	    IS_ENABLED(CONFIG_IA32_EMULATION))
> +		max_frame_size = max((unsigned long)SIZEOF_sigframe_ia32,
> +				     (unsigned long)SIZEOF_rt_sigframe_ia32);
> +
> +	if (IS_ENABLED(CONFIG_X86_X32_ABI))
> +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe_x32);
> +
> +	if (IS_ENABLED(CONFIG_X86_64))
> +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe);
> +
> +	max_frame_size += fpu__get_fpstate_sigframe_size() + MAX_XSAVE_PADDING;

For arm64, we round the worst-case padding up by one.

I can't remember the full rationale for this, but it at least seemed a
bit weird to report a size that is not a multiple of the alignment.

I'm can't think of a clear argument as to why it really matters, though.

[...]

Cheers
---Dave
Michael Kerrisk \(man-pages\) via Libc-alpha Oct. 6, 2020, 5:45 p.m. UTC | #2
On Mon, 2020-10-05 at 14:42 +0100, Dave Martin wrote:
> On Tue, Sep 29, 2020 at 01:57:43PM -0700, Chang S. Bae wrote:
> > 
> > +/*
> > + * The FP state frame contains an XSAVE buffer which must be 64-byte aligned.
> > + * If a signal frame starts at an unaligned address, extra space is required.
> > + * This is the max alignment padding, conservatively.
> > + */
> > +#define MAX_XSAVE_PADDING	63UL
> > +
> > +/*
> > + * The frame data is composed of the following areas and laid out as:
> > + *
> > + * -------------------------
> > + * | alignment padding     |
> > + * -------------------------
> > + * | (f)xsave frame        |
> > + * -------------------------
> > + * | fsave header          |
> > + * -------------------------
> > + * | siginfo + ucontext    |
> > + * -------------------------
> > + */
> > +
> > +/* max_frame_size tells userspace the worst case signal stack size. */
> > +static unsigned long __ro_after_init max_frame_size;
> > +
> > +void __init init_sigframe_size(void)
> > +{
> > +	/*
> > +	 * Use the largest of possible structure formats. This might
> > +	 * slightly oversize the frame for 64-bit apps.
> > +	 */
> > +
> > +	if (IS_ENABLED(CONFIG_X86_32) ||
> > +	    IS_ENABLED(CONFIG_IA32_EMULATION))
> > +		max_frame_size = max((unsigned long)SIZEOF_sigframe_ia32,
> > +				     (unsigned long)SIZEOF_rt_sigframe_ia32);
> > +
> > +	if (IS_ENABLED(CONFIG_X86_X32_ABI))
> > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe_x32);
> > +
> > +	if (IS_ENABLED(CONFIG_X86_64))
> > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe);
> > +
> > +	max_frame_size += fpu__get_fpstate_sigframe_size() + MAX_XSAVE_PADDING;
> 
> For arm64, we round the worst-case padding up by one.
> 

Yeah, I saw that. The ARM code adds the max padding, too:

	signal_minsigstksz = sigframe_size(&user) +
		round_up(sizeof(struct frame_record), 16) +
		16; /* max alignment padding */


https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/signal.c#n973

> I can't remember the full rationale for this, but it at least seemed a
> bit weird to report a size that is not a multiple of the alignment.
> 

Because the last state size of XSAVE may not be 64B aligned, the (reported)
sum of xstate size here does not guarantee 64B alignment.

> I'm can't think of a clear argument as to why it really matters, though.

We care about the start of XSAVE buffer for the XSAVE instructions, to be
64B-aligned.

Thanks,
Chang
Dave Martin Oct. 7, 2020, 10:05 a.m. UTC | #3
On Tue, Oct 06, 2020 at 05:45:24PM +0000, Bae, Chang Seok wrote:
> On Mon, 2020-10-05 at 14:42 +0100, Dave Martin wrote:
> > On Tue, Sep 29, 2020 at 01:57:43PM -0700, Chang S. Bae wrote:
> > > 
> > > +/*
> > > + * The FP state frame contains an XSAVE buffer which must be 64-byte aligned.
> > > + * If a signal frame starts at an unaligned address, extra space is required.
> > > + * This is the max alignment padding, conservatively.
> > > + */
> > > +#define MAX_XSAVE_PADDING	63UL
> > > +
> > > +/*
> > > + * The frame data is composed of the following areas and laid out as:
> > > + *
> > > + * -------------------------
> > > + * | alignment padding     |
> > > + * -------------------------
> > > + * | (f)xsave frame        |
> > > + * -------------------------
> > > + * | fsave header          |
> > > + * -------------------------
> > > + * | siginfo + ucontext    |
> > > + * -------------------------
> > > + */
> > > +
> > > +/* max_frame_size tells userspace the worst case signal stack size. */
> > > +static unsigned long __ro_after_init max_frame_size;
> > > +
> > > +void __init init_sigframe_size(void)
> > > +{
> > > +	/*
> > > +	 * Use the largest of possible structure formats. This might
> > > +	 * slightly oversize the frame for 64-bit apps.
> > > +	 */
> > > +
> > > +	if (IS_ENABLED(CONFIG_X86_32) ||
> > > +	    IS_ENABLED(CONFIG_IA32_EMULATION))
> > > +		max_frame_size = max((unsigned long)SIZEOF_sigframe_ia32,
> > > +				     (unsigned long)SIZEOF_rt_sigframe_ia32);
> > > +
> > > +	if (IS_ENABLED(CONFIG_X86_X32_ABI))
> > > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe_x32);
> > > +
> > > +	if (IS_ENABLED(CONFIG_X86_64))
> > > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe);
> > > +
> > > +	max_frame_size += fpu__get_fpstate_sigframe_size() + MAX_XSAVE_PADDING;
> > 
> > For arm64, we round the worst-case padding up by one.
> > 
> 
> Yeah, I saw that. The ARM code adds the max padding, too:
> 
> 	signal_minsigstksz = sigframe_size(&user) +
> 		round_up(sizeof(struct frame_record), 16) +
> 		16; /* max alignment padding */
> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/signal.c#n973
> 
> > I can't remember the full rationale for this, but it at least seemed a
> > bit weird to report a size that is not a multiple of the alignment.
> > 
> 
> Because the last state size of XSAVE may not be 64B aligned, the (reported)
> sum of xstate size here does not guarantee 64B alignment.
> 
> > I'm can't think of a clear argument as to why it really matters, though.
> 
> We care about the start of XSAVE buffer for the XSAVE instructions, to be
> 64B-aligned.

Ah, I see.  That makes sense.

For arm64, there is no additional alignment padding inside the frame,
only the padding inserted after the frame to ensure that the base
address is 16-byte aligned.

However, I wonder whether people will tend to assume that AT_MINSIGSTKSZ
is a sensible (if minimal) amount of stack to allocate.  Allocating an
odd number of bytes, or any amount that isn't a multiple of the
architecture's preferred (or mandated) stack alignment probably doesn't
make sense.

AArch64 has a mandatory stack alignment of 16 bytes; I'm not sure about
x86.

Cheers
---Dave
Michael Kerrisk \(man-pages\) via Libc-alpha Oct. 8, 2020, 10:43 p.m. UTC | #4
On Wed, 2020-10-07 at 11:05 +0100, Dave Martin wrote:
> On Tue, Oct 06, 2020 at 05:45:24PM +0000, Bae, Chang Seok wrote:
> > On Mon, 2020-10-05 at 14:42 +0100, Dave Martin wrote:
> > > On Tue, Sep 29, 2020 at 01:57:43PM -0700, Chang S. Bae wrote:
> > > > 
> > > > +/*
> > > > + * The FP state frame contains an XSAVE buffer which must be 64-byte aligned.
> > > > + * If a signal frame starts at an unaligned address, extra space is required.
> > > > + * This is the max alignment padding, conservatively.
> > > > + */
> > > > +#define MAX_XSAVE_PADDING	63UL
> > > > +
> > > > +/*
> > > > + * The frame data is composed of the following areas and laid out as:
> > > > + *
> > > > + * -------------------------
> > > > + * | alignment padding     |
> > > > + * -------------------------
> > > > + * | (f)xsave frame        |
> > > > + * -------------------------
> > > > + * | fsave header          |
> > > > + * -------------------------
> > > > + * | siginfo + ucontext    |
> > > > + * -------------------------
> > > > + */
> > > > +
> > > > +/* max_frame_size tells userspace the worst case signal stack size. */
> > > > +static unsigned long __ro_after_init max_frame_size;
> > > > +
> > > > +void __init init_sigframe_size(void)
> > > > +{
> > > > +	/*
> > > > +	 * Use the largest of possible structure formats. This might
> > > > +	 * slightly oversize the frame for 64-bit apps.
> > > > +	 */
> > > > +
> > > > +	if (IS_ENABLED(CONFIG_X86_32) ||
> > > > +	    IS_ENABLED(CONFIG_IA32_EMULATION))
> > > > +		max_frame_size = max((unsigned long)SIZEOF_sigframe_ia32,
> > > > +				     (unsigned long)SIZEOF_rt_sigframe_ia32);
> > > > +
> > > > +	if (IS_ENABLED(CONFIG_X86_X32_ABI))
> > > > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe_x32);
> > > > +
> > > > +	if (IS_ENABLED(CONFIG_X86_64))
> > > > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe);
> > > > +
> > > > +	max_frame_size += fpu__get_fpstate_sigframe_size() + MAX_XSAVE_PADDING;
> > > 
> > > For arm64, we round the worst-case padding up by one.
> > > 
> > 
> > Yeah, I saw that. The ARM code adds the max padding, too:
> > 
> > 	signal_minsigstksz = sigframe_size(&user) +
> > 		round_up(sizeof(struct frame_record), 16) +
> > 		16; /* max alignment padding */
> > 
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/signal.c#n973
> > 
> > > I can't remember the full rationale for this, but it at least seemed a
> > > bit weird to report a size that is not a multiple of the alignment.
> > > 
> > 
> > Because the last state size of XSAVE may not be 64B aligned, the (reported)
> > sum of xstate size here does not guarantee 64B alignment.
> > 
> > > I'm can't think of a clear argument as to why it really matters, though.
> > 
> > We care about the start of XSAVE buffer for the XSAVE instructions, to be
> > 64B-aligned.
> 
> Ah, I see.  That makes sense.
> 
> For arm64, there is no additional alignment padding inside the frame,
> only the padding inserted after the frame to ensure that the base
> address is 16-byte aligned.
> 
> However, I wonder whether people will tend to assume that AT_MINSIGSTKSZ
> is a sensible (if minimal) amount of stack to allocate.  Allocating an
> odd number of bytes, or any amount that isn't a multiple of the
> architecture's preferred (or mandated) stack alignment probably doesn't
> make sense.
> 
> AArch64 has a mandatory stack alignment of 16 bytes; I'm not sure about
> x86.

The x86 ABI looks to require 16-byte alignment (for both 32-/64-bit modes).
FWIW, the 32-bit ABI got changed from 4-byte alignement.

Thank you for brining up the point. Ack. The kernel is expected to return a
16-byte aligned size. I made this change after a discussion with H.J.:

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index c042236ef52e..52815d7c08fb 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -212,6 +212,11 @@ do {							
		\
  * Set up a signal frame.
  */
 
+/* x86 ABI requires 16-byte alignment */
+#define FRAME_ALIGNMENT	16UL
+
+#define MAX_FRAME_PADDING	FRAME_ALIGNMENT - 1
+
 /*
  * Determine which stack to use..
  */
@@ -222,9 +227,9 @@ static unsigned long align_sigframe(unsigned long sp)
 	 * Align the stack pointer according to the i386 ABI,
 	 * i.e. so that on function entry ((sp + 4) & 15) == 0.
 	 */
-	sp = ((sp + 4) & -16ul) - 4;
+	sp = ((sp + 4) & -FRAME_ALIGNMENT) - 4;
 #else /* !CONFIG_X86_32 */
-	sp = round_down(sp, 16) - 8;
+	sp = round_down(sp, FRAME_ALIGNMENT) - 8;
 #endif
 	return sp;
 }
@@ -404,7 +409,7 @@ static int __setup_rt_frame(int sig, struct ksignal
*ksig,
 	unsafe_put_sigcontext(&frame->uc.uc_mcontext, fp, regs, set,
Efault);
 	unsafe_put_sigmask(set, frame, Efault);
 	user_access_end();
-	
+
 	if (copy_siginfo_to_user(&frame->info, &ksig->info))
 		return -EFAULT;
 
@@ -685,6 +690,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
  * -------------------------
  * | fsave header          |
  * -------------------------
+ * | alignment padding     |
+ * -------------------------
  * | siginfo + ucontext    |
  * -------------------------
  */
@@ -710,7 +717,12 @@ void __init init_sigframe_size(void)
 	if (IS_ENABLED(CONFIG_X86_64))
 		max_frame_size = max(max_frame_size, (unsigned
long)SIZEOF_rt_sigframe);
 
+	max_frame_size += MAX_FRAME_PADDING;
+
 	max_frame_size += fpu__get_fpstate_sigframe_size() +
MAX_XSAVE_PADDING;
+
+	/* Userspace expects an aligned size. */
+	max_frame_size = round_up(max_frame_size, FRAME_ALIGNMENT);
 }
 
 unsigned long get_sigframe_size(void)


Thanks,
Chang
Dave Martin Oct. 12, 2020, 1:26 p.m. UTC | #5
On Thu, Oct 08, 2020 at 10:43:50PM +0000, Bae, Chang Seok wrote:
> On Wed, 2020-10-07 at 11:05 +0100, Dave Martin wrote:
> > On Tue, Oct 06, 2020 at 05:45:24PM +0000, Bae, Chang Seok wrote:
> > > On Mon, 2020-10-05 at 14:42 +0100, Dave Martin wrote:
> > > > On Tue, Sep 29, 2020 at 01:57:43PM -0700, Chang S. Bae wrote:
> > > > > 
> > > > > +/*
> > > > > + * The FP state frame contains an XSAVE buffer which must be 64-byte aligned.
> > > > > + * If a signal frame starts at an unaligned address, extra space is required.
> > > > > + * This is the max alignment padding, conservatively.
> > > > > + */
> > > > > +#define MAX_XSAVE_PADDING	63UL
> > > > > +
> > > > > +/*
> > > > > + * The frame data is composed of the following areas and laid out as:
> > > > > + *
> > > > > + * -------------------------
> > > > > + * | alignment padding     |
> > > > > + * -------------------------
> > > > > + * | (f)xsave frame        |
> > > > > + * -------------------------
> > > > > + * | fsave header          |
> > > > > + * -------------------------
> > > > > + * | siginfo + ucontext    |
> > > > > + * -------------------------
> > > > > + */
> > > > > +
> > > > > +/* max_frame_size tells userspace the worst case signal stack size. */
> > > > > +static unsigned long __ro_after_init max_frame_size;
> > > > > +
> > > > > +void __init init_sigframe_size(void)
> > > > > +{
> > > > > +	/*
> > > > > +	 * Use the largest of possible structure formats. This might
> > > > > +	 * slightly oversize the frame for 64-bit apps.
> > > > > +	 */
> > > > > +
> > > > > +	if (IS_ENABLED(CONFIG_X86_32) ||
> > > > > +	    IS_ENABLED(CONFIG_IA32_EMULATION))
> > > > > +		max_frame_size = max((unsigned long)SIZEOF_sigframe_ia32,
> > > > > +				     (unsigned long)SIZEOF_rt_sigframe_ia32);
> > > > > +
> > > > > +	if (IS_ENABLED(CONFIG_X86_X32_ABI))
> > > > > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe_x32);
> > > > > +
> > > > > +	if (IS_ENABLED(CONFIG_X86_64))
> > > > > +		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe);
> > > > > +
> > > > > +	max_frame_size += fpu__get_fpstate_sigframe_size() + MAX_XSAVE_PADDING;
> > > > 
> > > > For arm64, we round the worst-case padding up by one.
> > > > 
> > > 
> > > Yeah, I saw that. The ARM code adds the max padding, too:
> > > 
> > > 	signal_minsigstksz = sigframe_size(&user) +
> > > 		round_up(sizeof(struct frame_record), 16) +
> > > 		16; /* max alignment padding */
> > > 
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/signal.c#n973
> > > 
> > > > I can't remember the full rationale for this, but it at least seemed a
> > > > bit weird to report a size that is not a multiple of the alignment.
> > > > 
> > > 
> > > Because the last state size of XSAVE may not be 64B aligned, the (reported)
> > > sum of xstate size here does not guarantee 64B alignment.
> > > 
> > > > I'm can't think of a clear argument as to why it really matters, though.
> > > 
> > > We care about the start of XSAVE buffer for the XSAVE instructions, to be
> > > 64B-aligned.
> > 
> > Ah, I see.  That makes sense.
> > 
> > For arm64, there is no additional alignment padding inside the frame,
> > only the padding inserted after the frame to ensure that the base
> > address is 16-byte aligned.
> > 
> > However, I wonder whether people will tend to assume that AT_MINSIGSTKSZ
> > is a sensible (if minimal) amount of stack to allocate.  Allocating an
> > odd number of bytes, or any amount that isn't a multiple of the
> > architecture's preferred (or mandated) stack alignment probably doesn't
> > make sense.
> > 
> > AArch64 has a mandatory stack alignment of 16 bytes; I'm not sure about
> > x86.
> 
> The x86 ABI looks to require 16-byte alignment (for both 32-/64-bit modes).
> FWIW, the 32-bit ABI got changed from 4-byte alignement.
> 
> Thank you for brining up the point. Ack. The kernel is expected to return a
> 16-byte aligned size. I made this change after a discussion with H.J.:
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index c042236ef52e..52815d7c08fb 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -212,6 +212,11 @@ do {							
> 		\
>   * Set up a signal frame.
>   */
>  
> +/* x86 ABI requires 16-byte alignment */
> +#define FRAME_ALIGNMENT	16UL
> +
> +#define MAX_FRAME_PADDING	FRAME_ALIGNMENT - 1
> +

You might want () here, to avoid future surpsises.

>  /*
>   * Determine which stack to use..
>   */
> @@ -222,9 +227,9 @@ static unsigned long align_sigframe(unsigned long sp)
>  	 * Align the stack pointer according to the i386 ABI,
>  	 * i.e. so that on function entry ((sp + 4) & 15) == 0.
>  	 */
> -	sp = ((sp + 4) & -16ul) - 4;
> +	sp = ((sp + 4) & -FRAME_ALIGNMENT) - 4;
>  #else /* !CONFIG_X86_32 */
> -	sp = round_down(sp, 16) - 8;
> +	sp = round_down(sp, FRAME_ALIGNMENT) - 8;
>  #endif
>  	return sp;
>  }
> @@ -404,7 +409,7 @@ static int __setup_rt_frame(int sig, struct ksignal
> *ksig,
>  	unsafe_put_sigcontext(&frame->uc.uc_mcontext, fp, regs, set,
> Efault);
>  	unsafe_put_sigmask(set, frame, Efault);
>  	user_access_end();
> -	
> +
>  	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>  		return -EFAULT;
>  
> @@ -685,6 +690,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>   * -------------------------
>   * | fsave header          |
>   * -------------------------
> + * | alignment padding     |
> + * -------------------------
>   * | siginfo + ucontext    |
>   * -------------------------
>   */
> @@ -710,7 +717,12 @@ void __init init_sigframe_size(void)
>  	if (IS_ENABLED(CONFIG_X86_64))
>  		max_frame_size = max(max_frame_size, (unsigned
> long)SIZEOF_rt_sigframe);
>  
> +	max_frame_size += MAX_FRAME_PADDING;
> +
>  	max_frame_size += fpu__get_fpstate_sigframe_size() +
> MAX_XSAVE_PADDING;
> +
> +	/* Userspace expects an aligned size. */
> +	max_frame_size = round_up(max_frame_size, FRAME_ALIGNMENT);
>  }

[...]

Seems reasonable, I guess.

(I won't comment on the x86 ABI specifics.)

Cheers
---Dave
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
index 7fb516b6893a..5bfbf8f2e5a3 100644
--- a/arch/x86/include/asm/fpu/signal.h
+++ b/arch/x86/include/asm/fpu/signal.h
@@ -29,6 +29,8 @@  unsigned long
 fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
 		     unsigned long *buf_fx, unsigned long *size);
 
+unsigned long fpu__get_fpstate_sigframe_size(void);
+
 extern void fpu__init_prepare_fx_sw_frame(void);
 
 #endif /* _ASM_X86_FPU_SIGNAL_H */
diff --git a/arch/x86/include/asm/sigframe.h b/arch/x86/include/asm/sigframe.h
index 84eab2724875..ac77f3f90bc9 100644
--- a/arch/x86/include/asm/sigframe.h
+++ b/arch/x86/include/asm/sigframe.h
@@ -52,6 +52,15 @@  struct rt_sigframe_ia32 {
 	char retcode[8];
 	/* fp state follows here */
 };
+
+#define SIZEOF_sigframe_ia32	sizeof(struct sigframe_ia32)
+#define SIZEOF_rt_sigframe_ia32	sizeof(struct rt_sigframe_ia32)
+
+#else
+
+#define SIZEOF_sigframe_ia32	0
+#define SIZEOF_rt_sigframe_ia32	0
+
 #endif /* defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) */
 
 #ifdef CONFIG_X86_64
@@ -81,8 +90,22 @@  struct rt_sigframe_x32 {
 	/* fp state follows here */
 };
 
+#define SIZEOF_rt_sigframe_x32	sizeof(struct rt_sigframe_x32)
+
 #endif /* CONFIG_X86_X32_ABI */
 
+#define SIZEOF_rt_sigframe	sizeof(struct rt_sigframe)
+
+#else
+
+#define SIZEOF_rt_sigframe	0
+
 #endif /* CONFIG_X86_64 */
 
+#ifndef SIZEOF_rt_sigframe_x32
+#define SIZEOF_rt_sigframe_x32	0
+#endif
+
+void __init init_sigframe_size(void);
+
 #endif /* _ASM_X86_SIGFRAME_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c5d6f17d9b9d..d0363e15ec2e 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -57,6 +57,7 @@ 
 #include <asm/intel-family.h>
 #include <asm/cpu_device_id.h>
 #include <asm/uv/uv.h>
+#include <asm/sigframe.h>
 
 #include "cpu.h"
 
@@ -1276,6 +1277,8 @@  static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
 	fpu__init_system(c);
 
+	init_sigframe_size();
+
 #ifdef CONFIG_X86_32
 	/*
 	 * Regardless of whether PCID is enumerated, the SDM says
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index a4ec65317a7f..9f009525f551 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -507,6 +507,26 @@  fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
 
 	return sp;
 }
+
+unsigned long fpu__get_fpstate_sigframe_size(void)
+{
+	unsigned long xstate_size = xstate_sigframe_size();
+	unsigned long fsave_header_size = 0;
+
+	/*
+	 * This space is needed on (most) 32-bit kernels, or when a 32-bit
+	 * app is running on a 64-bit kernel. To keep things simple, just
+	 * assume the worst case and always include space for 'freg_state',
+	 * even for 64-bit apps on 64-bit kernels. This wastes a bit of
+	 * space, but keeps the code simple.
+	 */
+	if ((IS_ENABLED(CONFIG_IA32_EMULATION) ||
+	     IS_ENABLED(CONFIG_X86_32)) && use_fxsr())
+		fsave_header_size = sizeof(struct fregs_state);
+
+	return fsave_header_size + xstate_size;
+}
+
 /*
  * Prepare the SW reserved portion of the fxsave memory layout, indicating
  * the presence of the extended state information in the memory layout
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index be0d7d4152ec..239a0b23a4b0 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -663,6 +663,51 @@  SYSCALL_DEFINE0(rt_sigreturn)
 	return 0;
 }
 
+/*
+ * The FP state frame contains an XSAVE buffer which must be 64-byte aligned.
+ * If a signal frame starts at an unaligned address, extra space is required.
+ * This is the max alignment padding, conservatively.
+ */
+#define MAX_XSAVE_PADDING	63UL
+
+/*
+ * The frame data is composed of the following areas and laid out as:
+ *
+ * -------------------------
+ * | alignment padding     |
+ * -------------------------
+ * | (f)xsave frame        |
+ * -------------------------
+ * | fsave header          |
+ * -------------------------
+ * | siginfo + ucontext    |
+ * -------------------------
+ */
+
+/* max_frame_size tells userspace the worst case signal stack size. */
+static unsigned long __ro_after_init max_frame_size;
+
+void __init init_sigframe_size(void)
+{
+	/*
+	 * Use the largest of possible structure formats. This might
+	 * slightly oversize the frame for 64-bit apps.
+	 */
+
+	if (IS_ENABLED(CONFIG_X86_32) ||
+	    IS_ENABLED(CONFIG_IA32_EMULATION))
+		max_frame_size = max((unsigned long)SIZEOF_sigframe_ia32,
+				     (unsigned long)SIZEOF_rt_sigframe_ia32);
+
+	if (IS_ENABLED(CONFIG_X86_X32_ABI))
+		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe_x32);
+
+	if (IS_ENABLED(CONFIG_X86_64))
+		max_frame_size = max(max_frame_size, (unsigned long)SIZEOF_rt_sigframe);
+
+	max_frame_size += fpu__get_fpstate_sigframe_size() + MAX_XSAVE_PADDING;
+}
+
 static inline int is_ia32_compat_frame(struct ksignal *ksig)
 {
 	return IS_ENABLED(CONFIG_IA32_EMULATION) &&