Re: [Qemu-devel] [PATCH 00/34] linux-user: Fix signal race conditions and SA_RESTART

[Peter Maydell <peter.maydell@linaro.org>]

On 6 September 2015 at 00:56, Timothy E Baldwin
<T.E.Baldwin99@members.leeds.ac.uk> wrote:
> There are many races with signals in linux user:
>
>  - Multiple host signals in quick succession, fixed by keeping host signals
>    blocked, and checking if target signals are blocked before calling
>    target signal handler.
>  - Signal shortly before blocking system call, fixed by either:
>    - Block hosts signals, check and use host system call with
>      sigset_t parameter.
>    - Or check if signals are pending immediately before host system call
>      and if a signal arrives between the check and system call rewind
>      host instruction pointer to before the check. Also fixes SA_RESTART.
>  - Signal before or during sensitive system call, fixed in a similar manner.
>  - Close host and synchronous signals, partly fixed by implementing a separate
>    queue for synchronous signals which are dispatched first. The asynchronous
>    signal may still be delayed or lost rather than dispatched to another thread
>    or handled after exec().
>
> Also fixed:
>  - Errno array bounds.
>  - Default fatal actions occurring in the middle of target instructions.

Thanks for sending this patchset. This is really cool and we've needed
it for a long time...

> I have major problems testing the system call restarting:
>  - x86, ARM MIPS, PowerPC and SPARC sucessful tested.
>  - Microblaze and SH4 works without signals, but signal test case
>    crashes with or without my changes.
>  - Alpha works without signals, but don't have a toolchain.
>    to compile the signal test case.
>  - I have been unable to test UniCore32, OpenRISC, M68K, S390
>    and CRIS due to a lack of binaries and toolchains.

If the relevant maintainers don't get round to testing the patches
I think we can go ahead and commit anyway -- some of our minor
architectures are somewhat unmaintained. It's probably worth cc'ing
the architecture maintainers on the next round of this series
(look for who's listed for target-whatever in MAINTAINERS).

I ran this patchset through the Linux Test Project test suite for
the ARM target. Unfortunately it has a handful of extra test failures.
For instance:
open08      2  TFAIL  :  call succeeded unexpectedly
(a test which should have failed EISDIR)

read02      2  TFAIL  :  call succeeded unexpectedly
should also have failed EISDIR and didn't

The waitid01 and waitid02 tests also failed, as did a few others.
Some of the fcntl tests seemed to hang.

(http://wiki.qemu.org/Testing/LTP has the info on how to run the
tests if you want to try it yourself.)

You should probably run your patches through scripts/checkpatch.pl,
which will flag up formatting problems. (Don't worry about trying
to do that for the "reindent signal.c" patch, fixing the style
issues in the same commit there would make the patch hard to review
with diff -w.)

It might be wise to rearrange the patchseries a little, so all
the patches which do the 'support restarting syscalls' work
plus the 'safe_syscall' implementation go at the start. I think
these are more obviously correct and easier to review than the
changes to the signal handling code in 'Fix race between multiple
signals' and later patches. So they might be able to go into the
tree ahead of the rest if the later patches need to go through
further rounds of review.

I'll send out my comments on the first half or so of the
patchset in a moment; the second half I need to think about
in more detail first.

thanks
-- PMM