Re: [Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Put AArch64 frame record in the right place

[Laurent Vivier]

Le 12/04/2018 à 16:02, Peter Maydell a écrit :
> AArch64 stack frames include a 'frame record' which holds a pointer
> to the next frame record in the chain and the LR on entry to the
> function. The procedure calling standard doesn't mandate where
> exactly this frame record is in the stack frame, but for signal
> frames the kernel puts it right at the top. We used to put it
> there too, but in commit 7f0f4208b3a96f22 we accidentally put
> the "enlarge to the 4K reserved space minimum" check after the
> "allow for the frame record" code, rather than before it, with
> the effect that the frame record would be inside the reserved
> space and immediately after the last used part of it.
> 
> Move the frame record back out of the reserved space to where
> we used to put it.
> 
> This bug shouldn't break any sensible guest code, but test
> programs that deliberately look at the internal details
> of the signal frame layout will not find what they are
> expecting to see.
> 
> Fixes: 7f0f4208b3a96f22
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> I'm marking this as for-2.12 on the basis that it puts our frame
> layout back to exactly what 2.11 had, and so seems safest.
> No sensible guest code should really care, though, so this is
> in the "only if we're doing an rc4" bucket; but I think that the
> softfloat fixes deserve an rc4 anyway.
> 
>  linux-user/signal.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 8d9e6e8410..e6dfe0adfd 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1843,6 +1843,12 @@ static void target_setup_frame(int usig, struct 
> target_sigaction *ka,
>          layout.total_size += sizeof(struct target_aarch64_ctx);
>      }
>  
> +    /* We must always provide at least the standard 4K reserved space,
> +     * even if we don't use all of it (this is part of the ABI)
> +     */
> +    layout.total_size = MAX(layout.total_size,
> +                            sizeof(struct target_rt_sigframe));
> +
>      /* Reserve space for the return code.  On a real system this would
>       * be within the VDSO.  So, despite the name this is not a "real"
>       * record within the frame.
> @@ -1850,12 +1856,6 @@ static void target_setup_frame(int usig, struct 
> target_sigaction *ka,
>      fr_ofs = layout.total_size;
>      layout.total_size += sizeof(struct target_rt_frame_record);
>  
> -    /* We must always provide at least the standard 4K reserved space,
> -     * even if we don't use all of it (this is part of the ABI)
> -     */
> -    layout.total_size = MAX(layout.total_size,
> -                            sizeof(struct target_rt_sigframe));
> -
>      frame_addr = get_sigframe(ka, env, layout.total_size);
>      trace_user_setup_frame(env, frame_addr);
>      if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> 

It's hard to compare this code with the one in kernel, but if I compare
the offset of "fr" in QEMU and the one of "next_frame" they seem identical.

Reviewed-by: Laurent Vivier <address@hidden>