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:

quoted

On Mon, 2020-10-05 at 14:42 +0100, Dave Martin wrote:

quoted

On Tue, Sep 29, 2020 at 01:57:43PM -0700, Chang S. Bae wrote:

quoted

+/*
+ * 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

quoted

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.

quoted

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