Re: [Qemu-devel] [ARM/FDPIC v2 4/4] linux-user: ARM-FDPIC: Add support for signals for FDPIC targets

[Peter Maydell]

On 23 April 2018 at 15:22, Christophe Lyon <address@hidden> wrote:
> On 23/04/2018 15:05, Peter Maydell wrote:
>>
>> On 23 April 2018 at 08:51, Christophe Lyon <address@hidden> wrote:
>>> @@ -2149,7 +2164,21 @@ setup_return(CPUARMState *env, struct
>>> target_sigaction *ka,
>>>   {
>>>       abi_ulong handler = ka->_sa_handler;
>>>       abi_ulong retcode;
>>> -    int thumb = handler & 1;
>>> +    abi_ulong funcdesc_ptr = 0;
>>> +
>>> +    int thumb;
>>> +    int is_fdpic = ((TaskState *)thread_cpu->opaque)->is_fdpic;
>>> +
>>> +    if (is_fdpic) {
>>> +        /* In FDPIC mode, ka->_sa_handler points to a function
>>> +         * descriptor (FD). The first word contains the address of the
>>> +         * handler. The second word contains the value of the PIC
>>> +         * register (r9).  */
>>> +        funcdesc_ptr = ka->_sa_handler;
>>
>>
>> You already have ka->_sa_handler in the 'handler' local, so you
>> can just use that.
>>
> OK, I thought it was clearer to use a different name.

The other way to structure that would be to put
    handler = ka->_sa_handler;

in an else clause of this if ().

>>> +        get_user_ual(handler, funcdesc_ptr);
>>
>>
>> You need to check whether get_user_ual() returned non-zero
>> (indicating that the descriptor isn't readable) and handle that.
>>
> Ha... I feared that :)
> I noticed that quite a lot of get_user_XXX() calls do not
> check the returned value, and since this patch changes
> void setup_return(), I'm not sure what to do in case of
> read error? Call abort() or is there a global flag
> for this purpose, that I should set and which would
> be checked by the caller?

You need to:
 * make setup_return() return an error indication
   (other code in this file seems to follow the kernel's
   example and have this be a return value that's true on
   an error and false otherwise)
 * have the callers check the error indication, and
   if there was an error do:
     - unlock the user struct
     - call force_sigsegv()

All the callers already have code for doing force_sigsegv,
though they don't yet have a codepath that handles doing
the unlock first. You should be able to just put in
the call to unlock_user_struct(frame, ...) at the existing 'sigsegv:'
labels, because that is guaranteed to be safe on a NULL
host pointer, which is what you'll get in the other
code paths that go to that label. Then you can have
the error check be
    if (setup_return(...)) {
        goto sigsegv;
    }

thanks
-- PMM