diff mbox series

[RFC,v2,8/8] evtchn: don't call Xen consumer callback with per-channel lock held

Message ID 247f0d77-9447-47d0-4fa6-8e17b3e6a6de@suse.com (mailing list archive)
State New, archived
Headers show
Series evtchn: recent XSAs follow-on | expand

Commit Message

Jan Beulich Oct. 20, 2020, 2:13 p.m. UTC
While there don't look to be any problems with this right now, the lock
order implications from holding the lock can be very difficult to follow
(and may be easy to violate unknowingly). The present callbacks don't
(and no such callback should) have any need for the lock to be held.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TODO: vm_event_disable() frees the structures used by respective
      callbacks - need to either use call_rcu() for freeing, or maintain
      a count of in-progress calls, for evtchn_close() to wait to drop
      to zero before dropping the lock / returning.

Comments

Isaila Alexandru Nov. 3, 2020, 10:17 a.m. UTC | #1
Hi Jan and sorry for the late reply,

On 20.10.2020 17:13, Jan Beulich wrote:
> While there don't look to be any problems with this right now, the lock
> order implications from holding the lock can be very difficult to follow
> (and may be easy to violate unknowingly). The present callbacks don't
> (and no such callback should) have any need for the lock to be held.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TODO: vm_event_disable() frees the structures used by respective
>        callbacks - need to either use call_rcu() for freeing, or maintain
>        a count of in-progress calls, for evtchn_close() to wait to drop
>        to zero before dropping the lock / returning.

I would go with the second solution and maintain a count of in-progress 
calls.

Tamas, Petre how does this sound?

Alex

> 
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -763,9 +763,18 @@ int evtchn_send(struct domain *ld, unsig
>           rport = lchn->u.interdomain.remote_port;
>           rchn  = evtchn_from_port(rd, rport);
>           if ( consumer_is_xen(rchn) )
> -            xen_notification_fn(rchn)(rd->vcpu[rchn->notify_vcpu_id], rport);
> -        else
> -            evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
> +        {
> +            /* Don't keep holding the lock for the call below. */
> +            xen_event_channel_notification_t fn = xen_notification_fn(rchn);
> +            struct vcpu *rv = rd->vcpu[rchn->notify_vcpu_id];
> +
> +            rcu_lock_domain(rd);
> +            spin_unlock_irqrestore(&lchn->lock, flags);
> +            fn(rv, rport);
> +            rcu_unlock_domain(rd);
> +            return 0;
> +        }
> +        evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
>           break;
>       case ECS_IPI:
>           evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);
>
Tamas K Lengyel Nov. 3, 2020, 2:54 p.m. UTC | #2
On Tue, Nov 3, 2020 at 5:17 AM Isaila Alexandru <aisaila@bitdefender.com> wrote:
>
>
> Hi Jan and sorry for the late reply,
>
> On 20.10.2020 17:13, Jan Beulich wrote:
> > While there don't look to be any problems with this right now, the lock
> > order implications from holding the lock can be very difficult to follow
> > (and may be easy to violate unknowingly). The present callbacks don't
> > (and no such callback should) have any need for the lock to be held.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > TODO: vm_event_disable() frees the structures used by respective
> >        callbacks - need to either use call_rcu() for freeing, or maintain
> >        a count of in-progress calls, for evtchn_close() to wait to drop
> >        to zero before dropping the lock / returning.
>
> I would go with the second solution and maintain a count of in-progress
> calls.
>
> Tamas, Petre how does this sound?

Agree, doing a reference count before freeing is preferred.

Thanks,
Tamas
diff mbox series

Patch

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -763,9 +763,18 @@  int evtchn_send(struct domain *ld, unsig
         rport = lchn->u.interdomain.remote_port;
         rchn  = evtchn_from_port(rd, rport);
         if ( consumer_is_xen(rchn) )
-            xen_notification_fn(rchn)(rd->vcpu[rchn->notify_vcpu_id], rport);
-        else
-            evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
+        {
+            /* Don't keep holding the lock for the call below. */
+            xen_event_channel_notification_t fn = xen_notification_fn(rchn);
+            struct vcpu *rv = rd->vcpu[rchn->notify_vcpu_id];
+
+            rcu_lock_domain(rd);
+            spin_unlock_irqrestore(&lchn->lock, flags);
+            fn(rv, rport);
+            rcu_unlock_domain(rd);
+            return 0;
+        }
+        evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn);
         break;
     case ECS_IPI:
         evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);