2017-11-25 5:39 GMT+08:00 Jan H. Schönherr <jschoenh@xxxxxxxxx>:
> KVM API says for the signal mask you set via KVM_SET_SIGNAL_MASK, that
> "any unblocked signal received [...] will cause KVM_RUN to return with
> -EINTR" and that "the signal will only be delivered if not blocked by
> the original signal mask".
>
> This, however, is only true, when the calling task has a signal handler
> registered for a signal. If not, signal evaluation is short-circuited for
> SIG_IGN and SIG_DFL, and the signal is either ignored without KVM_RUN
> returning or the whole process is terminated.
>
> Make KVM_SET_SIGNAL_MASK behave as advertised by utilizing logic similar
> to that in do_sigtimedwait() to avoid short-circuiting of signals.
Not fully understand, I think the only distinction which this patch
introduces is to use current->real_blocked instead of a local variable
to save the mask of old blocked signals. How it fixes the issue which
you mentioned?
Regards,
Wanpeng Li
>
> Signed-off-by: Jan H. Schönherr <jschoenh@xxxxxxxxx>
> ---
> Note: Only tested on x86.
> ---
> arch/mips/kvm/mips.c | 7 ++-----
> arch/powerpc/kvm/powerpc.c | 7 ++-----
> arch/s390/kvm/kvm-s390.c | 7 ++-----
> arch/x86/kvm/x86.c | 7 ++-----
> include/linux/kvm_host.h | 3 +++
> virt/kvm/arm/arm.c | 8 +++-----
> virt/kvm/kvm_main.c | 23 +++++++++++++++++++++++
> 7 files changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index d535edc..75fdeaa 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -445,10 +445,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> int r = -EINTR;
> - sigset_t sigsaved;
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
> + kvm_sigset_activate(vcpu);
>
> if (vcpu->mmio_needed) {
> if (!vcpu->mmio_is_write)
> @@ -480,8 +478,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> local_irq_enable();
>
> out:
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> + kvm_sigset_deactivate(vcpu);
>
> return r;
> }
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 6b6c53c..1915e86 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1407,7 +1407,6 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
> int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> int r;
> - sigset_t sigsaved;
>
> if (vcpu->mmio_needed) {
> vcpu->mmio_needed = 0;
> @@ -1448,16 +1447,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> #endif
> }
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
> + kvm_sigset_activate(vcpu);
>
> if (run->immediate_exit)
> r = -EINTR;
> else
> r = kvmppc_vcpu_run(run, vcpu);
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> + kvm_sigset_deactivate(vcpu);
>
> return r;
> }
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 98ad8b9..9614aea 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3372,7 +3372,6 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> {
> int rc;
> - sigset_t sigsaved;
>
> if (kvm_run->immediate_exit)
> return -EINTR;
> @@ -3382,8 +3381,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> return 0;
> }
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
> + kvm_sigset_activate(vcpu);
>
> if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm)) {
> kvm_s390_vcpu_start(vcpu);
> @@ -3417,8 +3415,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> disable_cpu_timer_accounting(vcpu);
> store_regs(vcpu, kvm_run);
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> + kvm_sigset_deactivate(vcpu);
>
> vcpu->stat.exit_userspace++;
> return rc;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 985a305..990df39 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7270,12 +7270,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> {
> struct fpu *fpu = ¤t->thread.fpu;
> int r;
> - sigset_t sigsaved;
>
> fpu__initialize(fpu);
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
> + kvm_sigset_activate(vcpu);
>
> if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
> if (kvm_run->immediate_exit) {
> @@ -7318,8 +7316,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>
> out:
> post_kvm_run_save(vcpu);
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> + kvm_sigset_deactivate(vcpu);
>
> return r;
> }
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2e754b7..893d6d6 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -715,6 +715,9 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
> unsigned long len);
> void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn);
>
> +void kvm_sigset_activate(struct kvm_vcpu *vcpu);
> +void kvm_sigset_deactivate(struct kvm_vcpu *vcpu);
> +
> void kvm_vcpu_block(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index a6524ff..a67c106 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -615,7 +615,6 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> int ret;
> - sigset_t sigsaved;
>
> if (unlikely(!kvm_vcpu_initialized(vcpu)))
> return -ENOEXEC;
> @@ -633,8 +632,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> if (run->immediate_exit)
> return -EINTR;
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
> + kvm_sigset_activate(vcpu);
>
> ret = 1;
> run->exit_reason = KVM_EXIT_UNKNOWN;
> @@ -769,8 +767,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> kvm_pmu_update_run(vcpu);
> }
>
> - if (vcpu->sigset_active)
> - sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> + kvm_sigset_deactivate(vcpu);
> +
> return ret;
> }
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2dd1a9c..c01cff0 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2065,6 +2065,29 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn)
> }
> EXPORT_SYMBOL_GPL(kvm_vcpu_mark_page_dirty);
>
> +void kvm_sigset_activate(struct kvm_vcpu *vcpu)
> +{
> + if (!vcpu->sigset_active)
> + return;
> +
> + /*
> + * This does a lockless modification of ->real_blocked, which is fine
> + * because, only current can change ->real_blocked and all readers of
> + * ->real_blocked don't care as long ->real_blocked is always a subset
> + * of ->blocked.
> + */
> + sigprocmask(SIG_SETMASK, &vcpu->sigset, ¤t->real_blocked);
> +}
> +
> +void kvm_sigset_deactivate(struct kvm_vcpu *vcpu)
> +{
> + if (!vcpu->sigset_active)
> + return;
> +
> + sigprocmask(SIG_SETMASK, ¤t->real_blocked, NULL);
> + sigemptyset(¤t->real_blocked);
> +}
> +
> static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
> {
> unsigned int old, val, grow;
> --
> 2.3.1.dirty
>
