On 30.09.2020 10:36, Paul Durrant wrote: >> From: Jan Beulich <jbeul...@suse.com> >> Sent: 30 September 2020 09:32 >> >> On 30.09.2020 09:31, Paul Durrant wrote: >>>> From: Jan Beulich <jbeul...@suse.com> >>>> Sent: 30 September 2020 07:42 >>>> >>>> On 29.09.2020 18:31, Paul Durrant wrote: >>>>>> From: Xen-devel <xen-devel-boun...@lists.xenproject.org> 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
