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

[Keir Fraser <keir.xen@gmail.com>]

On 08/03/2011 15:56, "Paul Durrant" <Paul.Durrant@citrix.com> wrote:

> 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.

It will stand, in xen-unstable.

 K.

>   Paul
> 
>> -----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
>> 
>> 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
>>> 
>>> 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 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@citrix.com>
>>>> CC: Steven Smith <steven.smith@citrix.com>
>>>> 
>>>> 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@lists.xensource.com
>>>> http://lists.xensource.com/xen-devel
>>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel