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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2020 09:31, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 30 September 2020 07:42
>> To: paul@xxxxxxx
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; '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: 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 <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?
>>
>> 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