Re: [PATCH] x86/hvm: Fix rcu_unlock_domain call bypass

[Adrian Pop <apop@bitdefender.com>]

Hello,

On Tue, Nov 14, 2017 at 08:25:57AM -0700, Jan Beulich wrote:
> >>> On 14.11.17 at 16:11, <apop@bitdefender.com> wrote:
> > rcu_lock_current_domain is called at the beginning of do_altp2m_op, but
> > the altp2m_vcpu_enable_notify subop handler might skip calling
> > rcu_unlock_domain, possibly hanging the domain altogether.
> 
> I fully agree with the change, but the description needs improvement.
> For one, why would the domain be hanging with
> 
> static inline struct domain *rcu_lock_current_domain(void)
> {
>     return /*rcu_lock_domain*/(current->domain);
> }
> 
> ? And even if the lock function invocation wasn't commented
> out, all it does is preempt_disable(). That may cause an
> assertion to trigger in debug builds, but that's not a domain
> hang. Plus ...

Sorry, I was indeed referring to the preempt_count() assertion, only
using poor wording.  I had tested something else using
rcu_lock_domain_by_id() instead of rcu_lock_current_domain() which
triggered the assertion.

> 
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4534,12 +4534,18 @@ static int do_altp2m_op(
> >  
> >          if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
> >               a.u.enable_notify.vcpu_id != curr->vcpu_id )
> > +        {
> >              rc = -EINVAL;
> > +            break;
> > +        }
> 
> ... you also change flow here, which is a second bug you address,
> but you fail to mention it.

OK.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel