From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH] Fix rcu domain locking for transitive grants Date: Tue, 08 Mar 2011 16:01:41 +0000 Message-ID: References: <291EDFCB1E9E224A99088639C47620228E936E1989@LONPMAILBOX01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <291EDFCB1E9E224A99088639C47620228E936E1989@LONPMAILBOX01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Paul Durrant , George Dunlap , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 08/03/2011 15:56, "Paul Durrant" wrote: > I still think this patch should stand. The locking around transitive gran= ts is > just borked and if someone actually does implement the rcu locks in futur= e > they will get a nasty surprise. It will stand, in xen-unstable. K. > Paul >=20 >> -----Original Message----- >> From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel- >> bounces@lists.xensource.com] On Behalf Of Keir Fraser >> Sent: 08 March 2011 15:39 >> To: George Dunlap; xen-devel@lists.xensource.com >> Subject: Re: [Xen-devel] [PATCH] Fix rcu domain locking for >> transitive grants >>=20 >> On 08/03/2011 15:11, "George Dunlap" >> wrote: >>=20 >>> 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. >>=20 >> 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. >>=20 >> -- Keir >>=20 >>> -George >>>=20 >>> On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap >>> 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. >>>>=20 >>>> Signed-off-by: Paul Durrant >>>> CC: Steven Smith >>>>=20 >>>> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c >>>> --- a/xen/common/grant_table.c =A0Tue Mar 08 10:23:52 2011 +0000 >>>> +++ b/xen/common/grant_table.c =A0Tue Mar 08 14:39:03 2011 +0000 >>>> @@ -1626,11 +1626,10 @@ >>>> =A0 =A0 struct active_grant_entry *act; >>>> =A0 =A0 unsigned long r_frame; >>>> =A0 =A0 uint16_t *status; >>>> - =A0 =A0domid_t trans_domid; >>>> =A0 =A0 grant_ref_t trans_gref; >>>> =A0 =A0 int released_read; >>>> =A0 =A0 int released_write; >>>> - =A0 =A0struct domain *trans_dom; >>>> + =A0 =A0struct domain *td; >>>>=20 >>>> =A0 =A0 released_read =3D 0; >>>> =A0 =A0 released_write =3D 0; >>>> @@ -1644,15 +1643,13 @@ >>>> =A0 =A0 if (rd->grant_table->gt_version =3D=3D 1) >>>> =A0 =A0 { >>>> =A0 =A0 =A0 =A0 status =3D &sha->flags; >>>> - =A0 =A0 =A0 =A0trans_domid =3D rd->domain_id; >>>> - =A0 =A0 =A0 =A0/* Shut the compiler up. =A0This'll never be used, because >>>> - =A0 =A0 =A0 =A0 =A0 trans_domid =3D=3D rd->domain_id, but gcc doesn't know >> that. >>>> */ >>>> - =A0 =A0 =A0 =A0trans_gref =3D 0x1234567; >>>> + =A0 =A0 =A0 =A0td =3D rd; >>>> + =A0 =A0 =A0 =A0trans_gref =3D gref; >>>> =A0 =A0 } >>>> =A0 =A0 else >>>> =A0 =A0 { >>>> =A0 =A0 =A0 =A0 status =3D &status_entry(rd->grant_table, gref); >>>> - =A0 =A0 =A0 =A0trans_domid =3D act->trans_dom; >>>> + =A0 =A0 =A0 =A0td =3D act->trans_domain; >>>> =A0 =A0 =A0 =A0 trans_gref =3D act->trans_gref; >>>> =A0 =A0 } >>>>=20 >>>> @@ -1680,21 +1677,16 @@ >>>>=20 >>>> =A0 =A0 spin_unlock(&rd->grant_table->lock); >>>>=20 >>>> - =A0 =A0if ( trans_domid !=3D rd->domain_id ) >>>> + =A0 =A0if ( td !=3D rd ) >>>> =A0 =A0 { >>>> - =A0 =A0 =A0 =A0if ( released_write || released_read ) >>>> - =A0 =A0 =A0 =A0{ >>>> - =A0 =A0 =A0 =A0 =A0 =A0trans_dom =3D rcu_lock_domain_by_id(trans_domid); >>>> - =A0 =A0 =A0 =A0 =A0 =A0if ( trans_dom !=3D NULL ) >>>> - =A0 =A0 =A0 =A0 =A0 =A0{ >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Recursive calls, but they're tail calls, so >> it's >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 okay. */ >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if ( released_write ) >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0__release_grant_for_copy(trans_dom, >> trans_gref, >>>> 0); >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else if ( released_read ) >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0__release_grant_for_copy(trans_dom, >> trans_gref, >>>> 1); >>>> - =A0 =A0 =A0 =A0 =A0 =A0} >>>> - =A0 =A0 =A0 =A0} >>>> + =A0 =A0 =A0 =A0/* Recursive calls, but they're tail calls, so it's >>>> + =A0 =A0 =A0 =A0 =A0 okay. */ >>>> + =A0 =A0 =A0 =A0if ( released_write ) >>>> + =A0 =A0 =A0 =A0 =A0 =A0__release_grant_for_copy(td, trans_gref, 0); >>>> + =A0 =A0 =A0 =A0else if ( released_read ) >>>> + =A0 =A0 =A0 =A0 =A0 =A0__release_grant_for_copy(td, trans_gref, 1); >>>> + >>>> + =A0 =A0 =A0 rcu_unlock_domain(td); >>>> =A0 =A0 } >>>> =A0} >>>>=20 >>>> @@ -1731,7 +1723,7 @@ >>>> =A0 =A0 uint32_t old_pin; >>>> =A0 =A0 domid_t trans_domid; >>>> =A0 =A0 grant_ref_t trans_gref; >>>> - =A0 =A0struct domain *rrd; >>>> + =A0 =A0struct domain *td; >>>> =A0 =A0 unsigned long gfn; >>>> =A0 =A0 unsigned long grant_frame; >>>> =A0 =A0 unsigned trans_page_off; >>>> @@ -1785,8 +1777,8 @@ >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0status) ) !=3D GNTST_okay ) >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0goto unlock_out; >>>>=20 >>>> - =A0 =A0 =A0 =A0trans_domid =3D ld->domain_id; >>>> - =A0 =A0 =A0 =A0trans_gref =3D 0; >>>> + =A0 =A0 =A0 =A0td =3D rd; >>>> + =A0 =A0 =A0 =A0trans_gref =3D gref; >>>> =A0 =A0 =A0 =A0 if ( sha2 && (shah->flags & GTF_type_mask) =3D=3D >> GTF_transitive >>>> ) >>>> =A0 =A0 =A0 =A0 { >>>> =A0 =A0 =A0 =A0 =A0 =A0 if ( !allow_transitive ) >>>> @@ -1808,14 +1800,15 @@ >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0that you don't need to go out of your way to avoid >> it >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0in the guest. */ >>>>=20 >>>> - =A0 =A0 =A0 =A0 =A0 =A0rrd =3D rcu_lock_domain_by_id(trans_domid); >>>> - =A0 =A0 =A0 =A0 =A0 =A0if ( rrd =3D=3D NULL ) >>>> + =A0 =A0 =A0 =A0 =A0 =A0/* We need to leave the rrd locked during the grant >> copy >>>> + */ >>>> + =A0 =A0 =A0 =A0 =A0 =A0td =3D rcu_lock_domain_by_id(trans_domid); >>>> + =A0 =A0 =A0 =A0 =A0 =A0if ( td =3D=3D NULL ) >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PIN_FAIL(unlock_out, GNTST_general_error, >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"transitive grant referenced bad domain >>>> %d\n", >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0trans_domid); >>>> =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock(&rd->grant_table->lock); >>>>=20 >>>> - =A0 =A0 =A0 =A0 =A0 =A0rc =3D __acquire_grant_for_copy(rrd, trans_gref, rd, >>>> + =A0 =A0 =A0 =A0 =A0 =A0rc =3D __acquire_grant_for_copy(td, trans_gref, rd, >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 readonly, &grant_frame, >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &trans_page_off, >>>> &trans_length, >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 0, &ignore); @@ -1823,6 >>>> +1816,7 @@ >>>> =A0 =A0 =A0 =A0 =A0 =A0 spin_lock(&rd->grant_table->lock); >>>> =A0 =A0 =A0 =A0 =A0 =A0 if ( rc !=3D GNTST_okay ) { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 __fixup_status_for_pin(act, status); >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rcu_unlock_domain(td); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock(&rd->grant_table->lock); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc; >>>> =A0 =A0 =A0 =A0 =A0 =A0 } >>>> @@ -1834,6 +1828,7 @@ >>>> =A0 =A0 =A0 =A0 =A0 =A0 if ( act->pin !=3D old_pin ) >>>> =A0 =A0 =A0 =A0 =A0 =A0 { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 __fixup_status_for_pin(act, status); >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rcu_unlock_domain(td); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock(&rd->grant_table->lock); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return __acquire_grant_for_copy(rd, gref, ld, >>>> readonly, >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 frame, page_off, >>>> length, @@ -1845,7 +1840,7 @@ >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sub-page, but we always treat it as one because >> that >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0blocks mappings of transitive grants. */ >>>> =A0 =A0 =A0 =A0 =A0 =A0 is_sub_page =3D 1; >>>> - =A0 =A0 =A0 =A0 =A0 =A0*owning_domain =3D rrd; >>>> + =A0 =A0 =A0 =A0 =A0 =A0*owning_domain =3D td; >>>> =A0 =A0 =A0 =A0 =A0 =A0 act->gfn =3D -1ul; >>>> =A0 =A0 =A0 =A0 } >>>> =A0 =A0 =A0 =A0 else if ( sha1 ) >>>> @@ -1891,7 +1886,7 @@ >>>> =A0 =A0 =A0 =A0 =A0 =A0 act->is_sub_page =3D is_sub_page; >>>> =A0 =A0 =A0 =A0 =A0 =A0 act->start =3D trans_page_off; >>>> =A0 =A0 =A0 =A0 =A0 =A0 act->length =3D trans_length; >>>> - =A0 =A0 =A0 =A0 =A0 =A0act->trans_dom =3D trans_domid; >>>> + =A0 =A0 =A0 =A0 =A0 =A0act->trans_domain =3D td; >>>> =A0 =A0 =A0 =A0 =A0 =A0 act->trans_gref =3D trans_gref; >>>> =A0 =A0 =A0 =A0 =A0 =A0 act->frame =3D grant_frame; >>>> =A0 =A0 =A0 =A0 } >>>> diff -r f071d8e9f744 -r 14211e98efac >> xen/include/xen/grant_table.h >>>> --- a/xen/include/xen/grant_table.h =A0 =A0 Tue Mar 08 10:23:52 2011 >>>> +0000 >>>> +++ b/xen/include/xen/grant_table.h =A0 =A0 Tue Mar 08 14:39:03 2011 >>>> +++ +0000 >>>> @@ -32,7 +32,7 @@ >>>> =A0struct active_grant_entry { >>>> =A0 =A0 u32 =A0 =A0 =A0 =A0 =A0 pin; =A0 =A0/* Reference count >> information. >>>> */ >>>> =A0 =A0 domid_t =A0 =A0 =A0 domid; =A0/* Domain being granted >> access. >>>> */ >>>> - =A0 =A0domid_t =A0 =A0 =A0 trans_dom; >>>> + =A0 =A0struct domain *trans_domain; >>>> =A0 =A0 uint32_t =A0 =A0 =A0trans_gref; >>>> =A0 =A0 unsigned long frame; =A0/* Frame being >> granted. >>>> */ >>>> =A0 =A0 unsigned long gfn; =A0 =A0/* Guest's idea of the frame being >> granted. >>>> */ >>>>=20 >>>> _______________________________________________ >>>> Xen-devel mailing list >>>> Xen-devel@lists.xensource.com >>>> http://lists.xensource.com/xen-devel >>>>=20 >>>=20 >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xensource.com >>> http://lists.xensource.com/xen-devel >>=20 >>=20 >>=20 >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel