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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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