[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [Xen-devel] [PATCH] Fix rcu domain locking for transitive grants



I still think this patch should stand. The locking around transitive grants is 
just borked and if someone actually does implement the rcu locks in future they 
will get a nasty surprise.

  Paul

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Keir Fraser
> Sent: 08 March 2011 15:39
> To: George Dunlap; xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH] Fix rcu domain locking for
> transitive grants
> 
> On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@xxxxxxxxxxxxx>
> wrote:
> 
> > This should be backported to the 4.1 branch; it causes a
> hypervisor
> > BUG() if guests are using netchannel2 transtiive grants to talk to
> > each other when debug mode is on.
> 
> I stubbed out the preemption checking stuff in 4.1 branch (it's not
> really needed since there are no users of waitqueues in 4.1), so
> this patch is not required. And that's fortunate, since it's quite
> non-trivial.
> 
>  -- Keir
> 
> >  -George
> >
> > On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
> > <george.dunlap@xxxxxxxxxxxxx> wrote:
> >> When acquiring a transitive grant for copy then the owning domain
> >> needs to be locked down as well as the granting domain. This was
> >> being done, but the unlocking was not. The acquire code now
> stores
> >> the struct domain * of the owning domain (rather than the domid)
> in
> >> the active entry in the granting domain. The release code then
> does the unlock on the owning domain.
> >> Note that I believe I also fixed a bug where, for non-transitive
> >> grants the active entry contained a reference to the acquiring
> domain
> >> rather than the granting domain. From my reading of the code this
> >> would stop the release code for transitive grants from
> terminating
> >> its recursion correctly.
> >>
> >> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >> CC: Steven Smith <steven.smith@xxxxxxxxxx>
> >>
> >> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
> >> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
> >> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
> >> @@ -1626,11 +1626,10 @@
> >>     struct active_grant_entry *act;
> >>     unsigned long r_frame;
> >>     uint16_t *status;
> >> -    domid_t trans_domid;
> >>     grant_ref_t trans_gref;
> >>     int released_read;
> >>     int released_write;
> >> -    struct domain *trans_dom;
> >> +    struct domain *td;
> >>
> >>     released_read = 0;
> >>     released_write = 0;
> >> @@ -1644,15 +1643,13 @@
> >>     if (rd->grant_table->gt_version == 1)
> >>     {
> >>         status = &sha->flags;
> >> -        trans_domid = rd->domain_id;
> >> -        /* Shut the compiler up.  This'll never be used, because
> >> -           trans_domid == rd->domain_id, but gcc doesn't know
> that.
> >> */
> >> -        trans_gref = 0x1234567;
> >> +        td = rd;
> >> +        trans_gref = gref;
> >>     }
> >>     else
> >>     {
> >>         status = &status_entry(rd->grant_table, gref);
> >> -        trans_domid = act->trans_dom;
> >> +        td = act->trans_domain;
> >>         trans_gref = act->trans_gref;
> >>     }
> >>
> >> @@ -1680,21 +1677,16 @@
> >>
> >>     spin_unlock(&rd->grant_table->lock);
> >>
> >> -    if ( trans_domid != rd->domain_id )
> >> +    if ( td != rd )
> >>     {
> >> -        if ( released_write || released_read )
> >> -        {
> >> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
> >> -            if ( trans_dom != NULL )
> >> -            {
> >> -                /* Recursive calls, but they're tail calls, so
> it's
> >> -                   okay. */
> >> -                if ( released_write )
> >> -                    __release_grant_for_copy(trans_dom,
> trans_gref,
> >> 0);
> >> -                else if ( released_read )
> >> -                    __release_grant_for_copy(trans_dom,
> trans_gref,
> >> 1);
> >> -            }
> >> -        }
> >> +        /* Recursive calls, but they're tail calls, so it's
> >> +           okay. */
> >> +        if ( released_write )
> >> +            __release_grant_for_copy(td, trans_gref, 0);
> >> +        else if ( released_read )
> >> +            __release_grant_for_copy(td, trans_gref, 1);
> >> +
> >> +       rcu_unlock_domain(td);
> >>     }
> >>  }
> >>
> >> @@ -1731,7 +1723,7 @@
> >>     uint32_t old_pin;
> >>     domid_t trans_domid;
> >>     grant_ref_t trans_gref;
> >> -    struct domain *rrd;
> >> +    struct domain *td;
> >>     unsigned long gfn;
> >>     unsigned long grant_frame;
> >>     unsigned trans_page_off;
> >> @@ -1785,8 +1777,8 @@
> >>                                status) ) != GNTST_okay )
> >>              goto unlock_out;
> >>
> >> -        trans_domid = ld->domain_id;
> >> -        trans_gref = 0;
> >> +        td = rd;
> >> +        trans_gref = gref;
> >>         if ( sha2 && (shah->flags & GTF_type_mask) ==
> GTF_transitive
> >> )
> >>         {
> >>             if ( !allow_transitive )
> >> @@ -1808,14 +1800,15 @@
> >>                that you don't need to go out of your way to avoid
> it
> >>                in the guest. */
> >>
> >> -            rrd = rcu_lock_domain_by_id(trans_domid);
> >> -            if ( rrd == NULL )
> >> +            /* We need to leave the rrd locked during the grant
> copy
> >> + */
> >> +            td = rcu_lock_domain_by_id(trans_domid);
> >> +            if ( td == NULL )
> >>                 PIN_FAIL(unlock_out, GNTST_general_error,
> >>                          "transitive grant referenced bad domain
> >> %d\n",
> >>                          trans_domid);
> >>             spin_unlock(&rd->grant_table->lock);
> >>
> >> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
> >> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
> >>                                           readonly, &grant_frame,
> >>                                           &trans_page_off,
> >> &trans_length,
> >>                                           0, &ignore); @@ -1823,6
> >> +1816,7 @@
> >>             spin_lock(&rd->grant_table->lock);
> >>             if ( rc != GNTST_okay ) {
> >>                 __fixup_status_for_pin(act, status);
> >> +               rcu_unlock_domain(td);
> >>                 spin_unlock(&rd->grant_table->lock);
> >>                 return rc;
> >>             }
> >> @@ -1834,6 +1828,7 @@
> >>             if ( act->pin != old_pin )
> >>             {
> >>                 __fixup_status_for_pin(act, status);
> >> +               rcu_unlock_domain(td);
> >>                 spin_unlock(&rd->grant_table->lock);
> >>                 return __acquire_grant_for_copy(rd, gref, ld,
> >> readonly,
> >>                                                 frame, page_off,
> >> length, @@ -1845,7 +1840,7 @@
> >>                sub-page, but we always treat it as one because
> that
> >>                blocks mappings of transitive grants. */
> >>             is_sub_page = 1;
> >> -            *owning_domain = rrd;
> >> +            *owning_domain = td;
> >>             act->gfn = -1ul;
> >>         }
> >>         else if ( sha1 )
> >> @@ -1891,7 +1886,7 @@
> >>             act->is_sub_page = is_sub_page;
> >>             act->start = trans_page_off;
> >>             act->length = trans_length;
> >> -            act->trans_dom = trans_domid;
> >> +            act->trans_domain = td;
> >>             act->trans_gref = trans_gref;
> >>             act->frame = grant_frame;
> >>         }
> >> diff -r f071d8e9f744 -r 14211e98efac
> xen/include/xen/grant_table.h
> >> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011
> >> +0000
> >> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011
> >> +++ +0000
> >> @@ -32,7 +32,7 @@
> >>  struct active_grant_entry {
> >>     u32           pin;    /* Reference count
> information.
> >> */
> >>     domid_t       domid;  /* Domain being granted
> access.
> >> */
> >> -    domid_t       trans_dom;
> >> +    struct domain *trans_domain;
> >>     uint32_t      trans_gref;
> >>     unsigned long frame;  /* Frame being
> granted.
> >> */
> >>     unsigned long gfn;    /* Guest's idea of the frame being
> granted.
> >> */
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@xxxxxxxxxxxxxxxxxxx
> >> http://lists.xensource.com/xen-devel
> >>
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-devel
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.