|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |