Re: [Xen-devel] [PATCH 1/2] xen: fix a (latent) cpupool-related race during domain destroy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/07/16 15:54, Dario Faggioli wrote:
> On Thu, 2016-07-14 at 10:37 +0100, Andrew Cooper wrote:
>> On 14/07/16 07:41, Dario Faggioli wrote:
>>> So, during domain destruction, we do:
>>>  cpupool_rm_domain()    [ in domain_destroy() ]
>>>  sched_destroy_domain() [ in complete_domain_destroy() ]
>>>
>>> Therefore, there's a window during which, from the
>>> scheduler's point of view, a domain is still there, but
>>> without it being part of any cpupool.
>>>
>>> [...]
>>>
>>> On the other hand, cpupool_rm_domain() "only" does
>>> cpupool related bookkeeping, and there's no harm
>>> postponing it a little bit.
>>>
>>> Finally, considering that, during domain initialization,
>>> we do:
>>>  cpupool_add_domain()
>>>  sched_init_domain()
>>>
>>> It looks like it makes much more sense for the domain
>>> destroy path to look like the opposite of it, i.e.:
>>>  sched_destroy_domain()
>>>  cpupool_rm_domain()
>>>
>>> This patch does that, and it's considered worth, as it
>>> fixes a bug, even if only a latent one.
>>>
>>> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>> As the cpupool bookkeeping is very closely related to the scheduler
>> bookkeeping, how about having the sched_*_domain() functions involve
>> the
>> cpupool_*_domain() functions?
>>
> That's certainly a good point.
>
> At minimum, I certainly can (and probably should have :-P) put a couple
> of ASSERT()-s in place.
>
> However, both cpupool_add_domain() and cpupool_rm_domain() are called
> only once, and I guess I can make them go into sched_init_domain() and
> sched_destroy_domain(), respectively.

I think that would be the most robust solution.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel