On Mon, 17 Jan 2022 at 16:22, Warner Losh <i...@bsdimp.com> wrote: > On Thu, Jan 13, 2022 at 1:37 PM Peter Maydell <peter.mayd...@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
