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

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

On 08/03/2011 16:08, "George Dunlap" <george.dunlap@citrix.com> wrote:

> As I understood him, Keir was saying he wasn't going to apply it 4.1 --
> at least not before the official release.  (Perhaps in 4.1.1?)
> 
> I don't really understand the logic of having locks which act as noops
> at all -- I think no matter what, if someone decides to implement RCU,
> they're in for a boatload of nasty surprises.  It seems to me like the
> current locks will be little more than a hint to whatever future dev
> does that work that this is an area that they might want to look at.
> But it seems to me like they'll end up having to write the locks and
> locking disciplines from scratch anyway.

There is no doubt that it has been a lot easier to fix up a few bugs in the
existing no-op RCU read-lock usage than retrofit RCU read locks from
scratch, for the waitqueue work.

 -- Keir

>  -George
> 
> On Tue, 2011-03-08 at 15:56 +0000, Paul Durrant 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.
>> 
>>   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
> 
>