| Message ID | 20220517194113.2574-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some problems with "arm/dom0less: assign dom0less guests to cpupools" | expand |
On 17.05.22 21:41, Andrew Cooper wrote: > c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for > system domains") removed the path in domain_create() which called > sched_init_domain() with CPUPOOLID_NONE for system domains. > > Arguably, that changeset should have cleaned up this path too. > > However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > changed domain_create() from using a hardcoded poolid of 0, to using a value > passed by the toolstack. > > While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the > cpupool_id parameter and attempt to construct a real domain using default ops, > which at a minimum will fail the assertion in dom_scheduler(). > > Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
Hi Andrew, > On 17 May 2022, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for > system domains") removed the path in domain_create() which called > sched_init_domain() with CPUPOOLID_NONE for system domains. > > Arguably, that changeset should have cleaned up this path too. > > However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > changed domain_create() from using a hardcoded poolid of 0, to using a value > passed by the toolstack. > > While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the > cpupool_id parameter and attempt to construct a real domain using default ops, > which at a minimum will fail the assertion in dom_scheduler(). > > Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks for this fix, with the introduction of 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools”) we’ve checked all the path passing struct xen_domctl_createdomain, and at that time it seems to be that the new cpupool_id member would have been always zero when created from the tool stack, am I wrong? I’m asking so that I will keep in mind for the future. However with your second patch of this serie, the tool stack is able to write it, so I guess this fix now is mandatory. I’ve tested your patch, enabling boot time cpupools, on an arm machine and booting Xen+Dom0 and another DomU by dom0less feature, and all works. Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com> Cheers, Luca
On 18/05/2022 11:27, Luca Fancellu wrote: > Hi Andrew, > >> On 17 May 2022, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for >> system domains") removed the path in domain_create() which called >> sched_init_domain() with CPUPOOLID_NONE for system domains. >> >> Arguably, that changeset should have cleaned up this path too. >> >> However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") >> changed domain_create() from using a hardcoded poolid of 0, to using a value >> passed by the toolstack. >> >> While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the >> cpupool_id parameter and attempt to construct a real domain using default ops, >> which at a minimum will fail the assertion in dom_scheduler(). >> >> Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Thanks for this fix, with the introduction of 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools”) > we’ve checked all the path passing struct xen_domctl_createdomain, and at that time it seems to be that > the new cpupool_id member would have been always zero when created from the tool stack, am I wrong? Hypercalls are an entirely public API/ABI. Looking through xen.git gets you the common users, but it most definitely doesn't get you all users of the interface. This hypercall specifically gets fuzzed (there's a KFX PoC somewhere), but it's a bug for any hypercall to be able to hit an assertion/crash/etc. > I’m asking so that I will keep in mind for the future. > > However with your second patch of this serie, the tool stack is able to write it, so I guess this fix now is mandatory. > > I’ve tested your patch, enabling boot time cpupools, on an arm machine and booting Xen+Dom0 and another DomU > by dom0less feature, and all works. > > Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > Tested-by: Luca Fancellu <luca.fancellu@arm.com> Thanks. ~Andrew
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c index f6e3d97e5288..f1aa2db5f463 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -619,8 +619,6 @@ int cpupool_add_domain(struct domain *d, unsigned int poolid) int rc; int n_dom = 0; - if ( poolid == CPUPOOLID_NONE ) - return 0; spin_lock(&cpupool_lock); c = cpupool_find_by_id(poolid); if ( c == NULL )
c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for system domains") removed the path in domain_create() which called sched_init_domain() with CPUPOOLID_NONE for system domains. Arguably, that changeset should have cleaned up this path too. However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") changed domain_create() from using a hardcoded poolid of 0, to using a value passed by the toolstack. While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the cpupool_id parameter and attempt to construct a real domain using default ops, which at a minimum will fail the assertion in dom_scheduler(). Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Juergen Gross <jgross@suse.com> CC: Dario Faggioli <dfaggioli@suse.com> CC: Luca Fancellu <luca.fancellu@arm.com> --- xen/common/sched/cpupool.c | 2 -- 1 file changed, 2 deletions(-)