[libvirt PATCH 2/2] conf: log error on attempts to modify ACPI index of active device

[Laine Stump]

On 9/10/21 3:30 AM, Peter Krempa wrote:
> On Thu, Sep 09, 2021 at 13:46:41 -0400, Laine Stump wrote:
>> The ACPI index of a device in a running guest can't be modified, and
>> libvirt doesn't actually attempt to modify it, but it was possible for
>> a user to request such a modification, and libvirt wouldn't complain,
>> thus misleading the user into thinking that it had actually been changed.
>>
>> Resolves: https://bugzilla.redhat.com/1998920
>>
>> Signed-off-by: Laine Stump <laine at redhat.com>
>> ---
>>   src/conf/domain_conf.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index fefc527901..7ff890d8b7 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -28485,6 +28485,12 @@ virDomainDefCompatibleDevice(virDomainDef *def,
>>                              _("changing device alias is not allowed"));
>>               return -1;
>>           }
>> +
>> +        if (data.newInfo->acpiIndex != data.oldInfo->acpiIndex) {
>> +            virReportError(VIR_ERR_OPERATION_DENIED, "%s",
>> +                           _("changing device 'acpi index' is not allowed"));
>> +            return -1;
>> +        }
> 
> I don't remember any more what the intended semantics of the 'update'
> API are in regards to asking the user to provide a complete definition
> of the device, but according to the 'alias' check above we seem to be
> specifically ignoring the situation when the new definition didn't
> provide an alias. This seems to be hinting that we ignore stuff that the
> used didn't provide.
> 
> If that's the case you'll need to specifically exclude index '0' from
> the above check.

The difference here is that alias is something that normally is not set 
by the user, but is instead auto-generated by libvirt when the guest 
starts and not stored/reported in the persistent config. So in the 
common case of "grab config XML, extract device element, modify it, send 
it to update-device" the alias wouldn't be in the XML, but if the user 
instead grabs the *status* XML then the alias *would* be there. 
(However, if the alias had been set by the user (with the prefix "ua-"), 
the alias would be present in the config XML.) So we allow for either 
blank (in the case that config XML was used) or no-change (in case alias 
was user-set and/or the status XML is used).

The acpi index, on the other hand, is always unset unless the user 
specifies it, and so is never auto-generated and always matches in both 
the config and status XMLs. So whichever way the user gets the original 
XML (status or config) they are always going to have the actual setting.

There are many other attributes that are checked to assure they match in 
(e.g.) qemuDomainChangeNet() (for example, network device model name, 
virtio options, upscript name,...) and (with one exception) none of 
those check for "not specified or not changed" - they all just check for 
"not changed". The one exception is ifname, which is another attribute 
that (like alias) is autogenerated every time the guest starts, and is 
never output in config XML (unless it was user-specified); for ifname if 
it's been left unspecified in the update XML, then the value from the 
status is copied over, making it also effectively ("unspecified or 
unchanged"). But again, this special handling isn't done for any 
attribute that isn't both auto-generated and ephemeral (as alias and 
ifname are).

So in the end, I think it's correct that validation of acpi index 
*shouldn't* ignore an "apparently missing" value in the update.

======

BTW, something forgot to mention in the commit log - in the case of, 
e.g., network interfaces which is the only type of device where an alias 
is useful, most of the checks are in qemuDomainChangeNet() rather than 
in this virDomainDefCompatibleDevice() function. I was originally going 
to add this check into qemuDomainChangeNet() as well, but it is an 
attribute that's technically valid for any PCI device, not just 
interfaces; searching around, I found that 
virDomainDefCompatibleDevice() is called on update for all devices, and 
is already used to pretect against alias change (another attribute 
that's valid for all devices), so in the interest of consistency I put 
the check there.

That said, although I used it based on precendent, I can't say that I 
really like virDomainDefCompatibleDevice(). It is used for multiple 
unrelated things - the bit at the beginning of the function is only 
executed if action is UPDATE and live is true, and all the rest of the 
function is only useful if action is ATTACH - it's checking things that 
could verifiably never happen during a live device update (e.g. checking 
if the device is USB but the domain doesn't support USB devices, or 
checking attributes for a device type that isn't allowed to be updated 
at all.). So it's really 2 completely independent functions in one, 
where the final two args act as an extension to the function name.

I notice its action and live args previously weren't there - I did the 
spelunking into git history to find when/how; essentially there was at 
one time an unused action arg which was removed, but then re-added in 
order to add the check for alias). I didn't dig much deeper, but it 
seems like this might be an unnecessary tangle that should be split up 
into multiple functions. I didn't want to mess with that for a simple 
bugfix, though.