[PATCH] domain_validate: Check for domain address conflicts fully
Current implementation of virDomainMemoryDefCheckConflict() does only a one way comparison, i.e. if there's a memory device within def->mems[] which address falls in [mem->address, mem->address + mem->size] range (mem is basically an iterator within def->mems[]). And for static XML this works just fine. Problem is with hot/cold plugging of a memory device. Then mem points to freshly parsed memory device and these half checks are insufficient. Not only we must check whether an existing memory device doesn't clash with freshly parsed memory device, but also whether freshly parsed memory device does not fall into range of already existing memory device. Resolves: https://issues.redhat.com/browse/RHEL-4452 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index d485ec4fb1..14148a18d3 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2264,6 +2264,7 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, for (i = 0; i < def->nmems; i++) { const virDomainMemoryDef *other = def->mems[i]; unsigned long long otherStart = 0; + unsigned long long otherEnd = 0; if (other == mem) continue; @@ -2315,7 +2316,10 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, if (thisStart == 0 || otherStart == 0) continue; - if (thisStart <= otherStart && thisEnd > otherStart) { + otherEnd = otherStart + other->size; + + if ((thisStart <= otherStart && thisEnd > otherStart) || + (otherStart <= thisStart && otherEnd > thisStart)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("memory device address [0x%1$llx:0x%2$llx] overlaps with other memory device (0x%3$llx)"), thisStart, thisEnd, otherStart); -- 2.43.0
On Fri, Jan 19, 2024 at 04:25:11PM +0100, Michal Privoznik wrote:
+++ b/src/conf/domain_validate.c @@ -2315,7 +2316,10 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, if (thisStart == 0 || otherStart == 0) continue;
- if (thisStart <= otherStart && thisEnd > otherStart) { + otherEnd = otherStart + other->size;
Shouldn't you multiply other->size by 1024? That's what happens earlier with mem->size. I'm also curious about the zero check on thisStart and otherStart right above that. It looks like it would allow two overlapping memory devices to exists as long as either of them starts at zero, but I've certainly missed something in related code that makes that scenario impossible. -- Andrea Bolognani / Red Hat / Virtualization
On 1/23/24 10:45, Andrea Bolognani wrote:
On Fri, Jan 19, 2024 at 04:25:11PM +0100, Michal Privoznik wrote:
+++ b/src/conf/domain_validate.c @@ -2315,7 +2316,10 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, if (thisStart == 0 || otherStart == 0) continue;
- if (thisStart <= otherStart && thisEnd > otherStart) { + otherEnd = otherStart + other->size;
Shouldn't you multiply other->size by 1024? That's what happens earlier with mem->size.
D'oh! Of course. Nice catch.
I'm also curious about the zero check on thisStart and otherStart right above that. It looks like it would allow two overlapping memory devices to exists as long as either of them starts at zero, but I've certainly missed something in related code that makes that scenario impossible.
There are two ways in which thisStart/otherStart can be zero: 1) It's initialized to 0 and never changed => we're dealing with a memory device model that doesn't support setting addresses (e.g. SGX), or 2) it was set to zero => it's basically a not-user-set default, i.e. it's formatted into XML iff != zero (see virDomainDeviceInfoFormat(), virDomainMemoryTargetDefFormat()). IOW, this == 0 comparison checks if an address was set by user and only then check is performed. But truth to be told - we don't deny those values during parsing. They are silently ignored (and it's probably too late to change that). But I guess nobody want's to map a memory onto 0x0 anyway. Michal
On Tue, Jan 23, 2024 at 04:47:44PM +0100, Michal Prívozník wrote:
On 1/23/24 10:45, Andrea Bolognani wrote:
On Fri, Jan 19, 2024 at 04:25:11PM +0100, Michal Privoznik wrote:
+++ b/src/conf/domain_validate.c @@ -2315,7 +2316,10 @@ virDomainMemoryDefCheckConflict(const virDomainMemoryDef *mem, if (thisStart == 0 || otherStart == 0) continue;
- if (thisStart <= otherStart && thisEnd > otherStart) { + otherEnd = otherStart + other->size;
Shouldn't you multiply other->size by 1024? That's what happens earlier with mem->size.
D'oh! Of course. Nice catch.
I'm also curious about the zero check on thisStart and otherStart right above that. It looks like it would allow two overlapping memory devices to exists as long as either of them starts at zero, but I've certainly missed something in related code that makes that scenario impossible.
There are two ways in which thisStart/otherStart can be zero:
1) It's initialized to 0 and never changed => we're dealing with a memory device model that doesn't support setting addresses (e.g. SGX), or
2) it was set to zero => it's basically a not-user-set default, i.e. it's formatted into XML iff != zero (see virDomainDeviceInfoFormat(), virDomainMemoryTargetDefFormat()).
IOW, this == 0 comparison checks if an address was set by user and only then check is performed. But truth to be told - we don't deny those values during parsing. They are silently ignored (and it's probably too late to change that). But I guess nobody want's to map a memory onto 0x0 anyway.
That's convincing enough for me. Reviewed-by: Andrea Bolognani <abologna@redhat.com> with the unit conversion issue fixed. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Michal Privoznik -
Michal Prívozník