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

Commit Message

Jan Beulich Sept. 28, 2020, 10:57 a.m. UTC

evtchn_fifo_set_pending() (invoked with the per-channel lock held) has
two uses of the channel's priority field. 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.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Julien Grall Sept. 29, 2020, 10:21 a.m. UTC | #1

Hi Jan,

On 28/09/2020 11:57, Jan Beulich wrote:
> evtchn_fifo_set_pending() (invoked with the per-channel lock held) has
> two uses of the channel's priority field. 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.
> 
> Signed-off-by: Jan Beulich <[e-mail]>
> 
> --- 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);

Is it still necessary to hold 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);
>   
> 

Cheers,

Jan Beulich Sept. 29, 2020, 11:49 a.m. UTC | #2

On 29.09.2020 12:21, Julien Grall wrote:
> On 28/09/2020 11:57, Jan Beulich wrote:
>> evtchn_fifo_set_pending() (invoked with the per-channel lock held) has
>> two uses of the channel's priority field. 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.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
>>
>> --- 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);
> 
> Is it still necessary to hold d->event_lock?

Good point - I see no reason for it to be held anymore.

Jan

Paul Durrant Sept. 29, 2020, 4:31 p.m. UTC | #3

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> Sent: 28 September 2020 11:58
> To: [e-mail]
> Cc: Andrew Cooper <[e-mail]>; George Dunlap <[e-mail]>; Ian
> Jackson <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>; Stefano Stabellini
> <[e-mail]>
> 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 <[e-mail]>
> 
> --- 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);
> 
>

Jan Beulich Sept. 30, 2020, 6:29 a.m. UTC | #4

On 29.09.2020 18:31, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>> Sent: 28 September 2020 11:58
>> To: [e-mail]
>> Cc: Andrew Cooper <[e-mail]>; George Dunlap <[e-mail]>; Ian
>> Jackson <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>; Stefano Stabellini
>> <[e-mail]>
>> 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...

In the interdomain case you mean?

>> 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?

Indeed, this will require more thought then.

Jan

Jan Beulich Sept. 30, 2020, 6:41 a.m. UTC | #5

On 29.09.2020 18:31, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>> Sent: 28 September 2020 11:58
>> To: [e-mail]
>> Cc: Andrew Cooper <[e-mail]>; George Dunlap <[e-mail]>; Ian
>> Jackson <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>; Stefano Stabellini
>> <[e-mail]>
>> 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?

I guess the reasoning needs to change here - it should focus solely
on using the finer grained lock here (as holding the per-domain one
doesn't help anyway). It would then be patch 10 which addresses the
(FIFO-specific) concern of possibly reading inconsistent values.

Jan

Paul Durrant Sept. 30, 2020, 7:31 a.m. UTC | #6

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 30 September 2020 07:42
> To: [e-mail]
> Cc: [e-mail]; 'Andrew Cooper' <[e-mail]>; 'George Dunlap'
> <[e-mail]>; 'Ian Jackson' <[e-mail]>; 'Julien Grall' <[e-mail]>;
> 'Wei Liu' <[e-mail]>; 'Stefano Stabellini' <[e-mail]>
> Subject: Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock
> 
> On 29.09.2020 18:31, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> >> Sent: 28 September 2020 11:58
> >> To: [e-mail]
> >> Cc: Andrew Cooper <[e-mail]>; George Dunlap <[e-mail]>; Ian
> >> Jackson <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>; Stefano
> Stabellini
> >> <[e-mail]>
> >> 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?
> 
> I guess the reasoning needs to change here - it should focus solely
> on using the finer grained lock here (as holding the per-domain one
> doesn't help anyway). It would then be patch 10 which addresses the
> (FIFO-specific) concern of possibly reading inconsistent values.
> 

Yes, it looks like patch #10 should ensure consistency.

Prior to ad34d0656fc at least the first layer of calls done in evtchn_send() didn't take the evtchn itself as an arg. Of course, evtchn_set_pending() then looked up the evtchn and passed it to evtchn_port_set_pending() without any locking in the interdomain case. I wonder whether, to make reasoning easier, there ought to be a rule that ABI entry points are always called with the evtchn lock held?

  Paul

> Jan

Jan Beulich Sept. 30, 2020, 8:31 a.m. UTC | #7

On 30.09.2020 09:31, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 30 September 2020 07:42
>> To: [e-mail]
>> Cc: [e-mail]; 'Andrew Cooper' <[e-mail]>; 'George Dunlap'
>> <[e-mail]>; 'Ian Jackson' <[e-mail]>; 'Julien Grall' <[e-mail]>;
>> 'Wei Liu' <[e-mail]>; 'Stefano Stabellini' <[e-mail]>
>> Subject: Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock
>>
>> On 29.09.2020 18:31, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>>>> Sent: 28 September 2020 11:58
>>>> To: [e-mail]
>>>> Cc: Andrew Cooper <[e-mail]>; George Dunlap <[e-mail]>; Ian
>>>> Jackson <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>; Stefano
>> Stabellini
>>>> <[e-mail]>
>>>> 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?
>>
>> I guess the reasoning needs to change here - it should focus solely
>> on using the finer grained lock here (as holding the per-domain one
>> doesn't help anyway). It would then be patch 10 which addresses the
>> (FIFO-specific) concern of possibly reading inconsistent values.
>>
> 
> Yes, it looks like patch #10 should ensure consistency.
> 
> Prior to ad34d0656fc at least the first layer of calls done in evtchn_send() didn't take the evtchn itself as an arg. Of course, evtchn_set_pending() then looked up the evtchn and passed it to evtchn_port_set_pending() without any locking in the interdomain case. I wonder whether, to make reasoning easier, there ought to be a rule that ABI entry points are always called with the evtchn lock held?

What do you mean by "ABI entry points" here? To me this would sound
like what's directly accessible to guests, but that's hardly what
you mean. Do you perhaps mean the hooks in struct evtchn_port_ops?
As per the comment that got added there recently, the locking
unfortunately is less consistent there.

Jan

Paul Durrant Sept. 30, 2020, 8:36 a.m. UTC | #8

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 30 September 2020 09:32
> To: [e-mail]
> Cc: [e-mail]; 'Andrew Cooper' <[e-mail]>; 'George Dunlap'
> <[e-mail]>; 'Ian Jackson' <[e-mail]>; 'Julien Grall' <[e-mail]>;
> 'Wei Liu' <[e-mail]>; 'Stefano Stabellini' <[e-mail]>
> Subject: Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock
> 
> On 30.09.2020 09:31, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >> Sent: 30 September 2020 07:42
> >> To: [e-mail]
> >> Cc: [e-mail]; 'Andrew Cooper' <[e-mail]>; 'George Dunlap'
> >> <[e-mail]>; 'Ian Jackson' <[e-mail]>; 'Julien Grall' <[e-mail]>;
> >> 'Wei Liu' <[e-mail]>; 'Stefano Stabellini' <[e-mail]>
> >> Subject: Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock
> >>
> >> On 29.09.2020 18:31, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> >>>> Sent: 28 September 2020 11:58
> >>>> To: [e-mail]
> >>>> Cc: Andrew Cooper <[e-mail]>; George Dunlap <[e-mail]>; Ian
> >>>> Jackson <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>; Stefano
> >> Stabellini
> >>>> <[e-mail]>
> >>>> 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?
> >>
> >> I guess the reasoning needs to change here - it should focus solely
> >> on using the finer grained lock here (as holding the per-domain one
> >> doesn't help anyway). It would then be patch 10 which addresses the
> >> (FIFO-specific) concern of possibly reading inconsistent values.
> >>
> >
> > Yes, it looks like patch #10 should ensure consistency.
> >
> > Prior to ad34d0656fc at least the first layer of calls done in evtchn_send() didn't take the evtchn
> itself as an arg. Of course, evtchn_set_pending() then looked up the evtchn and passed it to
> evtchn_port_set_pending() without any locking in the interdomain case. I wonder whether, to make
> reasoning easier, there ought to be a rule that ABI entry points are always called with the evtchn
> lock held?
> 
> What do you mean by "ABI entry points" here? To me this would sound
> like what's directly accessible to guests, but that's hardly what
> you mean. Do you perhaps mean the hooks in struct evtchn_port_ops?

Yes, by ABI I mean 'fifo' or '2l'. (I guess that 'ABI' is just the name I chose to refer to them in the Windows PV driver code).

> As per the comment that got added there recently, the locking
> unfortunately is less consistent there.
> 

I looked to me that most functions were entered with channel lock held so wondered whether it could be a rule.

  Paul

> Jan

Jan Beulich Sept. 30, 2020, 8:41 a.m. UTC | #9

On 30.09.2020 10:36, Paul Durrant wrote:
>> From: Jan Beulich <[e-mail]>
>> Sent: 30 September 2020 09:32
>>
>> On 30.09.2020 09:31, Paul Durrant wrote:
>>>> From: Jan Beulich <[e-mail]>
>>>> Sent: 30 September 2020 07:42
>>>>
>>>> On 29.09.2020 18:31, Paul Durrant wrote:
>>>>>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>>>>>> Sent: 28 September 2020 11:58
>>>>>>
>>>>>> 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?
>>>>
>>>> I guess the reasoning needs to change here - it should focus solely
>>>> on using the finer grained lock here (as holding the per-domain one
>>>> doesn't help anyway). It would then be patch 10 which addresses the
>>>> (FIFO-specific) concern of possibly reading inconsistent values.
>>>>
>>>
>>> Yes, it looks like patch #10 should ensure consistency.
>>>
>>> Prior to ad34d0656fc at least the first layer of calls done in evtchn_send() didn't take the evtchn
>> itself as an arg. Of course, evtchn_set_pending() then looked up the evtchn and passed it to
>> evtchn_port_set_pending() without any locking in the interdomain case. I wonder whether, to make
>> reasoning easier, there ought to be a rule that ABI entry points are always called with the evtchn
>> lock held?
>>
>> What do you mean by "ABI entry points" here? To me this would sound
>> like what's directly accessible to guests, but that's hardly what
>> you mean. Do you perhaps mean the hooks in struct evtchn_port_ops?
> 
> Yes, by ABI I mean 'fifo' or '2l'. (I guess that 'ABI' is just the name I chose to refer to them in the Windows PV driver code).
> 
>> As per the comment that got added there recently, the locking
>> unfortunately is less consistent there.
>>
> 
> I looked to me that most functions were entered with channel lock
> held so wondered whether it could be a rule.

Well - especially for the sending paths it may be _a_ per-channel lock,
not _the_ one. While putting together the XSA fixed I had looked some at
the possibility of having a simple rule here, but acquiring _the_ lock
on the interdomain sending path looked to be complicating this path
quite a bit, when it specifically should be as lightweight as possible.

Jan

Patch

--- 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);