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

From: Jan Beulich <jbeulich@suse.com>
To: paul@xen.org
Cc: xen-devel@lists.xenproject.org,
	'Andrew Cooper' <andrew.cooper3@citrix.com>,
	'George Dunlap' <George.Dunlap@eu.citrix.com>,
	'Ian Jackson' <iwj@xenproject.org>,
	'Julien Grall' <julien@xen.org>, 'Wei Liu' <wl@xen.org>,
	'Stefano Stabellini' <sstabellini@kernel.org>
Subject: Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock
Date: Wed, 30 Sep 2020 10:41:39 +0200	[thread overview]
Message-ID: <2d2eb462-bd0e-a467-19a9-2bf2d55adcd8@suse.com> (raw)
In-Reply-To: <005f01d69704$c0c11130$42433390$@xen.org>

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