docs/man/xl.cfg.5.pod.in | 6 ++++++ docs/man/xl.conf.5.pod.in | 6 ++++++ tools/helpers/init-xenstore-domain.c | 2 ++ tools/include/libxl.h | 7 +++++++ tools/libs/light/libxl_create.c | 23 +++++++++++++++++++++++ tools/libs/light/libxl_dm.c | 1 + tools/libs/light/libxl_types.idl | 1 + tools/ocaml/libs/xc/xenctrl.ml | 1 + tools/ocaml/libs/xc/xenctrl.mli | 1 + tools/ocaml/libs/xc/xenctrl_stubs.c | 5 ++++- tools/xl/xl.c | 8 ++++++++ tools/xl/xl.h | 1 + tools/xl/xl_parse.c | 9 +++++++++ xen/arch/arm/domain_build.c | 2 ++ xen/arch/x86/setup.c | 1 + xen/common/domain.c | 3 ++- xen/common/grant_table.c | 21 +++++++++++++++++++-- xen/include/public/domctl.h | 5 +++++ xen/include/xen/grant_table.h | 5 +++-- 19 files changed, 102 insertions(+), 6 deletions(-)
Introduce a new domain create field so that toolstack can specify the
maximum grant table version usable by the domain. This is plumbed into
xl and settable by the user as max_grant_version.
Previously this was only settable on a per host basis using the
gnttab command line option.
Note the version is specified using 4 bits, which leaves room to
specify up to grant table version 15. Given that we only have 2 grant
table versions right now, and a new version is unlikely in the near
future using 4 bits seems more than enough.
xenstored stubdomains are limited to grant table v1 because the
current MiniOS code used to build them only has support for grants v1.
There are existing limits set for xenstored stubdomains at creation
time that already match the defaults in MiniOS.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
This needs to be applied on top of Andrew's:
xen: Report grant table v1/v2 capabilities to the toolstack
https://lore.kernel.org/xen-devel/20211029173813.23002-1-andrew.cooper3@citrix.com/
NB: the stubdom max grant version is cloned from the domain one. Not
sure whether long term we might want to use different options for the
stubdom and the domain. In any case the attack surface will always be
max(stubdom, domain), so maybe it's just pointless to allow more fine
grained settings.
---
Changes since v4:
- Remove the check for 16TB: there's no check currently, and there's
some discussion about how to implement it properly for all guest
types.
- Place the logic to pick the default version in the toolstack, so
the create domain domctl will always specify a grant table version.
Such version will also be part of the migration stream.
Changes since v3:
- Expand commit message re xenstored stubdomains.
Changes since v2:
- Drop XEN_DOMCTLGRANT_MAX - it's unused.
- Rename max_grant_version field to max_version in the grant table
struct.
- Print domain on log messages.
- Print a message if host has more than 16Tb of RAM and grant v2 is
disabled.
- Add a TB macro.
Changes since v1:
- Introduce a grant_opts field and use the low 4 bits to specify the
version. Remaining bits will be used for other purposes.
---
Cc: Ian Jackson <iwj@xenproject.org>
---
Posting this patch alone as I think allowing to control transient
grants on a per-domain basis will require a bit more of work.
Release rationale:
We have had a bunch of security issues involving grant table v2 (382,
379, 268, 255) which could have been avoided by limiting the grant
table version available to guests. This can be currently done using a
global host parameter, but it's certainly more helpful to be able to
do it on a per domain basis from the toolstack.
Changes to the hypervisor by this patch are fairly minimal, as there
are already checks for the max grant table version allowed, so the
main change there is moving the max grant table version limit inside
the domain struct and plumbing it through the toolstrack.
I think the risk here is quite low for libxl/xl, because it's
extensively tested by osstest, so the main risk would be breaking the
Ocaml stubs, which could go unnoticed as those are not actually tested
by osstest.
---
docs/man/xl.cfg.5.pod.in | 6 ++++++
docs/man/xl.conf.5.pod.in | 6 ++++++
tools/helpers/init-xenstore-domain.c | 2 ++
tools/include/libxl.h | 7 +++++++
tools/libs/light/libxl_create.c | 23 +++++++++++++++++++++++
tools/libs/light/libxl_dm.c | 1 +
tools/libs/light/libxl_types.idl | 1 +
tools/ocaml/libs/xc/xenctrl.ml | 1 +
tools/ocaml/libs/xc/xenctrl.mli | 1 +
tools/ocaml/libs/xc/xenctrl_stubs.c | 5 ++++-
tools/xl/xl.c | 8 ++++++++
tools/xl/xl.h | 1 +
tools/xl/xl_parse.c | 9 +++++++++
xen/arch/arm/domain_build.c | 2 ++
xen/arch/x86/setup.c | 1 +
xen/common/domain.c | 3 ++-
xen/common/grant_table.c | 21 +++++++++++++++++++--
xen/include/public/domctl.h | 5 +++++
xen/include/xen/grant_table.h | 5 +++--
19 files changed, 102 insertions(+), 6 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 55c4881205..21a39adb70 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -580,6 +580,12 @@ to have. This value controls how many pages of foreign domains can be accessed
via the grant mechanism by this domain. The default value is settable via
L<xl.conf(5)>.
+=item B<max_grant_version=NUMBER>
+
+Specify the maximum grant table version the domain is allowed to use. Current
+supported versions are 1 and 2. The default value is settable via
+L<xl.conf(5)>.
+
=item B<nomigrate=BOOLEAN>
Disable migration of this domain. This enables certain other features
diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
index b48e99131a..df20c08137 100644
--- a/docs/man/xl.conf.5.pod.in
+++ b/docs/man/xl.conf.5.pod.in
@@ -101,6 +101,12 @@ Sets the default value for the C<max_maptrack_frames> domain config value.
Default: value of Xen command line B<gnttab_max_maptrack_frames>
parameter (or its default value if unspecified).
+=item B<max_grant_version=NUMBER>
+
+Sets the default value for the C<max_grant_version> domain config value.
+
+Default: maximum grant version supported by the hypervisor.
+
=item B<vif.default.script="PATH">
Configures the default hotplug script used by virtual network devices.
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 6836002f0b..41a7c38ada 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -85,9 +85,11 @@ static int build(xc_interface *xch)
* 1 grant frame is enough: we don't need many grants.
* Mini-OS doesn't like less than 4, though, so use 4.
* 128 maptrack frames: 256 entries per frame, enough for 32768 domains.
+ * Currently Mini-OS only supports grant v1.
*/
.max_grant_frames = 4,
.max_maptrack_frames = 128,
+ .grant_opts = 1,
};
xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 54c10f6efe..2bbbd21f0b 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -520,6 +520,13 @@
*/
#define LIBXL_HAVE_PHYSINFO_CAP_GNTTAB 1
+/*
+ * LIBXL_HAVE_MAX_GRANT_VERSION indicates libxl_domain_build_info has a
+ * max_grant_version field for setting the max grant table version per
+ * domain.
+ */
+#define LIBXL_HAVE_MAX_GRANT_VERSION 1
+
/*
* libxl ABI compatibility
*
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 5a61d01722..b6855c7b46 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -454,6 +454,28 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->nested_hvm, false);
}
+ if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
+ libxl_physinfo info;
+
+ rc = libxl_get_physinfo(CTX, &info);
+ if (rc) {
+ LOG(ERROR, "failed to get hypervisor info");
+ return rc;
+ }
+
+ if (info.cap_gnttab_v2)
+ b_info->max_grant_version = 2;
+ else if (info.cap_gnttab_v1)
+ b_info->max_grant_version = 1;
+ else
+ /* No grant table support reported */
+ b_info->max_grant_version = 0;
+ } else if (b_info->max_grant_version & ~XEN_DOMCTL_GRANT_version_mask) {
+ LOG(ERROR, "max grant version %d out of range",
+ b_info->max_grant_version);
+ return -ERROR_INVAL;
+ }
+
return 0;
}
@@ -607,6 +629,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
.max_evtchn_port = b_info->event_channels,
.max_grant_frames = b_info->max_grant_frames,
.max_maptrack_frames = b_info->max_maptrack_frames,
+ .grant_opts = b_info->max_grant_version,
.vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
};
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 9d93056b5c..9a8ddbe188 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2320,6 +2320,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
dm_config->b_info.max_grant_frames = guest_config->b_info.max_grant_frames;
dm_config->b_info.max_maptrack_frames = guest_config->b_info.max_maptrack_frames;
+ dm_config->b_info.max_grant_version = guest_config->b_info.max_grant_version;
dm_config->b_info.u.pv.features = "";
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 573bba68ee..2a42da2f7d 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -519,6 +519,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("max_grant_frames", uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
+ ("max_grant_version", integer, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
("device_model_version", libxl_device_model_version),
("device_model_stubdomain", libxl_defbool),
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index ed2924a2b3..7503031d8f 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -84,6 +84,7 @@ type domctl_create_config =
max_evtchn_port: int;
max_grant_frames: int;
max_maptrack_frames: int;
+ max_grant_version: int;
arch: arch_domainconfig;
}
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index d20dc0108d..d1d9c9247a 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -76,6 +76,7 @@ type domctl_create_config = {
max_evtchn_port: int;
max_grant_frames: int;
max_maptrack_frames: int;
+ max_grant_version: int;
arch: arch_domainconfig;
}
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index ad953d36bd..eca0b8b334 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -188,7 +188,8 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
#define VAL_MAX_EVTCHN_PORT Field(config, 5)
#define VAL_MAX_GRANT_FRAMES Field(config, 6)
#define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
-#define VAL_ARCH Field(config, 8)
+#define VAL_MAX_GRANT_VERSION Field(config, 8)
+#define VAL_ARCH Field(config, 9)
uint32_t domid = Int_val(wanted_domid);
int result;
@@ -198,6 +199,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
.max_evtchn_port = Int_val(VAL_MAX_EVTCHN_PORT),
.max_grant_frames = Int_val(VAL_MAX_GRANT_FRAMES),
.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
+ .grant_opts = Int_val(VAL_MAX_GRANT_VERSION),
};
domain_handle_of_uuid_string(cfg.handle, String_val(VAL_HANDLE));
@@ -251,6 +253,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
}
#undef VAL_ARCH
+#undef VAL_MAX_GRANT_VERSION
#undef VAL_MAX_MAPTRACK_FRAMES
#undef VAL_MAX_GRANT_FRAMES
#undef VAL_MAX_EVTCHN_PORT
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index f422f9fed5..2d1ec18ea3 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -55,6 +55,7 @@ bool progress_use_cr = 0;
bool timestamps = 0;
int max_grant_frames = -1;
int max_maptrack_frames = -1;
+int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
libxl_domid domid_policy = INVALID_DOMID;
xentoollog_level minmsglevel = minmsglevel_default;
@@ -219,6 +220,13 @@ static void parse_global_config(const char *configfile,
else if (e != ESRCH)
exit(1);
+ e = xlu_cfg_get_bounded_long (config, "max_grant_version", 0,
+ INT_MAX, &l, 1);
+ if (!e)
+ max_grant_version = l;
+ else if (e != ESRCH)
+ exit(1);
+
libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0);
libxl_cpu_bitmap_alloc(ctx, &global_hvm_affinity_mask, 0);
libxl_cpu_bitmap_alloc(ctx, &global_pv_affinity_mask, 0);
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 7e23f30192..cf12c79a9b 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -282,6 +282,7 @@ extern char *default_colo_proxy_script;
extern char *blkdev_start;
extern int max_grant_frames;
extern int max_maptrack_frames;
+extern int max_grant_version;
extern libxl_bitmap global_vm_affinity_mask;
extern libxl_bitmap global_hvm_affinity_mask;
extern libxl_bitmap global_pv_affinity_mask;
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index c503b9be00..117fcdcb2b 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1431,6 +1431,15 @@ void parse_config_data(const char *config_source,
else
exit(1);
+ e = xlu_cfg_get_bounded_long (config, "max_grant_version", 0,
+ INT_MAX, &l, 1);
+ if (e == ESRCH) /* not specified */
+ b_info->max_grant_version = max_grant_version;
+ else if (!e)
+ b_info->max_grant_version = l;
+ else
+ exit(1);
+
libxl_defbool_set(&b_info->claim_mode, claim_mode);
if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 0167731ab0..9e92b640cd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2967,6 +2967,7 @@ void __init create_domUs(void)
.max_evtchn_port = -1,
.max_grant_frames = -1,
.max_maptrack_frames = -1,
+ .grant_opts = opt_gnttab_max_version,
};
if ( !dt_device_is_compatible(node, "xen,domain") )
@@ -3074,6 +3075,7 @@ void __init create_dom0(void)
.max_evtchn_port = -1,
.max_grant_frames = gnttab_dom0_frames(),
.max_maptrack_frames = -1,
+ .grant_opts = opt_gnttab_max_version,
};
/* The vGIC for DOM0 is exactly emulating the hardware GIC */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b101565f14..26fee5d9fb 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -750,6 +750,7 @@ static struct domain *__init create_dom0(const module_t *image,
.max_evtchn_port = -1,
.max_grant_frames = -1,
.max_maptrack_frames = -1,
+ .grant_opts = opt_gnttab_max_version,
.max_vcpus = dom0_max_vcpus(),
.arch = {
.misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8b53c49d1e..0c7052c770 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -678,7 +678,8 @@ struct domain *domain_create(domid_t domid,
init_status |= INIT_evtchn;
if ( (err = grant_table_init(d, config->max_grant_frames,
- config->max_maptrack_frames)) != 0 )
+ config->max_maptrack_frames,
+ config->grant_opts)) != 0 )
goto fail;
init_status |= INIT_gnttab;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a20319b22a..8b322b51c0 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -53,6 +53,7 @@ struct grant_table {
percpu_rwlock_t lock;
/* Lock protecting the maptrack limit */
spinlock_t maptrack_lock;
+ unsigned int max_version;
/*
* Defaults to v1. May be changed with GNTTABOP_set_version. All other
* values are invalid.
@@ -1917,11 +1918,26 @@ active_alloc_failed:
}
int grant_table_init(struct domain *d, int max_grant_frames,
- int max_maptrack_frames)
+ int max_maptrack_frames, unsigned int options)
{
struct grant_table *gt;
+ unsigned int max_grant_version = options & XEN_DOMCTL_GRANT_version_mask;
int ret = -ENOMEM;
+ if ( !max_grant_version )
+ {
+ dprintk(XENLOG_INFO, "%pd: invalid grant table version 0 requested\n",
+ d);
+ return -EINVAL;
+ }
+ if ( max_grant_version > opt_gnttab_max_version )
+ {
+ dprintk(XENLOG_INFO,
+ "%pd: requested grant version (%u) greater than supported (%u)\n",
+ d, max_grant_version, opt_gnttab_max_version);
+ return -EINVAL;
+ }
+
/* Default to maximum value if no value was specified */
if ( max_grant_frames < 0 )
max_grant_frames = opt_max_grant_frames;
@@ -1947,6 +1963,7 @@ int grant_table_init(struct domain *d, int max_grant_frames,
gt->gt_version = 1;
gt->max_grant_frames = max_grant_frames;
gt->max_maptrack_frames = max_maptrack_frames;
+ gt->max_version = max_grant_version;
/* Install the structure early to simplify the error path. */
gt->domain = d;
@@ -3076,7 +3093,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
goto out;
res = -ENOSYS;
- if ( op.version == 2 && opt_gnttab_max_version == 1 )
+ if ( op.version == 2 && gt->max_version == 1 )
goto out; /* Behave as before set_version was introduced. */
res = 0;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 51017b47bc..1c21d4dc75 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -97,6 +97,11 @@ struct xen_domctl_createdomain {
int32_t max_grant_frames;
int32_t max_maptrack_frames;
+/* Grant version, use low 4 bits. */
+#define XEN_DOMCTL_GRANT_version_mask 0xf
+
+ uint32_t grant_opts;
+
/* Per-vCPU buffer size in bytes. 0 to disable. */
uint32_t vmtrace_size;
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 9ee830cfd0..f79c866bd9 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -37,7 +37,7 @@ extern unsigned int opt_max_grant_frames;
/* Create/destroy per-domain grant table context. */
int grant_table_init(struct domain *d, int max_grant_frames,
- int max_maptrack_frames);
+ int max_maptrack_frames, unsigned int options);
void grant_table_destroy(
struct domain *d);
void grant_table_init_vcpu(struct vcpu *v);
@@ -69,7 +69,8 @@ int gnttab_acquire_resource(
static inline int grant_table_init(struct domain *d,
int max_grant_frames,
- int max_maptrack_frames)
+ int max_maptrack_frames,
+ unsigned int options)
{
return 0;
}
--
2.33.0
On 03.11.2021 15:57, Roger Pau Monne wrote:
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -454,6 +454,28 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl_defbool_setdefault(&b_info->nested_hvm, false);
> }
>
> + if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> + libxl_physinfo info;
> +
> + rc = libxl_get_physinfo(CTX, &info);
> + if (rc) {
> + LOG(ERROR, "failed to get hypervisor info");
> + return rc;
> + }
> +
> + if (info.cap_gnttab_v2)
> + b_info->max_grant_version = 2;
> + else if (info.cap_gnttab_v1)
> + b_info->max_grant_version = 1;
> + else
> + /* No grant table support reported */
> + b_info->max_grant_version = 0;
> + } else if (b_info->max_grant_version & ~XEN_DOMCTL_GRANT_version_mask) {
Aren't you introducing a dependency of a stable library on an unstable
interface here? I'm surprised this even builds, as I didn't expect
libxl sources to include domctl.h in the first place.
> @@ -219,6 +220,13 @@ static void parse_global_config(const char *configfile,
> else if (e != ESRCH)
> exit(1);
>
> + e = xlu_cfg_get_bounded_long (config, "max_grant_version", 0,
> + INT_MAX, &l, 1);
> + if (!e)
> + max_grant_version = l;
> + else if (e != ESRCH)
> + exit(1);
Would be kind of nice if out-of-range values were detected and
reported right here, rather than causing puzzling errors at domain
creation. But I have no idea whether doing so would be inconsistent
with the processing of other global settings.
> @@ -1917,11 +1918,26 @@ active_alloc_failed:
> }
>
> int grant_table_init(struct domain *d, int max_grant_frames,
> - int max_maptrack_frames)
> + int max_maptrack_frames, unsigned int options)
> {
> struct grant_table *gt;
> + unsigned int max_grant_version = options & XEN_DOMCTL_GRANT_version_mask;
> int ret = -ENOMEM;
>
> + if ( !max_grant_version )
> + {
> + dprintk(XENLOG_INFO, "%pd: invalid grant table version 0 requested\n",
> + d);
> + return -EINVAL;
> + }
Wouldn't 0 rather be the most natural way to request no gnttab at all
for a domain? (Arguably making things work that way could be left to
a future change.)
> + if ( max_grant_version > opt_gnttab_max_version )
> + {
> + dprintk(XENLOG_INFO,
> + "%pd: requested grant version (%u) greater than supported (%u)\n",
> + d, max_grant_version, opt_gnttab_max_version);
> + return -EINVAL;
> + }
> +
> /* Default to maximum value if no value was specified */
> if ( max_grant_frames < 0 )
> max_grant_frames = opt_max_grant_frames;
Neither here nor in domain_create() you check that the other bits of
"options" are zero.
> @@ -69,7 +69,8 @@ int gnttab_acquire_resource(
>
> static inline int grant_table_init(struct domain *d,
> int max_grant_frames,
> - int max_maptrack_frames)
> + int max_maptrack_frames,
> + unsigned int options)
> {
> return 0;
> }
While arbitrary table size requests may be okay to ignore here, I'm
unsure about the max-version: Requesting v3 is surely an error in any
event; I'd even be inclined to suggest requesting v1 or v2 is as
well. Adding such a check here looks like it would be compatible with
all the other adjustments you're making.
Jan
On Thu, Nov 04, 2021 at 09:55:03AM +0100, Jan Beulich wrote:
> On 03.11.2021 15:57, Roger Pau Monne wrote:
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -454,6 +454,28 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> > libxl_defbool_setdefault(&b_info->nested_hvm, false);
> > }
> >
> > + if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> > + libxl_physinfo info;
> > +
> > + rc = libxl_get_physinfo(CTX, &info);
> > + if (rc) {
> > + LOG(ERROR, "failed to get hypervisor info");
> > + return rc;
> > + }
> > +
> > + if (info.cap_gnttab_v2)
> > + b_info->max_grant_version = 2;
> > + else if (info.cap_gnttab_v1)
> > + b_info->max_grant_version = 1;
> > + else
> > + /* No grant table support reported */
> > + b_info->max_grant_version = 0;
> > + } else if (b_info->max_grant_version & ~XEN_DOMCTL_GRANT_version_mask) {
>
> Aren't you introducing a dependency of a stable library on an unstable
> interface here? I'm surprised this even builds, as I didn't expect
> libxl sources to include domctl.h in the first place.
libxl API is stable, but not it's ABI, and libxl strictly depends and
uses domctl. See for example libxl__domain_make and it's usage of
xen_domctl_createdomain.
I think the usage here of XEN_DOMCTL_GRANT_version_mask is fine.
> > @@ -219,6 +220,13 @@ static void parse_global_config(const char *configfile,
> > else if (e != ESRCH)
> > exit(1);
> >
> > + e = xlu_cfg_get_bounded_long (config, "max_grant_version", 0,
> > + INT_MAX, &l, 1);
> > + if (!e)
> > + max_grant_version = l;
> > + else if (e != ESRCH)
> > + exit(1);
>
> Would be kind of nice if out-of-range values were detected and
> reported right here, rather than causing puzzling errors at domain
> creation. But I have no idea whether doing so would be inconsistent
> with the processing of other global settings.
Hm, it could be done but doesn't seem to be common. AFAICT
parse_global_config just reads the values from the config file,
whether those values are sane doesn't seem to be implemented there.
> > @@ -1917,11 +1918,26 @@ active_alloc_failed:
> > }
> >
> > int grant_table_init(struct domain *d, int max_grant_frames,
> > - int max_maptrack_frames)
> > + int max_maptrack_frames, unsigned int options)
> > {
> > struct grant_table *gt;
> > + unsigned int max_grant_version = options & XEN_DOMCTL_GRANT_version_mask;
> > int ret = -ENOMEM;
> >
> > + if ( !max_grant_version )
> > + {
> > + dprintk(XENLOG_INFO, "%pd: invalid grant table version 0 requested\n",
> > + d);
> > + return -EINVAL;
> > + }
>
> Wouldn't 0 rather be the most natural way to request no gnttab at all
> for a domain? (Arguably making things work that way could be left to
> a future change.)
Indeed, but that's not part of this change. I will post an updated
version of the grant disabling patch separately, as I don't think
that's 4.16 material.
> > + if ( max_grant_version > opt_gnttab_max_version )
> > + {
> > + dprintk(XENLOG_INFO,
> > + "%pd: requested grant version (%u) greater than supported (%u)\n",
> > + d, max_grant_version, opt_gnttab_max_version);
> > + return -EINVAL;
> > + }
> > +
> > /* Default to maximum value if no value was specified */
> > if ( max_grant_frames < 0 )
> > max_grant_frames = opt_max_grant_frames;
>
> Neither here nor in domain_create() you check that the other bits of
> "options" are zero.
Right, will add.
> > @@ -69,7 +69,8 @@ int gnttab_acquire_resource(
> >
> > static inline int grant_table_init(struct domain *d,
> > int max_grant_frames,
> > - int max_maptrack_frames)
> > + int max_maptrack_frames,
> > + unsigned int options)
> > {
> > return 0;
> > }
>
> While arbitrary table size requests may be okay to ignore here, I'm
> unsure about the max-version: Requesting v3 is surely an error in any
> event; I'd even be inclined to suggest requesting v1 or v2 is as
> well. Adding such a check here looks like it would be compatible with
> all the other adjustments you're making.
I think if the grant table is disabled at build time we should only
accept version 0 as valid here.
Thanks, Roger.
On 04.11.2021 10:16, Roger Pau Monné wrote:
> On Thu, Nov 04, 2021 at 09:55:03AM +0100, Jan Beulich wrote:
>> On 03.11.2021 15:57, Roger Pau Monne wrote:
>>> @@ -69,7 +69,8 @@ int gnttab_acquire_resource(
>>>
>>> static inline int grant_table_init(struct domain *d,
>>> int max_grant_frames,
>>> - int max_maptrack_frames)
>>> + int max_maptrack_frames,
>>> + unsigned int options)
>>> {
>>> return 0;
>>> }
>>
>> While arbitrary table size requests may be okay to ignore here, I'm
>> unsure about the max-version: Requesting v3 is surely an error in any
>> event; I'd even be inclined to suggest requesting v1 or v2 is as
>> well. Adding such a check here looks like it would be compatible with
>> all the other adjustments you're making.
>
> I think if the grant table is disabled at build time we should only
> accept version 0 as valid here.
IOW "options" as a whole then wants to be checked against zero.
Jan
© 2016 - 2024 Red Hat, Inc.