Re: [Xen-devel] [PATCH for-4.12 V5 1/2] altp2m: Prevent deadlocks when a domain performs altp2m operations on itself

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.19 at 16:41, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> domain_pause_except_self() was introduced to allow a domain to pause
> itself while doing altp2m operations.  However, as written, it has a
> risk fo deadlock if two vcpus enter the loop at the same time.
> 
> Luckily, there's already a solution for this: Attempt to call domain's
> hypercall_deadlock_mutex, and restart the entire hypercall if you
> fail.
> 
> Make domain_pause_except_self() attempt to grab this mutex when
> pausing itself, returning -ERESTART if it fails.  Have the callers
> check for errors and pass the value up.  In both cases, the top-level
> do_hvm_op() should DTRT when -ERESTART is returned.
> 
> The (necessary) reuse of the hypercall deadlock mutex poses the risk
> of getting called from a context where the lock was already acquired
> (e.g. someone may (say) call domctl_lock(), then afterwards call
> domain_pause_except_self()). However, in the interest of not
> overcomplicating things, no changes are made here to the mutex.
> Attempted nesting of this lock isn't a security issue, because all
> that will happen is that the vcpu will livelock taking continuations.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Tested-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> ---
> Release exception justification:
> - Fixes a bug in the current code
> - Lays the foundation of another fix
> - Only affects altp2m, which isn't a supported feature

Cc-ing Jürgen for this reason.

Jan

> NB two possible further improvements to put on the list for 4.13:
>  - Replace send-interrupt-and-wait-for-each-one loop with
>    hvmop_flush_tlb_all()'s more efficient
>    send-all-the-interrupts-then-wait loop.
>  - Modify hvmop_flush_tlb_all() to use domain_pause_except_self() to
>    avoid code duplication
> ---
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> ---
>  xen/arch/x86/mm/p2m.c   | 10 ++++++++--
>  xen/common/domain.c     |  8 +++++++-
>  xen/include/xen/sched.h |  2 +-
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index d14ce57..7232dbf 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2530,8 +2530,11 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
> unsigned int idx)
>      if ( !idx || idx >= MAX_ALTP2M )
>          return rc;
>  
> -    domain_pause_except_self(d);
> +    rc = domain_pause_except_self(d);
> +    if ( rc )
> +        return rc;
>  
> +    rc = -EBUSY;
>      altp2m_list_lock(d);
>  
>      if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
> @@ -2561,8 +2564,11 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, 
> unsigned int idx)
>      if ( idx >= MAX_ALTP2M )
>          return rc;
>  
> -    domain_pause_except_self(d);
> +    rc = domain_pause_except_self(d);
> +    if ( rc )
> +        return rc;
>  
> +    rc = -EINVAL;
>      altp2m_list_lock(d);
>  
>      if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 7470cd9..32bca8d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1134,18 +1134,24 @@ int domain_unpause_by_systemcontroller(struct domain 
> *d)
>      return 0;
>  }
>  
> -void domain_pause_except_self(struct domain *d)
> +int domain_pause_except_self(struct domain *d)
>  {
>      struct vcpu *v, *curr = current;
>  
>      if ( curr->domain == d )
>      {
> +        /* Avoid racing with other vcpus which may want to be pausing us */
> +        if ( !spin_trylock(&d->hypercall_deadlock_mutex) )
> +            return -ERESTART;
>          for_each_vcpu( d, v )
>              if ( likely(v != curr) )
>                  vcpu_pause(v);
> +        spin_unlock(&d->hypercall_deadlock_mutex);
>      }
>      else
>          domain_pause(d);
> +
> +    return 0;
>  }
>  
>  void domain_unpause_except_self(struct domain *d)
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index d633e1d..edee52d 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -839,7 +839,7 @@ static inline int 
> domain_pause_by_systemcontroller_nosync(struct domain *d)
>  }
>  
>  /* domain_pause() but safe against trying to pause current. */
> -void domain_pause_except_self(struct domain *d);
> +int __must_check domain_pause_except_self(struct domain *d);
>  void domain_unpause_except_self(struct domain *d);
>  
>  /*
> -- 
> 2.7.4




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel