Hi Jan,
On 01/06/2021 12:54, Jan Beulich wrote:
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.
Yes, this satistfies my requests. Feel free to add my reviewed-by on the
patch.
Cheers,
--
Julien Grall
