| Message ID | 20220419135254.21729-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fix and cleanup domctl handling | expand |
On 19.04.2022 15:52, Juergen Gross wrote: > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -871,6 +871,13 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > copyback = 1; > break; > > + case XEN_DOMCTL_assign_device: > + case XEN_DOMCTL_test_assign_device: > + case XEN_DOMCTL_deassign_device: > + case XEN_DOMCTL_get_device_group: As said in reply to Andrew, I'm not convinced having these enumerated here is a good idea. But in any event the whole addition wants framing by #ifdef CONFIG_HAS_PASSTHROUGH now. For x86 I wonder whether the adjustment wouldn't allow to drop domctl.c's including of xen/iommu.h (but perhaps it's included indirectly anyway). Jan > + ret = iommu_do_domctl(op, d, u_domctl); > + break; > + > default: > ret = arch_do_domctl(op, d, u_domctl); > break;
On 19/04/2022 14:52, Juergen Gross wrote: > Today iommu_do_domctl() is being called from arch_do_domctl() in the > "default:" case of a switch statement. This has led already to crashes > due to unvalidated parameters. > > Fix that by moving the call of iommu_do_domctl() to the main switch > statement of do_domctl(). > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > Another possibility would even be to merge iommu_do_domctl() completely > into do_domctl(), but I wanted to start with a less intrusive variant. > V3: > - new patch I definitely prefer this approach, thanks. In addition to being clearer, it's also faster because there isn't a long line of "do you understand this subop?" calls when we know exactly what it is. However, I think we need stub for the !HAS_PASSTHROUGH case now that it is being called from common code. I'd forgotten that it was used on ARM now, and yes - it absolutely should be called from somewhere common, not from the arch hooks. ~Andrew
On 19.04.22 16:51, Andrew Cooper wrote: > On 19/04/2022 14:52, Juergen Gross wrote: >> Today iommu_do_domctl() is being called from arch_do_domctl() in the >> "default:" case of a switch statement. This has led already to crashes >> due to unvalidated parameters. >> >> Fix that by moving the call of iommu_do_domctl() to the main switch >> statement of do_domctl(). >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> Another possibility would even be to merge iommu_do_domctl() completely >> into do_domctl(), but I wanted to start with a less intrusive variant. >> V3: >> - new patch > > I definitely prefer this approach, thanks. In addition to being > clearer, it's also faster because there isn't a long line of "do you > understand this subop?" calls when we know exactly what it is. > > However, I think we need stub for the !HAS_PASSTHROUGH case now that it > is being called from common code. Okay, I'll add it. Jan, are you fine with a stub? Juergen
On 19.04.2022 16:56, Juergen Gross wrote: > On 19.04.22 16:51, Andrew Cooper wrote: >> On 19/04/2022 14:52, Juergen Gross wrote: >>> Today iommu_do_domctl() is being called from arch_do_domctl() in the >>> "default:" case of a switch statement. This has led already to crashes >>> due to unvalidated parameters. >>> >>> Fix that by moving the call of iommu_do_domctl() to the main switch >>> statement of do_domctl(). >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> Another possibility would even be to merge iommu_do_domctl() completely >>> into do_domctl(), but I wanted to start with a less intrusive variant. >>> V3: >>> - new patch >> >> I definitely prefer this approach, thanks. In addition to being >> clearer, it's also faster because there isn't a long line of "do you >> understand this subop?" calls when we know exactly what it is. >> >> However, I think we need stub for the !HAS_PASSTHROUGH case now that it >> is being called from common code. > > Okay, I'll add it. Jan, are you fine with a stub? Sure. Jan
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c index 6245af6d0b..1baf25c3d9 100644 --- a/xen/arch/arm/domctl.c +++ b/xen/arch/arm/domctl.c @@ -176,16 +176,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d, return rc; } default: - { - int rc; - - rc = subarch_do_domctl(domctl, d, u_domctl); - - if ( rc == -ENOSYS ) - rc = iommu_do_domctl(domctl, d, u_domctl); - - return rc; - } + return subarch_do_domctl(domctl, d, u_domctl); } } diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index a6aae500a3..c9699bb868 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1380,7 +1380,7 @@ long arch_do_domctl( break; default: - ret = iommu_do_domctl(domctl, d, u_domctl); + ret = -ENOSYS; break; } diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 5879117580..0a866e3132 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -871,6 +871,13 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) copyback = 1; break; + case XEN_DOMCTL_assign_device: + case XEN_DOMCTL_test_assign_device: + case XEN_DOMCTL_deassign_device: + case XEN_DOMCTL_get_device_group: + ret = iommu_do_domctl(op, d, u_domctl); + break; + default: ret = arch_do_domctl(op, d, u_domctl); break;
Today iommu_do_domctl() is being called from arch_do_domctl() in the "default:" case of a switch statement. This has led already to crashes due to unvalidated parameters. Fix that by moving the call of iommu_do_domctl() to the main switch statement of do_domctl(). Signed-off-by: Juergen Gross <jgross@suse.com> --- Another possibility would even be to merge iommu_do_domctl() completely into do_domctl(), but I wanted to start with a less intrusive variant. V3: - new patch --- xen/arch/arm/domctl.c | 11 +---------- xen/arch/x86/domctl.c | 2 +- xen/common/domctl.c | 7 +++++++ 3 files changed, 9 insertions(+), 11 deletions(-)