* [PATCH v6 0/3] evtchn: (not so) recent XSAs follow-on
@ 2021-05-27 11:27 Jan Beulich
2021-05-27 11:28 ` [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Jan Beulich @ 2021-05-27 11:27 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu,
Stefano Stabellini
These are grouped into a series largely because of their origin,
not so much because there are (heavy) dependencies among them.
The main change from v6 is the dropping of three more patches,
and re-basing.
1: slightly defer lock acquire where possible
2: add helper for port_is_valid() + evtchn_from_port()
3: type adjustments
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-27 11:27 [PATCH v6 0/3] evtchn: (not so) recent XSAs follow-on Jan Beulich @ 2021-05-27 11:28 ` Jan Beulich 2021-05-27 13:46 ` Roger Pau Monné 2021-05-27 18:48 ` Julien Grall 2021-05-27 11:28 ` [PATCH v6 2/3] evtchn: add helper for port_is_valid() + evtchn_from_port() Jan Beulich 2021-05-27 11:28 ` [PATCH v6 3/3] evtchn: type adjustments Jan Beulich 2 siblings, 2 replies; 15+ messages in thread From: Jan Beulich @ 2021-05-27 11:28 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Roger Pau Monné 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(). Signed-off-by: Jan Beulich <jbeulich@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)) ) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-27 11:28 ` [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible Jan Beulich @ 2021-05-27 13:46 ` Roger Pau Monné 2021-05-27 18:48 ` Julien Grall 1 sibling, 0 replies; 15+ messages in thread From: Roger Pau Monné @ 2021-05-27 13:46 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini On Thu, May 27, 2021 at 01:28:07PM +0200, 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(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-27 11:28 ` [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible Jan Beulich 2021-05-27 13:46 ` Roger Pau Monné @ 2021-05-27 18:48 ` Julien Grall 2021-05-28 8:30 ` Roger Pau Monné 2021-06-01 11:54 ` Jan Beulich 1 sibling, 2 replies; 15+ messages in thread From: Julien Grall @ 2021-05-27 18:48 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini, Roger Pau Monné 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 <jbeulich@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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-27 18:48 ` Julien Grall @ 2021-05-28 8:30 ` Roger Pau Monné 2021-05-28 10:23 ` Jan Beulich 2021-06-01 11:54 ` Jan Beulich 1 sibling, 1 reply; 15+ messages in thread From: Roger Pau Monné @ 2021-05-28 8:30 UTC (permalink / raw) To: Julien Grall Cc: Jan Beulich, xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-28 8:30 ` Roger Pau Monné @ 2021-05-28 10:23 ` Jan Beulich 2021-05-28 10:48 ` Julien Grall 0 siblings, 1 reply; 15+ messages in thread From: Jan Beulich @ 2021-05-28 10:23 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini, Julien Grall On 28.05.2021 10:30, Roger Pau Monné wrote: > On Thu, May 27, 2021 at 07:48:41PM +0100, 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 > > The only instances evtchn_close is called with current != d and the > domain could be unpaused is in free_xen_event_channel AFAICT. As long as the domain is not paused, ->valid_evtchns can't ever decrease: The only point where this gets done is in evtchn_destroy(). Hence ... >> 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? ... I don't see this as relevant. >> 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. Because of the above property plus the fact that even if ->valid_evtchns decreases, the underlying struct evtchn instance will remain valid (i.e. won't get de-allocated, which happens only in evtchn_destroy_final()), it is always fine to use it without lock. With this I'm having trouble seeing what would need adding to port_is_valid()'s commentary. The only thing which shouldn't happen anywhere is following a port_is_valid() check which has returned false by code assuming the port is going to remain invalid. But I think that's obvious when you don't hold any suitable lock. I do intend to follow Julien's request to explain things more for evtchn_close(). Jan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-28 10:23 ` Jan Beulich @ 2021-05-28 10:48 ` Julien Grall 2021-05-28 13:31 ` Roger Pau Monné 0 siblings, 1 reply; 15+ messages in thread From: Julien Grall @ 2021-05-28 10:48 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monné Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini Hi Jan, On 28/05/2021 11:23, Jan Beulich wrote: > On 28.05.2021 10:30, Roger Pau Monné wrote: >> On Thu, May 27, 2021 at 07:48:41PM +0100, 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 >> >> The only instances evtchn_close is called with current != d and the >> domain could be unpaused is in free_xen_event_channel AFAICT. > > As long as the domain is not paused, ->valid_evtchns can't ever > decrease: The only point where this gets done is in evtchn_destroy(). > Hence ... > >>> 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? > > ... I don't see this as relevant. > >>> 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. > > Because of the above property plus the fact that even if > ->valid_evtchns decreases, the underlying struct evtchn instance > will remain valid (i.e. won't get de-allocated, which happens only > in evtchn_destroy_final()), it is always fine to use it without > lock. With this I'm having trouble seeing what would need adding > to port_is_valid()'s commentary. Lets take the example of free_xen_event_channel(). The function is checking if the port is valid. If it is, then evtchn_close() will be called. At this point, it would be fair for a developper to assume that port_is_valid() will also return true in event_close(). To push to the extreme, if free_xen_event_channel() was the only caller of evtchn_close(), one could argue that the check in evtchn_close() could be a BUG_ON(). However, this can't be because this would sooner or later turn to an XSA. Effectively, it means that is_port_valid() *cannot* be used in an ASSERT()/BUG_ON() and every caller should check the return even if the port was previously validated. So I think a comment on top of is_port_valid() would be really useful before someone rediscover it the hard way. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-28 10:48 ` Julien Grall @ 2021-05-28 13:31 ` Roger Pau Monné 2021-05-28 13:41 ` Jan Beulich 2021-05-28 14:26 ` Julien Grall 0 siblings, 2 replies; 15+ messages in thread From: Roger Pau Monné @ 2021-05-28 13:31 UTC (permalink / raw) To: Julien Grall Cc: Jan Beulich, xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini On Fri, May 28, 2021 at 11:48:51AM +0100, Julien Grall wrote: > Hi Jan, > > On 28/05/2021 11:23, Jan Beulich wrote: > > On 28.05.2021 10:30, Roger Pau Monné wrote: > > > On Thu, May 27, 2021 at 07:48:41PM +0100, 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 > > > > > > The only instances evtchn_close is called with current != d and the > > > domain could be unpaused is in free_xen_event_channel AFAICT. > > > > As long as the domain is not paused, ->valid_evtchns can't ever > > decrease: The only point where this gets done is in evtchn_destroy(). > > Hence ... > > > > > > 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? > > > > ... I don't see this as relevant. > > > > > > 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. > > > > Because of the above property plus the fact that even if > > ->valid_evtchns decreases, the underlying struct evtchn instance > > will remain valid (i.e. won't get de-allocated, which happens only > > in evtchn_destroy_final()), it is always fine to use it without > > lock. With this I'm having trouble seeing what would need adding > > to port_is_valid()'s commentary. > > Lets take the example of free_xen_event_channel(). The function is checking > if the port is valid. If it is, then evtchn_close() will be called. > > At this point, it would be fair for a developper to assume that > port_is_valid() will also return true in event_close(). > > To push to the extreme, if free_xen_event_channel() was the only caller of > evtchn_close(), one could argue that the check in evtchn_close() could be a > BUG_ON(). > > However, this can't be because this would sooner or later turn to an XSA. > > Effectively, it means that is_port_valid() *cannot* be used in an > ASSERT()/BUG_ON() and every caller should check the return even if the port > was previously validated. We already have cases of is_port_valid being used in ASSERTs (in the shim) and a BUG_ON (with the domain event lock held in evtchn_close). > So I think a comment on top of is_port_valid() would be really useful before > someone rediscover it the hard way. I think I'm being extremely dull here, sorry. From your text I understand that the value returned by is_port_valid could be stale by the time the user reads it? I think there's some condition that makes this value stale, and it's not the common case? Thanks, Roger. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-28 13:31 ` Roger Pau Monné @ 2021-05-28 13:41 ` Jan Beulich 2021-05-28 14:26 ` Julien Grall 1 sibling, 0 replies; 15+ messages in thread From: Jan Beulich @ 2021-05-28 13:41 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini, Julien Grall On 28.05.2021 15:31, Roger Pau Monné wrote: > I think I'm being extremely dull here, sorry. From your text I > understand that the value returned by is_port_valid could be stale by > the time the user reads it? > > I think there's some condition that makes this value stale, and it's > not the common case? It's evtchn_destroy() running in parallel which can have this effect. Jan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-28 13:31 ` Roger Pau Monné 2021-05-28 13:41 ` Jan Beulich @ 2021-05-28 14:26 ` Julien Grall 1 sibling, 0 replies; 15+ messages in thread From: Julien Grall @ 2021-05-28 14:26 UTC (permalink / raw) To: Roger Pau Monné Cc: Jan Beulich, xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini Hi Roger, On 28/05/2021 14:31, Roger Pau Monné wrote: > On Fri, May 28, 2021 at 11:48:51AM +0100, Julien Grall wrote: >> Hi Jan, >> >> On 28/05/2021 11:23, Jan Beulich wrote: >>> On 28.05.2021 10:30, Roger Pau Monné wrote: >>>> On Thu, May 27, 2021 at 07:48:41PM +0100, 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 >>>> >>>> The only instances evtchn_close is called with current != d and the >>>> domain could be unpaused is in free_xen_event_channel AFAICT. >>> >>> As long as the domain is not paused, ->valid_evtchns can't ever >>> decrease: The only point where this gets done is in evtchn_destroy(). >>> Hence ... >>> >>>>> 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? >>> >>> ... I don't see this as relevant. >>> >>>>> 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. >>> >>> Because of the above property plus the fact that even if >>> ->valid_evtchns decreases, the underlying struct evtchn instance >>> will remain valid (i.e. won't get de-allocated, which happens only >>> in evtchn_destroy_final()), it is always fine to use it without >>> lock. With this I'm having trouble seeing what would need adding >>> to port_is_valid()'s commentary. >> >> Lets take the example of free_xen_event_channel(). The function is checking >> if the port is valid. If it is, then evtchn_close() will be called. >> >> At this point, it would be fair for a developper to assume that >> port_is_valid() will also return true in event_close(). >> >> To push to the extreme, if free_xen_event_channel() was the only caller of >> evtchn_close(), one could argue that the check in evtchn_close() could be a >> BUG_ON(). >> >> However, this can't be because this would sooner or later turn to an XSA. >> >> Effectively, it means that is_port_valid() *cannot* be used in an >> ASSERT()/BUG_ON() and every caller should check the return even if the port >> was previously validated. > > We already have cases of is_port_valid being used in ASSERTs (in the > shim) and a BUG_ON (with the domain event lock held in evtchn_close). I was likely a bit too restrictive in my remark. The BUG_ON() in evtchn_close() is fine because this is used on a (in theory) an open port with the both domains event lock held. This would be more a problem if we have either: if ( !port_is_valid(d1, port1) ) return -EINVAL; /* .... */ BUG_ON(port_is_valid(d1, port1)); > >> So I think a comment on top of is_port_valid() would be really useful before >> someone rediscover it the hard way. > > I think I'm being extremely dull here, sorry. From your text I > understand that the value returned by is_port_valid could be stale by > the time the user reads it? > > I think there's some condition that makes this value stale, and it's > not the common case? It is any code that race with evtchn_destroy(). Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-05-27 18:48 ` Julien Grall 2021-05-28 8:30 ` Roger Pau Monné @ 2021-06-01 11:54 ` Jan Beulich 2021-06-07 18:15 ` Julien Grall 1 sibling, 1 reply; 15+ messages in thread From: Jan Beulich @ 2021-06-01 11:54 UTC (permalink / raw) To: Julien Grall, Roger Pau Monné Cc: Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini, xen-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible 2021-06-01 11:54 ` Jan Beulich @ 2021-06-07 18:15 ` Julien Grall 0 siblings, 0 replies; 15+ messages in thread From: Julien Grall @ 2021-06-07 18:15 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monné Cc: Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Stefano Stabellini, xen-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6 2/3] evtchn: add helper for port_is_valid() + evtchn_from_port() 2021-05-27 11:27 [PATCH v6 0/3] evtchn: (not so) recent XSAs follow-on Jan Beulich 2021-05-27 11:28 ` [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible Jan Beulich @ 2021-05-27 11:28 ` Jan Beulich 2021-05-27 11:28 ` [PATCH v6 3/3] evtchn: type adjustments Jan Beulich 2 siblings, 0 replies; 15+ messages in thread From: Jan Beulich @ 2021-05-27 11:28 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Roger Pau Monné The combination is pretty common, so adding a simple local helper seems worthwhile. Make it const- and type-correct, in turn requiring the two called function to also be const-correct (and at this occasion also make them type-correct). Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Julien Grall <jgrall@amazon.com> --- v6: Re-base, also for re-ordering / shrinking of series. v4: New. --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -147,6 +147,12 @@ static bool virq_is_global(unsigned int return true; } +static struct evtchn *_evtchn_from_port(const struct domain *d, + evtchn_port_t port) +{ + return port_is_valid(d, port) ? evtchn_from_port(d, port) : NULL; +} + static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket) { if ( !bucket ) @@ -319,7 +325,6 @@ static long evtchn_alloc_unbound(evtchn_ return rc; } - static void double_evtchn_lock(struct evtchn *lchn, struct evtchn *rchn) { ASSERT(lchn != rchn); @@ -365,9 +370,9 @@ static long evtchn_bind_interdomain(evtc ERROR_EXIT(lport); lchn = evtchn_from_port(ld, lport); - if ( !port_is_valid(rd, rport) ) + rchn = _evtchn_from_port(rd, rport); + if ( !rchn ) ERROR_EXIT_DOM(-EINVAL, rd); - rchn = evtchn_from_port(rd, rport); if ( (rchn->state != ECS_UNBOUND) || (rchn->u.unbound.remote_domid != ld->domain_id) ) ERROR_EXIT_DOM(-EINVAL, rd); @@ -602,15 +607,12 @@ static long evtchn_bind_pirq(evtchn_bind int evtchn_close(struct domain *d1, int port1, bool guest) { struct domain *d2 = NULL; - struct evtchn *chn1, *chn2; - int port2; + struct evtchn *chn1 = _evtchn_from_port(d1, port1), *chn2; long rc = 0; - if ( !port_is_valid(d1, port1) ) + if ( !chn1 ) return -EINVAL; - chn1 = evtchn_from_port(d1, port1); - again: spin_lock(&d1->event_lock); @@ -698,10 +700,8 @@ int evtchn_close(struct domain *d1, int goto out; } - port2 = chn1->u.interdomain.remote_port; - BUG_ON(!port_is_valid(d2, port2)); - - chn2 = evtchn_from_port(d2, port2); + chn2 = _evtchn_from_port(d2, chn1->u.interdomain.remote_port); + BUG_ON(!chn2); BUG_ON(chn2->state != ECS_INTERDOMAIN); BUG_ON(chn2->u.interdomain.remote_dom != d1); @@ -739,15 +739,13 @@ int evtchn_close(struct domain *d1, int int evtchn_send(struct domain *ld, unsigned int lport) { - struct evtchn *lchn, *rchn; + struct evtchn *lchn = _evtchn_from_port(ld, lport), *rchn; struct domain *rd; int rport, ret = 0; - if ( !port_is_valid(ld, lport) ) + if ( !lchn ) return -EINVAL; - lchn = evtchn_from_port(ld, lport); - evtchn_read_lock(lchn); /* Guest cannot send via a Xen-attached event channel. */ @@ -967,15 +965,15 @@ int evtchn_status(evtchn_status_t *statu if ( d == NULL ) return -ESRCH; - spin_lock(&d->event_lock); - - if ( !port_is_valid(d, port) ) + chn = _evtchn_from_port(d, port); + if ( !chn ) { - rc = -EINVAL; - goto out; + rcu_unlock_domain(d); + return -EINVAL; } - chn = evtchn_from_port(d, port); + spin_lock(&d->event_lock); + if ( consumer_is_xen(chn) ) { rc = -EACCES; @@ -1038,11 +1036,10 @@ long evtchn_bind_vcpu(unsigned int port, if ( (v = domain_vcpu(d, vcpu_id)) == NULL ) return -ENOENT; - if ( !port_is_valid(d, port) ) + chn = _evtchn_from_port(d, port); + if ( !chn ) return -EINVAL; - chn = evtchn_from_port(d, port); - spin_lock(&d->event_lock); /* Guest cannot re-bind a Xen-attached event channel. */ @@ -1088,13 +1085,11 @@ long evtchn_bind_vcpu(unsigned int port, int evtchn_unmask(unsigned int port) { struct domain *d = current->domain; - struct evtchn *evtchn; + struct evtchn *evtchn = _evtchn_from_port(d, port); - if ( unlikely(!port_is_valid(d, port)) ) + if ( unlikely(!evtchn) ) return -EINVAL; - evtchn = evtchn_from_port(d, port); - evtchn_read_lock(evtchn); evtchn_port_unmask(d, evtchn); @@ -1177,14 +1172,12 @@ static long evtchn_set_priority(const st { struct domain *d = current->domain; unsigned int port = set_priority->port; - struct evtchn *chn; + struct evtchn *chn = _evtchn_from_port(d, port); long ret; - if ( !port_is_valid(d, port) ) + if ( !chn ) return -EINVAL; - chn = evtchn_from_port(d, port); - evtchn_read_lock(chn); ret = evtchn_port_set_priority(d, chn, set_priority->priority); @@ -1410,10 +1403,10 @@ void free_xen_event_channel(struct domai void notify_via_xen_event_channel(struct domain *ld, int lport) { - struct evtchn *lchn, *rchn; + struct evtchn *lchn = _evtchn_from_port(ld, lport), *rchn; struct domain *rd; - if ( !port_is_valid(ld, lport) ) + if ( !lchn ) { /* * Make sure ->is_dying is read /after/ ->valid_evtchns, pairing @@ -1424,8 +1417,6 @@ void notify_via_xen_event_channel(struct return; } - lchn = evtchn_from_port(ld, lport); - if ( !evtchn_read_trylock(lchn) ) return; @@ -1580,12 +1571,14 @@ static void domain_dump_evtchn_info(stru spin_lock(&d->event_lock); - for ( port = 1; port_is_valid(d, port); ++port ) + for ( port = 1; ; ++port ) { - const struct evtchn *chn; + const struct evtchn *chn = _evtchn_from_port(d, port); char *ssid; - chn = evtchn_from_port(d, port); + if ( !chn ) + break; + if ( chn->state == ECS_FREE ) continue; --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -120,7 +120,7 @@ static inline void evtchn_read_unlock(st read_unlock(&evtchn->lock); } -static inline bool_t port_is_valid(struct domain *d, unsigned int p) +static inline bool port_is_valid(const struct domain *d, evtchn_port_t p) { if ( p >= read_atomic(&d->valid_evtchns) ) return false; @@ -135,7 +135,8 @@ static inline bool_t port_is_valid(struc return true; } -static inline struct evtchn *evtchn_from_port(struct domain *d, unsigned int p) +static inline struct evtchn *evtchn_from_port(const struct domain *d, + evtchn_port_t p) { if ( p < EVTCHNS_PER_BUCKET ) return &d->evtchn[array_index_nospec(p, EVTCHNS_PER_BUCKET)]; ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6 3/3] evtchn: type adjustments 2021-05-27 11:27 [PATCH v6 0/3] evtchn: (not so) recent XSAs follow-on Jan Beulich 2021-05-27 11:28 ` [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible Jan Beulich 2021-05-27 11:28 ` [PATCH v6 2/3] evtchn: add helper for port_is_valid() + evtchn_from_port() Jan Beulich @ 2021-05-27 11:28 ` Jan Beulich 2021-05-27 13:52 ` Roger Pau Monné 2 siblings, 1 reply; 15+ messages in thread From: Jan Beulich @ 2021-05-27 11:28 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Roger Pau Monné First of all avoid "long" when "int" suffices, i.e. in particular when merely conveying error codes. 32-bit values are slightly cheaper to deal with on x86, and their processing is at least no more expensive on Arm. Where possible use evtchn_port_t for port numbers and unsigned int for other unsigned quantities in adjacent code. In evtchn_set_priority() eliminate a local variable altogether instead of changing its type. Signed-off-by: Jan Beulich <jbeulich@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 @@ -284,13 +284,12 @@ void evtchn_free(struct domain *d, struc xsm_evtchn_close_post(chn); } -static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) { struct evtchn *chn; struct domain *d; - int port; + int port, rc; domid_t dom = alloc->dom; - long rc; d = rcu_lock_domain_by_any_id(dom); if ( d == NULL ) @@ -342,13 +341,13 @@ static void double_evtchn_unlock(struct evtchn_write_unlock(rchn); } -static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) +static int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind) { struct evtchn *lchn, *rchn; struct domain *ld = current->domain, *rd; - int lport, rport = bind->remote_port; + int lport, rc; + evtchn_port_t rport = bind->remote_port; domid_t rdom = bind->remote_dom; - long rc; if ( (rd = rcu_lock_domain_by_any_id(rdom)) == NULL ) return -ESRCH; @@ -484,12 +483,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t } -static long evtchn_bind_ipi(evtchn_bind_ipi_t *bind) +static int evtchn_bind_ipi(evtchn_bind_ipi_t *bind) { struct evtchn *chn; struct domain *d = current->domain; - int port, vcpu = bind->vcpu; - long rc = 0; + int port, rc = 0; + unsigned int vcpu = bind->vcpu; if ( domain_vcpu(d, vcpu) == NULL ) return -ENOENT; @@ -543,16 +542,16 @@ static void unlink_pirq_port(struct evtc } -static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind) +static int evtchn_bind_pirq(evtchn_bind_pirq_t *bind) { struct evtchn *chn; struct domain *d = current->domain; struct vcpu *v = d->vcpu[0]; struct pirq *info; - int port = 0, pirq = bind->pirq; - long rc; + int port = 0, rc; + unsigned int pirq = bind->pirq; - if ( (pirq < 0) || (pirq >= d->nr_pirqs) ) + if ( pirq >= d->nr_pirqs ) return -EINVAL; if ( !is_hvm_domain(d) && !pirq_access_permitted(d, pirq) ) @@ -608,7 +607,7 @@ int evtchn_close(struct domain *d1, int { struct domain *d2 = NULL; struct evtchn *chn1 = _evtchn_from_port(d1, port1), *chn2; - long rc = 0; + int rc = 0; if ( !chn1 ) return -EINVAL; @@ -959,7 +958,7 @@ int evtchn_status(evtchn_status_t *statu domid_t dom = status->dom; int port = status->port; struct evtchn *chn; - long rc = 0; + int rc = 0; d = rcu_lock_domain_by_any_id(dom); if ( d == NULL ) @@ -1025,11 +1024,11 @@ int evtchn_status(evtchn_status_t *statu } -long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id) +int evtchn_bind_vcpu(evtchn_port_t port, unsigned int vcpu_id) { struct domain *d = current->domain; struct evtchn *chn; - long rc = 0; + int rc = 0; struct vcpu *v; /* Use the vcpu info to prevent speculative out-of-bound accesses */ @@ -1168,12 +1167,11 @@ int evtchn_reset(struct domain *d, bool return rc; } -static long evtchn_set_priority(const struct evtchn_set_priority *set_priority) +static int evtchn_set_priority(const struct evtchn_set_priority *set_priority) { struct domain *d = current->domain; - unsigned int port = set_priority->port; - struct evtchn *chn = _evtchn_from_port(d, port); - long ret; + struct evtchn *chn = _evtchn_from_port(d, set_priority->port); + int ret; if ( !chn ) return -EINVAL; @@ -1189,7 +1187,7 @@ static long evtchn_set_priority(const st long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { - long rc; + int rc; switch ( cmd ) { --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -54,7 +54,7 @@ void send_guest_pirq(struct domain *, co int evtchn_send(struct domain *d, unsigned int lport); /* Bind a local event-channel port to the specified VCPU. */ -long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id); +int evtchn_bind_vcpu(evtchn_port_t port, unsigned int vcpu_id); /* Bind a VIRQ. */ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6 3/3] evtchn: type adjustments 2021-05-27 11:28 ` [PATCH v6 3/3] evtchn: type adjustments Jan Beulich @ 2021-05-27 13:52 ` Roger Pau Monné 0 siblings, 0 replies; 15+ messages in thread From: Roger Pau Monné @ 2021-05-27 13:52 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini On Thu, May 27, 2021 at 01:28:59PM +0200, Jan Beulich wrote: > First of all avoid "long" when "int" suffices, i.e. in particular when > merely conveying error codes. 32-bit values are slightly cheaper to > deal with on x86, and their processing is at least no more expensive on > Arm. Where possible use evtchn_port_t for port numbers and unsigned int > for other unsigned quantities in adjacent code. In evtchn_set_priority() > eliminate a local variable altogether instead of changing its type. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> Just one style nit below. > -long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id) > +int evtchn_bind_vcpu(evtchn_port_t port, unsigned int vcpu_id) > { > struct domain *d = current->domain; > struct evtchn *chn; > - long rc = 0; > + int rc = 0; ^ I think you are missing one space here, other functions in the same file don't align at the *. Thanks, Roger. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-06-07 18:15 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-27 11:27 [PATCH v6 0/3] evtchn: (not so) recent XSAs follow-on Jan Beulich 2021-05-27 11:28 ` [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible Jan Beulich 2021-05-27 13:46 ` Roger Pau Monné 2021-05-27 18:48 ` Julien Grall 2021-05-28 8:30 ` Roger Pau Monné 2021-05-28 10:23 ` Jan Beulich 2021-05-28 10:48 ` Julien Grall 2021-05-28 13:31 ` Roger Pau Monné 2021-05-28 13:41 ` Jan Beulich 2021-05-28 14:26 ` Julien Grall 2021-06-01 11:54 ` Jan Beulich 2021-06-07 18:15 ` Julien Grall 2021-05-27 11:28 ` [PATCH v6 2/3] evtchn: add helper for port_is_valid() + evtchn_from_port() Jan Beulich 2021-05-27 11:28 ` [PATCH v6 3/3] evtchn: type adjustments Jan Beulich 2021-05-27 13:52 ` Roger Pau Monné
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).