From mboxrd@z Thu Jan  1 00:00:00 1970
From: Keir Fraser <keir.xen@gmail.com>
Subject: Re: [PATCH] Fix rcu domain locking for transitive grants
Date: Tue, 08 Mar 2011 15:38:39 +0000
Message-ID: <C99BFDFF.14508%keir.xen@gmail.com>
References: <AANLkTiky-mPiQ5KLe7AGCb0trFDGViw7WksRyukUXeAD@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain;
	charset="ISO-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <AANLkTiky-mPiQ5KLe7AGCb0trFDGViw7WksRyukUXeAD@mail.gmail.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: George Dunlap <George.Dunlap@eu.citrix.com>, xen-devel@lists.xensource.com
List-Id: xen-devel@lists.xenproject.org

On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com> 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
>=20
> On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
> <george.dunlap@eu.citrix.com> 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 th=
e
>> owning domain (rather than the domid) in the active entry in the grantin=
g
>> 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 th=
e
>> release code for transitive grants from terminating its recursion
>> correctly.
>>=20
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> CC: Steven Smith <steven.smith@citrix.com>
>>=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 =A0 =A0 =A0 =A0 */
>> =A0 =A0 domid_t =A0 =A0 =A0 domid; =A0/* Domain being granted access. =A0 =A0 =A0 =A0 =A0 =A0 */
>> - =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 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
>> =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