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 risk that port_is_valid() may be valid and then invalid because d->valid_evtchns is decremented in evtchn_destroy().
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.
Signed-off-by: Jan Beulich <jbeul...@suse.com> --- v6: Re-base for re-ordering / shrinking of series. v4: New. --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -606,17 +606,14 @@ int evtchn_close(struct domain *d1, int int port2; long rc = 0;- again:- spin_lock(&d1->event_lock); - if ( !port_is_valid(d1, port1) ) - { - rc = -EINVAL; - goto out; - } + return -EINVAL;chn1 = evtchn_from_port(d1, port1); + again:+ spin_lock(&d1->event_lock); + /* Guest cannot close a Xen-attached event channel. */ if ( unlikely(consumer_is_xen(chn1)) && guest ) { @@ -1041,16 +1038,13 @@ long evtchn_bind_vcpu(unsigned int port, if ( (v = domain_vcpu(d, vcpu_id)) == NULL ) return -ENOENT;- spin_lock(&d->event_lock);- if ( !port_is_valid(d, port) ) - { - rc = -EINVAL; - goto out; - } + return -EINVAL;chn = evtchn_from_port(d, port); + spin_lock(&d->event_lock);+ /* Guest cannot re-bind a Xen-attached event channel. */ if ( unlikely(consumer_is_xen(chn)) ) {
Cheers, -- Julien Grall
