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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 30 September 2020 09:32
> 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 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?

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