On Tue, Jan 16, 2018 at 01:11:49PM +0300, Yury Norov wrote:
> On Mon, Jan 15, 2018 at 05:22:01PM +0000, Dave Martin wrote:

[...]

> > I'll take a look at your code anyway in case there's something
> > else one of us didn't think of.
> 
> Thanks, Dave.
> 
> This is the branch:
> https://github.com/norov/linux/tree/ilp32-4.15-rc7
> 
> SVE-related changes are mostly in patches:
> arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext
> arm64: signal32: move ilp32 and aarch32 common code to separated file
> arm64: signal: share lp64 signal structures and routines to ilp32
> arm64: ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it

Thanks for the pointer.

Quick review of the relevant patches below.

> From 6f566a512cbac378ccee66094b5f9124b6069275 Mon Sep 17 00:00:00 2001
> From: Andrew Pinski <apin...@cavium.com>
> Date: Tue, 24 May 2016 03:04:47 +0300
> Subject: [PATCH 17/24] arm64: ilp32: add sys_ilp32.c and a separate table (in
>  entry.S) to use it
> 
> Add a separate syscall-table for ILP32, which dispatches either to native
> LP64 system call implementation or to compat-syscalls, as appropriate.
> 
> Signed-off-by: Andrew Pinski <andrew.pin...@caviumnetworks.com>
> Signed-off-by: Yury Norov <yno...@caviumnetworks.com>
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangj...@linaro.org>

[...]

> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 778726d..d2d7336 100644

[...]

> @@ -864,14 +882,15 @@ ENDPROC(ret_to_user)
>  el0_svc:
>       ldr     x16, [tsk, #TSK_TI_FLAGS]       // load thread flags
>       adrp    stbl, sys_call_table            // load syscall table pointer
> +     ldr     x19, [tsk, #TSK_TI_FLAGS]
>       mov     wscno, w8                       // syscall number in w8
>       mov     wsc_nr, #__NR_syscalls
>  
>  #ifdef CONFIG_ARM64_SVE
>  alternative_if_not ARM64_SVE
> -     b       el0_svc_naked
> +     b       el0_svc_select_table
>  alternative_else_nop_endif
> -     tbz     x16, #TIF_SVE, el0_svc_naked    // Skip unless TIF_SVE set:
> +     tbz     x16, #TIF_SVE, el0_svc_select_table     // Skip unless TIF_SVE 
> set:
>       bic     x16, x16, #_TIF_SVE             // discard SVE state
>       str     x16, [tsk, #TSK_TI_FLAGS]
>  
> @@ -887,12 +906,19 @@ alternative_else_nop_endif
>       msr     cpacr_el1, x9                   // synchronised by eret to el0
>  #endif
>  
> +el0_svc_select_table:
> +#ifdef CONFIG_ARM64_ILP32
> +     tst     x19, #_TIF_32BIT_AARCH64
> +     b.eq    el0_svc_naked                   // We are using LP64  syscall 
> table

Can tbz be used here?

> +     adrp    stbl, sys_call_ilp32_table      // load ilp32 syscall table 
> pointer
> +     delouse_input_regs
> +#endif
>  el0_svc_naked:                                       // compat entry point
>       stp     x0, xscno, [sp, #S_ORIG_X0]     // save the original x0 and 
> syscall number

[...]


> From 6e55e1c381aa4b6e10ac5eda0a587adf5558f438 Mon Sep 17 00:00:00 2001
> From: Yury Norov <yno...@caviumnetworks.com>
> Date: Mon, 26 Jun 2017 19:11:58 +0300
> Subject: [PATCH 18/24] arm64: signal: share lp64 signal structures and
>  routines to ilp32

Nit: Please ensure that the commit message makes sense without the
subject line, so that users of Mutt etc., can see all necessary context
in the message body when drafting a reply.

> After that, it will be possible to reuse it in ilp32.
> 
> Signed-off-by: Yury Norov <yno...@caviumnetworks.com>


[...]

> +#define parse_user_sigcontext(user, sf)                                      
> \
> +     __parse_user_sigcontext(user, &(sf)->uc.uc_mcontext, sf)

Nit: can this #define be kept next to the function it wraps?

> +
> +struct user_ctxs {
> +     struct fpsimd_context __user *fpsimd;
> +     struct sve_context __user *sve;
> +};
> +
> +struct frame_record {
> +     u64 fp;
> +     u64 lr;
> +};
> +struct rt_sigframe_user_layout;
> +
> +int setup_extra_context(char __user *sfp, unsigned long users, char __user 
> *userp);
> +int __parse_user_sigcontext(struct user_ctxs *user,
> +                                struct sigcontext __user const *sc,
> +                                void __user const *sigframe_base);

[...]

> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c

[...]

> +int setup_extra_context(char __user *sfp, unsigned long users, char __user 
> *userp)
> +{
> +     int err =0;

Nit: missing space.

Also, while "user<blah>" seemed OK as a local variable name, it now
looks rather obscure as a function parameter name, since the meaning of
the parameter is not so obvious.

"users" is really the total sigframe size, so "sf_size" may be a
reasonable name.

"extrap" might be a slightly better name for "userp", since this is not
a general-purpose cursor and must point to the base address computed for
the extra_context block.

(I'm open to better suggestions though.)

> +     struct extra_context __user *extra;
> +     struct _aarch64_ctx __user *end;
> +     u64 extra_datap;
> +     u32 extra_size;
> +
> +     extra = (struct extra_context __user *)userp;
> +     userp += EXTRA_CONTEXT_SIZE;
> +
> +     end = (struct _aarch64_ctx __user *)userp;
> +     userp += TERMINATOR_SIZE;
> +
> +     /*
> +      * extra_datap is just written to the signal frame.
> +      * The value gets cast back to a void __user *
> +      * during sigreturn.
> +      */
> +     extra_datap = (__force u64)userp;
> +     extra_size = sfp + round_up(users, 16) - userp;
> +
> +     __put_user_error(EXTRA_MAGIC, &extra->head.magic, err);
> +     __put_user_error(EXTRA_CONTEXT_SIZE, &extra->head.size, err);
> +     __put_user_error(extra_datap, &extra->datap, err);
> +     __put_user_error(extra_size, &extra->size, err);
> +
> +     /* Add the terminator */
> +     __put_user_error(0, &end->magic, err);
> +     __put_user_error(0, &end->size, err);
> +
> +     return err;
> +}

[...]

> @@ -652,39 +656,9 @@ static int setup_sigframe(struct rt_sigframe_user_layout 
> *user,
>               err |= preserve_sve_context(sve_ctx);
>       }
>  
> -     if (err == 0 && user->extra_offset) {
> -             char __user *sfp = (char __user *)user->sigframe;
> -             char __user *userp =
> -                     apply_user_offset(user, user->extra_offset);
> -
> -             struct extra_context __user *extra;
> -             struct _aarch64_ctx __user *end;
> -             u64 extra_datap;
> -             u32 extra_size;
> -
> -             extra = (struct extra_context __user *)userp;
> -             userp += EXTRA_CONTEXT_SIZE;
> -
> -             end = (struct _aarch64_ctx __user *)userp;
> -             userp += TERMINATOR_SIZE;
> -
> -             /*
> -              * extra_datap is just written to the signal frame.
> -              * The value gets cast back to a void __user *
> -              * during sigreturn.
> -              */
> -             extra_datap = (__force u64)userp;
> -             extra_size = sfp + round_up(user->size, 16) - userp;
> -
> -             __put_user_error(EXTRA_MAGIC, &extra->head.magic, err);
> -             __put_user_error(EXTRA_CONTEXT_SIZE, &extra->head.size, err);
> -             __put_user_error(extra_datap, &extra->datap, err);
> -             __put_user_error(extra_size, &extra->size, err);
> -
> -             /* Add the terminator */
> -             __put_user_error(0, &end->magic, err);
> -             __put_user_error(0, &end->size, err);
> -     }
> +     if (err == 0 && user->extra_offset)
> +             setup_extra_context((char *) user->sigframe, user->size,
> +                     (char *) apply_user_offset(user, user->extra_offset));

Nit: no space after (type *) please.

Also, can we have (char __user *)?  This is more "correct" because these
arguments are not valid kernel pointers.  Keeping the __user may avoid
sparse warnings.  (I've not tried to build the code yet, so I don't know
whether sparse actually complains about __user being cast on and off
here.)

[...]


> From 735931121210a692038859448bf9f4ac5905eb73 Mon Sep 17 00:00:00 2001
> From: Yury Norov <yno...@caviumnetworks.comk>
> Date: Tue, 24 May 2016 03:04:50 +0300
> Subject: [PATCH 20/24] arm64: ilp32: introduce ilp32-specific handlers for
>  sigframe and ucontext
>
> ILP32 uses AARCH32 compat structures and syscall handlers for signals.
> But ILP32 struct rt_sigframe  and ucontext differs from both LP64 and
> AARCH32. So some specific mechanism is needed to take care of it.
>
> Signed-off-by: Yury Norov <yno...@caviumnetworks.com>

The code here looks reasonably correct, but there is a lot of
unnecessary duplication of code that should really be common.

This code is security-critical and tricky to get right, so we don't
want to have to maintain and test two independent implementations of
anything if we can possibly avoid it.

I think there may at least one change in the LP64 code that has not
been propagated here (the call to
fpsimd_signal_preserve_current_state() in setup_rt_frame()).  There
will likely be more such cases in the future.

[...]

diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c
new file mode 100644
index 0000000..a1cb058
> --- /dev/null
> +++ b/arch/arm64/kernel/signal_ilp32.c

[...]

> +#define BASE_SIGFRAME_SIZE round_up(sizeof(struct ilp32_rt_sigframe), 16)
> +
> +struct ilp32_ucontext {
> +        u32          uc_flags;
> +        u32          uc_link;
> +        compat_stack_t  uc_stack;
> +        compat_sigset_t uc_sigmask;
> +        /* glibc uses a 1024-bit sigset_t */
> +        __u8            __unused[1024 / 8 - sizeof(compat_sigset_t)];
> +        /* last for future expansion */
> +        struct sigcontext uc_mcontext;
> +};
> +
> +struct ilp32_rt_sigframe {
> +     struct compat_siginfo info;
> +     struct ilp32_ucontext uc;
> +};
> +
> +struct ilp32_rt_sigframe_user_layout {
> +     struct ilp32_rt_sigframe __user *sigframe;

I think much of the duplication here flows from the fact that this
struct currently has a different type for ILP32 versus LP64, even
though all the important contents of the structure are equivalent for
the two cases.

Can we replace the sigframe pointer with a void __user *, or union {
        struct rt_sigframe __user *;
        struct ilp32_rt_sigframe __user *;
} ?

Putting a pointer to sigcontext in here may also help, since it
looks the same for both cases: only its location changes (I think).


This would allow us to use the same sigframe_user_layout struct for
the lp64 and ilp32 cases, which will make it easier to share code.

The __reserved[] and extra_context blocks and the records in them
should be handled identically for the two ABIs: the only thing that
should differ is the offset of __reserved[] in the complete signal
frame.


We should try hard to make as much code as possible generic here.  I
won't comment on the individual functions for now: I think they can
all be mostly or completely eliminated.

[...]

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reply via email to