Re: [PATCH 16/17] KVM: PPC: MPIC: Add support for KVM_IRQ_LINE

Scott Wood
2013-04-19T18:51:05Z

On 04/19/2013 09:06:27 AM, Alexander Graf wrote:
> Now that all pieces are in place for reusing generic irq
> infrastructure,
> we can copy x86's implementation of KVM_IRQ_LINE irq injection and
> simply
> reuse it for PPC, as it will work there just as well.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> arch/powerpc/include/uapi/asm/kvm.h | 1 +
> arch/powerpc/kvm/powerpc.c | 13 +++++++++++++
> 2 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
> b/arch/powerpc/include/uapi/asm/kvm.h
> index 3537bf3..dbb2ac2 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -26,6 +26,7 @@
> #define __KVM_HAVE_SPAPR_TCE
> #define __KVM_HAVE_PPC_SMT
> #define __KVM_HAVE_IRQCHIP
> +#define __KVM_HAVE_IRQ_LINE
>
> struct kvm_regs {
> __u64 pc;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index c431fea..874c106 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -33,6 +33,7 @@
> #include <asm/cputhreads.h>
> #include <asm/irqflags.h>
> #include "timing.h"
> +#include "irq.h"
> #include "../mm/mmu_decl.h"
>
> #define CREATE_TRACE_POINTS
> @@ -945,6 +946,18 @@ static int kvm_vm_ioctl_get_pvinfo(struct
> kvm_ppc_pvinfo *pvinfo)
> return 0;
> }
>
> +int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level
> *irq_event,
> + bool line_status)
> +{
> + if (!irqchip_in_kernel(kvm))
> + return -ENXIO;
> +
> + irq_event->status = kvm_set_irq(kvm,
> KVM_USERSPACE_IRQ_SOURCE_ID,
> + irq_event->irq,
> irq_event->level,
> + line_status);
> + return 0;
> +}

As Paul noted in the XICS patchset, this could reference an MPIC that
has gone away if the user never attached any vcpus and then closed the
MPIC fd. It's not a reasonable use case, but it could be used
malicously to get the kernel to access a bad pointer. The
irqchip_in_kernel check helps somewhat, but it's meant for ensuring
that the creation has happened -- it's racy if used for ensuring that
destruction hasn't happened.

The problem is rooted in the awkwardness of performing an operation
that logically should be on the MPIC fd, but is instead being done on
the vm fd.

I think these three steps would fix it (the first two seem like things
we should be doing anyway):
- During MPIC destruction, make sure MPIC deregisters all routes that
reference it.
- In kvm_set_irq(), do not release the RCU read lock until after the
set() function has been called.
- Do not hook up kvm_send_userspace_msi() to MPIC or other new
irqchips, as that bypasses the RCU lock. It could be supported as a
device fd ioctl if desired, or it could be reworked to operate on an
RCU-managed list of MSI handlers, though MPIC really doesn't need this
at all.

-Scott