WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: Re: [Xen-devel] [PATCH] Fix rcu domain locking for transitive grants
From: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
Date: Tue, 8 Mar 2011 15:11:08 +0000
Cc: george.dunlap@xxxxxxxxxxxxx
Delivery-date: Tue, 08 Mar 2011 07:11:55 -0800
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=O61WTRAIAgxubbRTRkcGDES4mh569dqT+23XKfxUzuI=; b=XUHAv0GdVJukkfnaQAWhEiBE/oaDOJ068qtpk8EWHG0EvXNQIwFhFoRe/G5o/Te2Oq 96OI9qpZ9EzKVpoJyZUHBQHL9UqUBBCzfGVgjHaOm0hK/KPZ7vQ8xrE+H9qhOxirbuA+ 30jyWTs+Xu6gUNqtJ5nEvoD+liAbXdfIUlsl8=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=xeu5L2vMidE7dCtv05aKOZNlv1tRphM3f4n0ebFOE1vgcf3KVdTM5OR3NCm4g8S2SJ YPng8lyHALUop+CxtgjtZutXb1yPbqFPM8aeVwGHyOLtPhe+mpEUF2kAMfaqI5AY1ZK8 E9Soc6LOaVdXW0LxHra4KVXekQcw9zBUv/99E=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <14211e98efac33d4eb0a.1299596541@elijah>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <14211e98efac33d4eb0a.1299596541@elijah>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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.

 -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