domain_validate: Check for domain address conflicts fully

[PATCH] domain_validate: Check for domain address conflicts fully

Posted by Michal Privoznik 3 months, 1 week ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH] domain_validate: Check for domain address conflicts fully

Posted by Andrea Bolognani 3 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH] domain_validate: Check for domain address conflicts fully

Posted by Michal Prívozník 3 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: Re: [PATCH] domain_validate: Check for domain address conflicts fully

Posted by Andrea Bolognani 3 months ago

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org