Re: [PATCH 2/2] x86: fix RCU locking in PHYSDEVOP_get_free_pirq

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

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