Re: [PATCH 1/3] Mini-OS: call event handlers always with interrupts off

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

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@xxxxxxxx>

Reviewed-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>

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
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.