RE: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> Beulich
> Sent: 28 September 2020 11:58
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxxxxx>; Ian
> Jackson <iwj@xxxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>
> Subject: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the 
> per-channel lock
> 
> evtchn_fifo_set_pending() (invoked with the per-channel lock held) has
> two uses of the channel's priority field.

AFAICT it is invoked with only the sending end's lock held...

> The field gets updated by
> evtchn_fifo_set_priority() with only the per-domain event_lock held,
> i.e. the two reads may observe two different values. While the 2nd use
> could - afaict - in principle be replaced by q->priority, I think
> evtchn_set_priority() should acquire the per-channel lock in any event.
> 

... so how is this going to help?

  Paul

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1132,7 +1132,9 @@ static long evtchn_set_priority(const st
>  {
>      struct domain *d = current->domain;
>      unsigned int port = set_priority->port;
> +    struct evtchn *chn;
>      long ret;
> +    unsigned long flags;
> 
>      spin_lock(&d->event_lock);
> 
> @@ -1142,8 +1144,10 @@ static long evtchn_set_priority(const st
>          return -EINVAL;
>      }
> 
> -    ret = evtchn_port_set_priority(d, evtchn_from_port(d, port),
> -                                   set_priority->priority);
> +    chn = evtchn_from_port(d, port);
> +    spin_lock_irqsave(&chn->lock, flags);
> +    ret = evtchn_port_set_priority(d, chn, set_priority->priority);
> +    spin_unlock_irqrestore(&chn->lock, flags);
> 
>      spin_unlock(&d->event_lock);
> 
>