Re: [PATCH for-4.14 1/2] x86/mem_sharing: Prohibit interrupt injection for forks

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 21, 2020 at 03:53:22PM -0700, Tamas K Lengyel wrote:
> When running shallow forks without device models it may be undesirable for Xen
> to inject interrupts. With Windows forks we have observed the kernel going 
> into
> infinite loops when trying to process such interrupts. By disabling interrupt
> injection the fuzzer can exercise the target code without interference.

Could you add some more information about why windows goes into
infinite loops? Is it trying to access MMIO regions of emulated
devices and getting ~0 out of them instead of the expected data and
that causes it to loop indefinitely?

> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/vmx/intr.c      | 4 ++++
>  xen/arch/x86/mm/mem_sharing.c    | 6 +++++-
>  xen/include/asm-x86/hvm/domain.h | 2 ++
>  xen/include/public/memory.h      | 1 +
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 000e14af49..3814795e3f 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c

I think you are missing the AMD side of this change? A similar
adjustment should be done to svm_intr_assist, or else it should be
noted in the commit message the reason this is Intel only.

> @@ -256,6 +256,10 @@ void vmx_intr_assist(void)
>      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
>          return;
>  
> +    /* Block event injection for VM fork if requested */
> +    if ( unlikely(v->domain->arch.hvm.mem_sharing.prohibit_interrupts) )
> +        return;
> +
>      /* Crank the handle on interrupt state. */
>      pt_vector = pt_update_irq(v);
>  
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 7271e5c90b..7352fce866 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -2106,7 +2106,8 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>          rc = -EINVAL;
>          if ( mso.u.fork.pad )
>              goto out;
> -        if ( mso.u.fork.flags & ~XENMEM_FORK_WITH_IOMMU_ALLOWED )
> +        if ( mso.u.fork.flags & ~(XENMEM_FORK_WITH_IOMMU_ALLOWED |
> +                                  XENMEM_FORK_PROHIBIT_INTERRUPTS) )


Nit: I would move the XENMEM_FORK_ option ORing to a newline, so that
you don't have to use a whole line every time a new option is used.
Ie:

        if ( mso.u.fork.flags &
             ~(XENMEM_FORK_WITH_IOMMU_ALLOWED | XENMEM_FORK_BLOCK_INTERRUPTS) )


>              goto out;
>  
>          rc = rcu_lock_live_remote_domain_by_id(mso.u.fork.parent_domain,
> @@ -2134,6 +2135,9 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>              rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
>                                                 "lh", XENMEM_sharing_op,
>                                                 arg);
> +        else if ( !rc && (mso.u.fork.flags & 
> XENMEM_FORK_PROHIBIT_INTERRUPTS) )
> +            d->arch.hvm.mem_sharing.prohibit_interrupts = true;
> +
>          rcu_unlock_domain(pd);
>          break;
>      }
> diff --git a/xen/include/asm-x86/hvm/domain.h 
> b/xen/include/asm-x86/hvm/domain.h
> index 95fe18cddc..e114f818d3 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -74,6 +74,8 @@ struct mem_sharing_domain
>       * to resume the search.
>       */
>      unsigned long next_shared_gfn_to_relinquish;
> +
> +    bool prohibit_interrupts;

Nit: I would prefer block_interrupts, prohibit seems very formal to
me, but I'm not a native speaker, so feel free to ignore this
suggestion.

>  };
>  #endif
>  
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index dbd35305df..fe2e6caa68 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -537,6 +537,7 @@ struct xen_mem_sharing_op {
>          struct mem_sharing_op_fork {      /* OP_FORK */
>              domid_t parent_domain;        /* IN: parent's domain id */
>  #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> +#define XENMEM_FORK_PROHIBIT_INTERRUPTS (1u << 1)

FWIW, I would also use BLOCK here instead of PROHIBIT.

Thanks, Roger.