| Message ID | 20250107101711.5980-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remove libxenctrl usage from xenstored | expand |
On 07.01.2025 11:17, Juergen Gross wrote: > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -979,6 +979,7 @@ void send_global_virq(uint32_t virq) > int set_global_virq_handler(struct domain *d, uint32_t virq) > { > struct domain *old; > + int rc = 0; > > if (virq >= NR_VIRQS) > return -EINVAL; > @@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, uint32_t virq) > return -EINVAL; > > spin_lock(&global_virq_handlers_lock); > - old = global_virq_handlers[virq]; > - global_virq_handlers[virq] = d; > + > + if ( d->is_dying != DOMDYING_alive ) > + { > + old = d; > + rc = -EINVAL; > + } While I can see how this eliminates the zombie domain aspect, this doesn't fully eliminate the race. Doing so would require (also) using the domain's event lock. Assuming we're okay with the remaining race, imo a code comment would be needed to state this (including the fact that it's then unpredictable whether this operation might still succeed for a domain already having d->is_dying != DOMDYING_alive). Plus the way you do it the early success path remains; ideally that case would also fail for an already dying domain. > + else > + { > + old = global_virq_handlers[virq]; > + global_virq_handlers[virq] = d; > + } > spin_unlock(&global_virq_handlers_lock); Nit: Much like you do at the top of your addition, a new blank line at the bottom would be nice. Jan
On 07.01.25 16:23, Jan Beulich wrote: > On 07.01.2025 11:17, Juergen Gross wrote: >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -979,6 +979,7 @@ void send_global_virq(uint32_t virq) >> int set_global_virq_handler(struct domain *d, uint32_t virq) >> { >> struct domain *old; >> + int rc = 0; >> >> if (virq >= NR_VIRQS) >> return -EINVAL; >> @@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, uint32_t virq) >> return -EINVAL; >> >> spin_lock(&global_virq_handlers_lock); >> - old = global_virq_handlers[virq]; >> - global_virq_handlers[virq] = d; >> + >> + if ( d->is_dying != DOMDYING_alive ) >> + { >> + old = d; >> + rc = -EINVAL; >> + } > > While I can see how this eliminates the zombie domain aspect, this doesn't > fully eliminate the race. Doing so would require (also) using the domain's > event lock. Assuming we're okay with the remaining race, imo a code comment > would be needed to state this (including the fact that it's then > unpredictable whether this operation might still succeed for a domain > already having d->is_dying != DOMDYING_alive). AFAIU you mean that it is still possible to set a domain to handle a virq when it is in the process of going down, especially if is_dying is set just after it has been tested to be DOMDYING_alive? I don't see this being a problem, as the same would happen if the domain would go down just a millisecond later. This is something we will never be able to handle. And after all the call of clear_global_virq_handlers() will now reset the handling domain to the hardware domain in all cases. > > Plus the way you do it the early success path remains; ideally that case > would also fail for an already dying domain. Same again: clear_global_virq_handlers() will reset the handling domain. > >> + else >> + { >> + old = global_virq_handlers[virq]; >> + global_virq_handlers[virq] = d; >> + } >> spin_unlock(&global_virq_handlers_lock); > > Nit: Much like you do at the top of your addition, a new blank line at the > bottom would be nice. Will add that if a respin is needed. Juergen
On 07.01.2025 16:37, Juergen Gross wrote: > On 07.01.25 16:23, Jan Beulich wrote: >> On 07.01.2025 11:17, Juergen Gross wrote: >>> --- a/xen/common/event_channel.c >>> +++ b/xen/common/event_channel.c >>> @@ -979,6 +979,7 @@ void send_global_virq(uint32_t virq) >>> int set_global_virq_handler(struct domain *d, uint32_t virq) >>> { >>> struct domain *old; >>> + int rc = 0; >>> >>> if (virq >= NR_VIRQS) >>> return -EINVAL; >>> @@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, uint32_t virq) >>> return -EINVAL; >>> >>> spin_lock(&global_virq_handlers_lock); >>> - old = global_virq_handlers[virq]; >>> - global_virq_handlers[virq] = d; >>> + >>> + if ( d->is_dying != DOMDYING_alive ) >>> + { >>> + old = d; >>> + rc = -EINVAL; >>> + } >> >> While I can see how this eliminates the zombie domain aspect, this doesn't >> fully eliminate the race. Doing so would require (also) using the domain's >> event lock. Assuming we're okay with the remaining race, imo a code comment >> would be needed to state this (including the fact that it's then >> unpredictable whether this operation might still succeed for a domain >> already having d->is_dying != DOMDYING_alive). > > AFAIU you mean that it is still possible to set a domain to handle a virq > when it is in the process of going down, especially if is_dying is set just > after it has been tested to be DOMDYING_alive? > > I don't see this being a problem, as the same would happen if the domain > would go down just a millisecond later. This is something we will never be > able to handle. Right, but the sequence of events in the case you mention is different: The insertion into the array would still happen while the domain isn't marked dying. > And after all the call of clear_global_virq_handlers() will now reset the > handling domain to the hardware domain in all cases. Of course, but in the meantime an event may be sent to such a domain already marked dying. That likely isn't going to cause problems, but is unexpected with what description here says is being addressed. >> Plus the way you do it the early success path remains; ideally that case >> would also fail for an already dying domain. > > Same again: clear_global_virq_handlers() will reset the handling domain. Right. In summary: As indicated, we may be okay with the remaining race, but then we also should be making clear that we've decided to leave it at that. Hence my earlier request: If we accept this, say (and briefly justify) this in a code comment. Jan
On 07.01.25 16:49, Jan Beulich wrote: > On 07.01.2025 16:37, Juergen Gross wrote: >> On 07.01.25 16:23, Jan Beulich wrote: >>> On 07.01.2025 11:17, Juergen Gross wrote: >>>> --- a/xen/common/event_channel.c >>>> +++ b/xen/common/event_channel.c >>>> @@ -979,6 +979,7 @@ void send_global_virq(uint32_t virq) >>>> int set_global_virq_handler(struct domain *d, uint32_t virq) >>>> { >>>> struct domain *old; >>>> + int rc = 0; >>>> >>>> if (virq >= NR_VIRQS) >>>> return -EINVAL; >>>> @@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, uint32_t virq) >>>> return -EINVAL; >>>> >>>> spin_lock(&global_virq_handlers_lock); >>>> - old = global_virq_handlers[virq]; >>>> - global_virq_handlers[virq] = d; >>>> + >>>> + if ( d->is_dying != DOMDYING_alive ) >>>> + { >>>> + old = d; >>>> + rc = -EINVAL; >>>> + } >>> >>> While I can see how this eliminates the zombie domain aspect, this doesn't >>> fully eliminate the race. Doing so would require (also) using the domain's >>> event lock. Assuming we're okay with the remaining race, imo a code comment >>> would be needed to state this (including the fact that it's then >>> unpredictable whether this operation might still succeed for a domain >>> already having d->is_dying != DOMDYING_alive). >> >> AFAIU you mean that it is still possible to set a domain to handle a virq >> when it is in the process of going down, especially if is_dying is set just >> after it has been tested to be DOMDYING_alive? >> >> I don't see this being a problem, as the same would happen if the domain >> would go down just a millisecond later. This is something we will never be >> able to handle. > > Right, but the sequence of events in the case you mention is different: The > insertion into the array would still happen while the domain isn't marked > dying. > >> And after all the call of clear_global_virq_handlers() will now reset the >> handling domain to the hardware domain in all cases. > > Of course, but in the meantime an event may be sent to such a domain already > marked dying. That likely isn't going to cause problems, but is unexpected > with what description here says is being addressed. > >>> Plus the way you do it the early success path remains; ideally that case >>> would also fail for an already dying domain. >> >> Same again: clear_global_virq_handlers() will reset the handling domain. > > Right. > > In summary: As indicated, we may be okay with the remaining race, but then > we also should be making clear that we've decided to leave it at that. > Hence my earlier request: If we accept this, say (and briefly justify) this > in a code comment. Okay, would you be fine with: Note that this check won't guarantee that a domain just going down can't be set as the handling domain of a virq, as the is_dying indicator might change just after testing it. This isn't going to be a major problem, as clear_global_virq_handlers() is guaranteed to run afterwards and it will reset the handling domain for the virq to the hardware domain. Juergen
On 07.01.2025 16:56, Juergen Gross wrote: > On 07.01.25 16:49, Jan Beulich wrote: >> On 07.01.2025 16:37, Juergen Gross wrote: >>> On 07.01.25 16:23, Jan Beulich wrote: >>>> On 07.01.2025 11:17, Juergen Gross wrote: >>>>> --- a/xen/common/event_channel.c >>>>> +++ b/xen/common/event_channel.c >>>>> @@ -979,6 +979,7 @@ void send_global_virq(uint32_t virq) >>>>> int set_global_virq_handler(struct domain *d, uint32_t virq) >>>>> { >>>>> struct domain *old; >>>>> + int rc = 0; >>>>> >>>>> if (virq >= NR_VIRQS) >>>>> return -EINVAL; >>>>> @@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, uint32_t virq) >>>>> return -EINVAL; >>>>> >>>>> spin_lock(&global_virq_handlers_lock); >>>>> - old = global_virq_handlers[virq]; >>>>> - global_virq_handlers[virq] = d; >>>>> + >>>>> + if ( d->is_dying != DOMDYING_alive ) >>>>> + { >>>>> + old = d; >>>>> + rc = -EINVAL; >>>>> + } >>>> >>>> While I can see how this eliminates the zombie domain aspect, this doesn't >>>> fully eliminate the race. Doing so would require (also) using the domain's >>>> event lock. Assuming we're okay with the remaining race, imo a code comment >>>> would be needed to state this (including the fact that it's then >>>> unpredictable whether this operation might still succeed for a domain >>>> already having d->is_dying != DOMDYING_alive). >>> >>> AFAIU you mean that it is still possible to set a domain to handle a virq >>> when it is in the process of going down, especially if is_dying is set just >>> after it has been tested to be DOMDYING_alive? >>> >>> I don't see this being a problem, as the same would happen if the domain >>> would go down just a millisecond later. This is something we will never be >>> able to handle. >> >> Right, but the sequence of events in the case you mention is different: The >> insertion into the array would still happen while the domain isn't marked >> dying. >> >>> And after all the call of clear_global_virq_handlers() will now reset the >>> handling domain to the hardware domain in all cases. >> >> Of course, but in the meantime an event may be sent to such a domain already >> marked dying. That likely isn't going to cause problems, but is unexpected >> with what description here says is being addressed. >> >>>> Plus the way you do it the early success path remains; ideally that case >>>> would also fail for an already dying domain. >>> >>> Same again: clear_global_virq_handlers() will reset the handling domain. >> >> Right. >> >> In summary: As indicated, we may be okay with the remaining race, but then >> we also should be making clear that we've decided to leave it at that. >> Hence my earlier request: If we accept this, say (and briefly justify) this >> in a code comment. > > Okay, would you be fine with: > > Note that this check won't guarantee that a domain just going down can't be > set as the handling domain of a virq, as the is_dying indicator might change > just after testing it. > This isn't going to be a major problem, as clear_global_virq_handlers() is > guaranteed to run afterwards and it will reset the handling domain for the > virq to the hardware domain. Reads okay, thanks. Jan
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 8db2ca4ba2..f2b64c48fb 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -979,6 +979,7 @@ void send_global_virq(uint32_t virq) int set_global_virq_handler(struct domain *d, uint32_t virq) { struct domain *old; + int rc = 0; if (virq >= NR_VIRQS) return -EINVAL; @@ -992,14 +993,23 @@ int set_global_virq_handler(struct domain *d, uint32_t virq) return -EINVAL; spin_lock(&global_virq_handlers_lock); - old = global_virq_handlers[virq]; - global_virq_handlers[virq] = d; + + if ( d->is_dying != DOMDYING_alive ) + { + old = d; + rc = -EINVAL; + } + else + { + old = global_virq_handlers[virq]; + global_virq_handlers[virq] = d; + } spin_unlock(&global_virq_handlers_lock); if (old != NULL) put_domain(old); - return 0; + return rc; } static void clear_global_virq_handlers(struct domain *d)
There is a possible race scenario between set_global_virq_handler() and clear_global_virq_handlers() targeting the same domain, which might result in that domain ending as a zombie domain. In case set_global_virq_handler() is being called for a domain which is just dying, it might happen that clear_global_virq_handlers() is running first, resulting in set_global_virq_handler() taking a new reference for that domain and entering in the global_virq_handlers[] array afterwards. The reference will never be dropped, thus the domain will never be freed completely. This can be fixed by checking the is_dying state of the domain inside the region guarded by global_virq_handlers_lock. In case the domain is dying, handle it as if the domain wouldn't exist, which will be the case in near future anyway. Fixes: 87521589aa6a ("xen: allow global VIRQ handlers to be delegated to other domains") Signed-off-by: Juergen Gross <jgross@suse.com> --- V6: - new patch --- xen/common/event_channel.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)