Re: [Qemu-devel] [PATCH] Actually block signals that have been queued by Qemu.

[Timothy Baldwin]

On 08/03/15 10:23, Peter Maydell wrote:
> On 8 March 2015 at 04:18, Timothy Baldwin
> <address@hidden> wrote:
> >  From ebcb08f4f4ed90c8557cafc02593360c59e10a49 Mon Sep 17 00:00:00 2001
> > From: Timothy E Baldwin <address@hidden>
> > Date: Sat, 7 Mar 2015 18:42:41 +0000
> > Subject: [PATCH] Actually block signals that have been queued in TaskState.
> >
> > Actually block signals that have been queued in TaskState.
> > For this purpose the set of block signals is stored in TaskState.
> >
> > Fixes bug #1429313
> >
> > Signed-off-by: Timothy Baldwin <address@hidden>
> Thanks for this patch. How much testing of it have you done
> beyond checking that it fixes the issue you've run into?
> I ask because our signal handling behaviour in linux-user is
> definitely dubious in several ways, so if we're unlucky then
> fixing this might break some guest programs that happen to work
> by fluke right now...
I do have SIGILL being handled in a guest to emulate ARM floating point 
(after disabling Qemu internal floating point support) I don't know what 
else to test it with, I could try Qemu itself.
> > ---
> >   linux-user/qemu.h   |  2 +-
> >   linux-user/signal.c | 39 ++++++++++++++++++++-------------------
> >   2 files changed, 21 insertions(+), 20 deletions(-)
> >
> > diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> > index 8012cc2..7de543b 100644
> > --- a/linux-user/qemu.h
> > +++ b/linux-user/qemu.h
> > @@ -127,13 +127,13 @@ typedef struct TaskState {
> >   #endif
> >       uint32_t stack_base;
> >       int used; /* non zero if used */
> > -    bool sigsegv_blocked; /* SIGSEGV blocked by guest */
> >       struct image_info *info;
> >       struct linux_binprm *bprm;
> >        struct emulated_sigtable sigtab[TARGET_NSIG];
> >       struct sigqueue sigqueue_table[MAX_SIGQUEUE_SIZE]; /* siginfo queue */
> >       struct sigqueue *first_free; /* first free siginfo queue entry */
> > +    sigset_t signal_mask;
> >       int signal_pending; /* non zero if a signal may be pending */
> >   } __attribute__((aligned(16))) TaskState;
> >   diff --git a/linux-user/signal.c b/linux-user/signal.c
> > index 5bb399e..0997c45 100644
> > --- a/linux-user/signal.c
> > +++ b/linux-user/signal.c
> > @@ -209,40 +209,38 @@ int do_sigprocmask(int how, const sigset_t *set,
> > sigset_t *oldset)
> >       sigset_t *temp = NULL;
> >       CPUState *cpu = thread_cpu;
> >       TaskState *ts = (TaskState *)cpu->opaque;
> > -    bool segv_was_blocked = ts->sigsegv_blocked;
> > +
> > +    if (oldset) {
> > +        *oldset = ts->signal_mask;
> > +    }
> >        if (set) {
> > -        bool has_sigsegv = sigismember(set, SIGSEGV);
> >           val = *set;
> >           temp = &val;
> > -
> > -        sigdelset(temp, SIGSEGV);
> > +        int i;
> Variable declarations should go at the start of a block.
It is, note the deleted lines.
> >            switch (how) {
> >           case SIG_BLOCK:
> > -            if (has_sigsegv) {
> > -                ts->sigsegv_blocked = true;
> > -            }
> > +            sigorset(&ts->signal_mask, &ts->signal_mask, &val);
> >               break;
> >           case SIG_UNBLOCK:
> > -            if (has_sigsegv) {
> > -                ts->sigsegv_blocked = false;
> > +            /* There appears to be no signotset() */
> > +            for(i = 0; i != sizeof(val.__val) / sizeof(val.__val[0]); ++i)
> > {
> > +                ts->signal_mask.__val[i] &= ~val.__val[i];
> >               }
> > +            ts->signal_pending = 1; /* May be unblocking pending signal */
> >               break;
> >           case SIG_SETMASK:
> > -            ts->sigsegv_blocked = has_sigsegv;
> > +            ts->signal_mask = val;
> > +            ts->signal_pending = 1;
> >               break;
> >           default:
> >               g_assert_not_reached();
> >           }
> > +        sigdelset(temp, SIGSEGV);
> >       }
> >   -    ret = sigprocmask(how, temp, oldset);
> > -
> > -    if (oldset && segv_was_blocked) {
> > -        sigaddset(oldset, SIGSEGV);
> > -    }
> > -
> > +    ret = sigprocmask(how, temp, 0);
> So the effect of this patch is that we now have the guest's
> signal mask in two places:
>   1) the actual effective signal mask (except that SIGSEGV is never blocked)
>   2) in ts->signal_mask
>
> right?
Yes. I don't think this is correct in the case of execve().
> Rather than manually updating the ts->signal_mask for each
> of the set/unblock/block operations (which is pretty ugly for
> unblock), can we just use the third argument of the sigprocmask()
> call we're making anyway to get the previous effective signal
> mask into ts->signal_mask, and then fix up the SIGSEGV entry in it?
No, that would be the old value, we could make another call to 
sigprocmask(). We could abandon the above changes and just call 
sigprocmask() in process_pending_signals(), making this a 3 line change.