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

[Scott Wood <scottwood@freescale.com>]

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