On 27.05.2021 20:48, Julien Grall wrote: > 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 > risk that port_is_valid() may be valid and then invalid because > d->valid_evtchns is decremented in evtchn_destroy().
While this is the case for other functions as well (and hence a comment along the lines of what you ask for below should have been in place already), I've added /* * While calling the function is okay without holding a suitable lock yet * (see the comment ahead of struct evtchn_port_ops for which ones those * are), for a dying domain it may start returning false at any point - see * evtchn_destroy(). This is not a fundamental problem though, as the * struct evtchn instance won't disappear (and will continue to hold valid * data) until final cleanup of the domain, at which point the domain itself * cannot be looked up anymore and hence calls here can't occur anymore in * the first place. */ ... > 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. ... ahead of port_is_valid() (and not, as I did intend originally, in evtchn_close()). As far as the commit message goes, I'll have it refer to the comment only. I hope this satisfies the requests of both of you. I'll take the liberty and retain your ack, Roger. Jan
