Re: [PATCH v18 02/13] linux-user: Add LoongArch signal support

[gaosong]

Hi Richard

On 2022/6/21 上午12:23, Richard Henderson wrote:
> On 6/20/22 02:33, Song Gao wrote:
> > +static int restore_sigcontext(CPULoongArchState *env,
> > +                               struct target_sigcontext *sc)
> > +{
> > +    int i;
> > +    int ret = 0;
> > +    struct extctx_layout extctx;
> > +
> > +    memset(&extctx, 0, sizeof(struct extctx_layout));
> > +
> > +    __get_user(extctx.flags, &sc->sc_flags);
> > +
> > +    ret = parse_extcontext(sc, &extctx);
> > +    if (ret < 0) {
> > +        goto bad;
> > +    }
> > +
> > +    __get_user(env->pc, &sc->sc_pc);
> > +    for (i = 1; i < 32; ++i) {
> > +        __get_user(env->gpr[i], &sc->sc_regs[i]);
> > +    }
> > +
> > +    if (extctx.fpu.addr) {
> > +        copy_fpu_from_sigcontext(env, &extctx);
> > +        restore_fp_status(env);
> > +    }
> > +bad:
> > +    return ret;
> > +}
>
> This is missing lock_user/unlock_user somewhere.
> You can't use the double-underscore __get/__put_user without having 
> done that.
My understanding is that the struct exctx need 
lock_user_struct/unlock_user_struct,  then we can use __get/__put the 
struct extctx.
> You can use the non-underscore get_user in parse_extcontext, and 
> separately lock the target_fpu_context.  Failures must goto invalid.
>
>
> > +void setup_rt_frame(int sig, struct target_sigaction *ka,
> > +                    target_siginfo_t *info,
> > +                    target_sigset_t *set, CPULoongArchState *env)
> > +{
> > +    struct target_rt_sigframe *frame;
> > +    struct extctx_layout extctx;
> > +    abi_ulong frame_addr;
> > +    int i;
> > +
> > +    frame_addr = get_sigframe(ka, env, sizeof(*frame), &extctx);
> > +    trace_user_setup_rt_frame(env, frame_addr);
> > +    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> > +        goto give_sigsegv;
> > +    }
>
> Similarly, this lock...
>
> > +
> > +    tswap_siginfo(&frame->rs_info, info);
> > +
> > +    __put_user(0, &frame->rs_uc.tuc_flags);
> > +    __put_user(0, &frame->rs_uc.tuc_link);
> > +    target_save_altstack(&frame->rs_uc.tuc_stack, env);
> > +
> > +    setup_sigcontext(env, &frame->rs_uc.tuc_mcontext, &extctx);
>
> ... fails to cover the extra memory allocated for extctx.
>
> This is why I suggested statically allocating the extra
> pieces of the signal frame *on write*.  You obviously
> cannot rely on the signal frame being identical on
> signal return -- the guest is allowed to create any valid
> context to give to rt_sigreturn.
I don’t know if my understanding is correct,

we can put the exctx or target_fpu_context into target_rt_sigframe, like 
this:
struct target_rt_sigframe {
    struct target_siginfo rs_info;
    struct target_ucontext rs_uc;
    struct extctx_layout rs_ext;
};
This way  that we just need lo lock/unlock target_rt_sigframe on 
setup_sigcontext/restore_sigcontext,

Thanks.
Song Gao