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 <[email protected]>
Reviewed-by: Len Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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/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) &&
--
2.17.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 <[email protected]>
> Reviewed-by: Len Brown <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
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
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
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
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