[libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address

John Ferlan jferlan at redhat.com
Tue Sep 29 21:27:58 UTC 2015



On 09/29/2015 11:03 AM, Michal Privoznik wrote:
> On 25.09.2015 14:45, Martin Kletzander wrote:
>> On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
>>>
>>>
>>> On 09/25/2015 05:38 AM, Michal Privoznik wrote:
>>>> On 25.09.2015 11:36, Martin Kletzander wrote:
>>>>> On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1257844
>>>>>>
>>>>>> Imagine an user who is trying to attach a disk to a domain with
>>>>>> the following XML:
>>>>>>
>>>>>>  <disk type='block' device='disk'>
>>>>>>    <driver name='qemu' type='raw'/>
>>>>>>    <source dev='/dev/sr0'/>
>>>>>>    <target dev='vde' bus='virtio'/>
>>>>>>    <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>>>>>>  </disk>
>>>>>>
>>>>>> The XML is obviously wrong. It's trying to attach a virtio disk
>>>>>> onto non-PCI bus. We should forbid that.
>>>>>>
>>>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>>>> ---
>>>>>> src/qemu/qemu_hotplug.c | 7 +++++++
>>>>>> 1 file changed, 7 insertions(+)
>>>>>>
>>>>>
>>>>> How come this is not handled in qemuDomainAssignAddresses(), it
>>>>> doesn't get called?  There's a check for exactly that in
>>>>> qemuAssignDevicePCISlots().
>>>>
>>>> Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
>>>>
>>>
>>> Seems to me this may be more of a generic problem - a user providing the
>>> wrong address type for the type of device. I have a recollection of
>>
>> Yes, and since we have checks for those, it's confusing to me why
>> would qemuAssignDevicePCISlots() be called only with --config.  Can we
>> use that code which checks for more things already?  For example, the
>> here-missing virtio-mmio.
> 
> Yes and no. qemuDomainAssignAddresses() expects to see vm->def which
> already has the device attached. Moreover, it not only works over all
> devices in the domain (filling in all the missing addresses), but
> possibly creating new controllers too (e.g. plugging new pci bridges if
> running out of addresses on a bus).
> 
> Does anybody have a bright idea how this can be fixed apart from obvious
> one - breaking whole address assignment code into parts and reassembling
> it back together again?
> 

>From the v4 from Ruifeng Beng:

http://www.redhat.com/archives/libvir-list/2015-September/msg00524.html

which has some validity w/r/t using qemuCheckDiskConfig; however, I'm
wondering now if that's far too late in processing. Is it agreeable that
the checks in virDomainDeviceAddressTypeIsValid will cover the cases
when the incoming device has an address in the XML?

Why not use the power of virDomainDeviceDefPostParseInternal? There's a
check in that code:

        if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
             virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0)
             return -1;

why not make an "else" which says, let's be sure the disk->info.type
provided is valid?

I've attached a patch which works for the cases shown in the bz (both no
params and --config).

This would solve attach, update, and detach since each would call the
virDomainDeviceDefParse which calls the PostParse code.

John

NOTE: The change to the test is because the failure now occurs during
parse rather than at run (e.g. earlier, where I think it should).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-bug-1257844.patch
Type: text/x-patch
Size: 3411 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150929/de454e20/attachment-0001.bin>


More information about the libvir-list mailing list