| Message ID | 7666b5bba73a1410446789a0c4ea908376da3487.1590101479.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.14,1/2] x86/mem_sharing: Prohibit interrupt injection for forks | expand |
> 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 > @@ -256,6 +256,10 @@ void vmx_intr_assist(void) > if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event ) > return; > Just noticed after sending the patch that this block needs to be wrapped in #ifdef CONFIG_MEM_SHARING > + /* Block event injection for VM fork if requested */ > + if ( unlikely(v->domain->arch.hvm.mem_sharing.prohibit_interrupts) ) > + return; #endif > + > /* Crank the handle on interrupt state. */ > pt_vector = pt_update_irq(v); > I can resend if necessary but its also a trivial fixup when applying so let me know what would be preferred. I pushed the fixed-up version to http://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=shortlog;h=refs/heads/fork_interrupts. Thanks, Tamas
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@intel.com> > --- > 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.
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 @@ -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) ) 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; }; #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) uint16_t flags; /* IN: optional settings */ uint32_t pad; /* Must be set to 0 */ } fork;
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. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- 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(-)