Xen devel - Sep 2012 - [PATCH 2/2] x86: fix RCU locking in PHYSDEVOP_get_free_pirq

Apart from properly pairing locks with unlocks, also reduce the lock
scope - no need to do the copy_{from,to}_guest()-s inside the protected
region.

I actually wonder whether the RCU locks are needed here at all.

Reported-by: Tim Deegan <tim@xen.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -698,13 +698,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
         struct physdev_get_free_pirq out;
         struct domain *d;
 
-        d = rcu_lock_current_domain();
-        
         ret = -EFAULT;
         if ( copy_from_guest(&out, arg, 1) != 0 )
             break;
 
+        d = rcu_lock_current_domain();
         spin_lock(&d->event_lock);
+
         ret = get_free_pirq(d, out.type);
         if ( ret >= 0 )
         {
@@ -715,7 +715,9 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
             else
                 ret = -ENOMEM;
         }
+
         spin_unlock(&d->event_lock);
+        rcu_unlock_domain(d);
 
         if ( ret >= 0 )
         {
@@ -723,7 +725,6 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
             ret = copy_to_guest(arg, &out, 1) ? -EFAULT : 0;
         }
 
-        rcu_unlock_domain(d);
         break;
     }
     default:





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 05/09/2012 13:25, "Jan Beulich" <JBeulich@suse.com> wrote:
> Apart from properly pairing locks with unlocks, also reduce the lock
> scope - no need to do the copy_{from,to}_guest()-s inside the protected
> region.
> 
> I actually wonder whether the RCU locks are needed here at all.
If it''s a path that only acts on current domain, then no.

 -- Keir
> Reported-by: Tim Deegan <tim@xen.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -698,13 +698,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>          struct physdev_get_free_pirq out;
>          struct domain *d;
>  
> -        d = rcu_lock_current_domain();
> -        
>          ret = -EFAULT;
>          if ( copy_from_guest(&out, arg, 1) != 0 )
>              break;
>  
> +        d = rcu_lock_current_domain();
>          spin_lock(&d->event_lock);
> +
>          ret = get_free_pirq(d, out.type);
>          if ( ret >= 0 )
>          {
> @@ -715,7 +715,9 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>              else
>                  ret = -ENOMEM;
>          }
> +
>          spin_unlock(&d->event_lock);
> +        rcu_unlock_domain(d);
>  
>          if ( ret >= 0 )
>          {
> @@ -723,7 +725,6 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>              ret = copy_to_guest(arg, &out, 1) ? -EFAULT : 0;
>          }
>  
> -        rcu_unlock_domain(d);
>          break;
>      }
>      default:
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 05.09.12 at 14:33, Keir Fraser <keir.xen@gmail.com> wrote:
> On 05/09/2012 13:25, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> Apart from properly pairing locks with unlocks, also reduce the lock
>> scope - no need to do the copy_{from,to}_guest()-s inside the protected
>> region.
>> 
>> I actually wonder whether the RCU locks are needed here at all.
> 
> If it''s a path that only acts on current domain, then no.
So for what case does rcu_lock_current_domain() then exist at all?

Jan

On 05/09/2012 13:45, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> Apart from properly pairing locks with unlocks, also reduce the
lock
>>> scope - no need to do the copy_{from,to}_guest()-s inside the
protected
>>> region.
>>> 
>>> I actually wonder whether the RCU locks are needed here at all.
>> 
>> If it''s a path that only acts on current domain, then no.
> 
> So for what case does rcu_lock_current_domain() then exist at all?
To match an unconditional rcu_unlock_domain() on exit paths, for operations
which can either be on current->domain or on a rcu_lock_domain_by_id().

 -- Keir