On Thu, May 27, 2021 at 07:48:41PM +0100, Julien Grall wrote: > Hi Jan, > > On 27/05/2021 12:28, Jan Beulich wrote: > > port_is_valid() and evtchn_from_port() are fine to use without holding > > any locks. Accordingly acquire the per-domain lock slightly later in > > evtchn_close() and evtchn_bind_vcpu(). > > So I agree that port_is_valid() and evtchn_from_port() are fine to use > without holding any locks in evtchn_bind_vcpu(). However, this is misleading > to say there is no problem with evtchn_close(). > > evtchn_close() can be called with current != d and therefore, there is a
The only instances evtchn_close is called with current != d and the domain could be unpaused is in free_xen_event_channel AFAICT. > risk that port_is_valid() may be valid and then invalid because > d->valid_evtchns is decremented in evtchn_destroy(). Hm, I guess you could indeed have parallel calls to free_xen_event_channel and evtchn_destroy in a way that free_xen_event_channel could race with valid_evtchns getting decreased? All callers of free_xen_event_channel are internal to the hypervisor, so maybe with proper ordering of the operations this could be avoided, albeit it's not easy to assert. > Thankfully the memory is still there. So the current code is okayish and I > could reluctantly accept this behavior to be spread. However, I don't think > this should be left uncommented in both the code (maybe on top of > port_is_valid()?) and the commit message. Indeed, I think we need some expansion of the comment in port_is_valid to clarify all this. I'm not sure I understand it properly myself when it's fine to use port_is_valid without holding the per domain event lock. evtchn_close shouldn't be a very common operation anyway, so it holding the per domain lock a bit longer doesn't seem that critical to me anyway. Thanks, Roger.
