Re: [PATCH 22/30] bsd-user/signal.c: Fill in queue_signal

[Peter Maydell]

On Mon, 17 Jan 2022 at 16:22, Warner Losh <imp@bsdimp.com> wrote:
> On Thu, Jan 13, 2022 at 1:37 PM Peter Maydell <peter.maydell@linaro.org> 
> wrote:
>> > +    /*
>> > +     * FreeBSD signals are always queued.  Linux only queues real time 
>> > signals.
>> > +     * XXX this code is not thread safe.  "What lock protects ts->sigtab?"
>> > +     */
>>
>> ts->sigtab shouldn't need a lock, because it is per-thread,
>> like all of TaskState. (The TaskState structure is pointed
>> to by the CPUState 'opaque' field. CPUStates are per-thread;
>> the TaskState for a new thread's new CPUState is allocated
>> and initialized as part of the emulating of whatever the
>> "create new thread" syscall is. For Linux this is in
>> do_fork() for the CLONE_VM case. The TaskState for the
>> initial thread is allocated in main.c.) We do need to deal
>> with the fact that ts->sigtab can be updated by a signal
>> handler (which always runs in the thread corresponding to
>> that guest CPU): the linux-user process_pending_signals()
>> has been written with that in mind.
>
>
> Gotcha. That makes sense. Any reason that atomics aren't used
> for this between the different routines?

We use atomics in some places, eg the qatomic_read()/qatomic_set()
of ts->signal_pending in process_pending_signals. We deal with
some other races by blocking signals. sigtab's a cmplex
data structure, so simply making accesses to its individual
fields atomic isn't sufficient, and the more usual approach of
taking a lock doesn't work when the thing being protected against
is code running in a signal handler.

It's also quite possible that we missed some places
where we should be being stricter about using the atomic
accessors -- if you spot anything like that let us know.

thanks
-- PMM