| Message ID | 20231211134827.7130-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Mini-OS: fix and cleanup of event handling | expand |
Juergen Gross, le lun. 11 déc. 2023 14:48:25 +0100, a ecrit: > When unmasking an event channel the associated event handler can be > called with interrupts enabled when not running as a PV guest. > > This can result in hard to debug races in case e.g. a handler is > registered for multiple events or when the handler is not using a lock > as it assumes to have interrupts disabled. > > Instead of using the PV interrupt disabling points just before calling > the handler, do the disabling once at the very start of > force_evtchn_callback(). > > Replace the evtchn_upcall_mask test in unmask_evtchn() with the more > appropriate irqs_disabled() test. > > As a precaution add a test to do_hypervisor_callback() that interrupts > are really disabled and crash in case this is not true. > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Thanks! > --- > hypervisor.c | 26 +++++++++----------------- > 1 file changed, 9 insertions(+), 17 deletions(-) > > diff --git a/hypervisor.c b/hypervisor.c > index f2cbbc1c..5309daa3 100644 > --- a/hypervisor.c > +++ b/hypervisor.c > @@ -102,6 +102,8 @@ void do_hypervisor_callback(struct pt_regs *regs) > shared_info_t *s = HYPERVISOR_shared_info; > vcpu_info_t *vcpu_info = &s->vcpu_info[cpu]; > > + BUG_ON(!irqs_disabled()); > + > in_callback = 1; > > vcpu_info->evtchn_upcall_pending = 0; > @@ -131,27 +133,19 @@ void do_hypervisor_callback(struct pt_regs *regs) > > void force_evtchn_callback(void) > { > -#ifdef XEN_HAVE_PV_UPCALL_MASK > - int save; > -#endif > vcpu_info_t *vcpu; > + unsigned long flags; > + > + local_irq_save(flags); > + > vcpu = &HYPERVISOR_shared_info->vcpu_info[smp_processor_id()]; > -#ifdef XEN_HAVE_PV_UPCALL_MASK > - save = vcpu->evtchn_upcall_mask; > -#endif > > while (vcpu->evtchn_upcall_pending) { > -#ifdef XEN_HAVE_PV_UPCALL_MASK > - vcpu->evtchn_upcall_mask = 1; > -#endif > - barrier(); > do_hypervisor_callback(NULL); > barrier(); > -#ifdef XEN_HAVE_PV_UPCALL_MASK > - vcpu->evtchn_upcall_mask = save; > - barrier(); > -#endif > }; > + > + local_irq_restore(flags); > } > > inline void mask_evtchn(uint32_t port) > @@ -177,9 +171,7 @@ inline void unmask_evtchn(uint32_t port) > &vcpu_info->evtchn_pending_sel) ) > { > vcpu_info->evtchn_upcall_pending = 1; > -#ifdef XEN_HAVE_PV_UPCALL_MASK > - if ( !vcpu_info->evtchn_upcall_mask ) > -#endif > + if ( !irqs_disabled() ) > force_evtchn_callback(); > } > } > -- > 2.35.3 >
diff --git a/hypervisor.c b/hypervisor.c index f2cbbc1c..5309daa3 100644 --- a/hypervisor.c +++ b/hypervisor.c @@ -102,6 +102,8 @@ void do_hypervisor_callback(struct pt_regs *regs) shared_info_t *s = HYPERVISOR_shared_info; vcpu_info_t *vcpu_info = &s->vcpu_info[cpu]; + BUG_ON(!irqs_disabled()); + in_callback = 1; vcpu_info->evtchn_upcall_pending = 0; @@ -131,27 +133,19 @@ void do_hypervisor_callback(struct pt_regs *regs) void force_evtchn_callback(void) { -#ifdef XEN_HAVE_PV_UPCALL_MASK - int save; -#endif vcpu_info_t *vcpu; + unsigned long flags; + + local_irq_save(flags); + vcpu = &HYPERVISOR_shared_info->vcpu_info[smp_processor_id()]; -#ifdef XEN_HAVE_PV_UPCALL_MASK - save = vcpu->evtchn_upcall_mask; -#endif while (vcpu->evtchn_upcall_pending) { -#ifdef XEN_HAVE_PV_UPCALL_MASK - vcpu->evtchn_upcall_mask = 1; -#endif - barrier(); do_hypervisor_callback(NULL); barrier(); -#ifdef XEN_HAVE_PV_UPCALL_MASK - vcpu->evtchn_upcall_mask = save; - barrier(); -#endif }; + + local_irq_restore(flags); } inline void mask_evtchn(uint32_t port) @@ -177,9 +171,7 @@ inline void unmask_evtchn(uint32_t port) &vcpu_info->evtchn_pending_sel) ) { vcpu_info->evtchn_upcall_pending = 1; -#ifdef XEN_HAVE_PV_UPCALL_MASK - if ( !vcpu_info->evtchn_upcall_mask ) -#endif + if ( !irqs_disabled() ) force_evtchn_callback(); } }
When unmasking an event channel the associated event handler can be called with interrupts enabled when not running as a PV guest. This can result in hard to debug races in case e.g. a handler is registered for multiple events or when the handler is not using a lock as it assumes to have interrupts disabled. Instead of using the PV interrupt disabling points just before calling the handler, do the disabling once at the very start of force_evtchn_callback(). Replace the evtchn_upcall_mask test in unmask_evtchn() with the more appropriate irqs_disabled() test. As a precaution add a test to do_hypervisor_callback() that interrupts are really disabled and crash in case this is not true. Signed-off-by: Juergen Gross <jgross@suse.com> --- hypervisor.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-)