[libvirt] [PATCH v2 3/3] storage_conf: Resolve libvirtd crash matching scsi_host

[Ján Tomko]

On 10/06/2014 11:49 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1146837
> 
> Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6)
> which added parentaddr and unique_id to allow unique identification of
> a scsi_host, but assumed that all the pool entries and the incoming
> definition would be similarly defined. If the existing pool uses the
> 'name' attribute and an incoming pool is using the parentaddr/unique_id,
> then the code will attempt to compare the existing name string against
> the incoming name string which doesn't exist (is NULL) and results in
> a core (STREQ).
> 
> Conversely, if the existing pool used the parentaddr/unique_id and the
> to be defined pool used the name, then the comparison would be against
> the parentaddr, but since the incoming pool doesn't have one - that would
> leave the comparison against a parentaddr of all 0's and a unique_id of 0,
> which will always comparison to fail. This means someone could define the
> same source adapter for two pools
> 
> In order to resolve this, adjust the code to get the 'host#' to be used
> by the storage scsi backend in order to check/start the pool and make sure
> the incoming definition doesn't match any of the existing pool defs.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/storage_conf.c | 69 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 36696a4..19c452b 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2062,26 +2062,37 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
>      return ret;
>  }
>  
> -static bool
> -matchSCSIAdapterParent(virStoragePoolObjPtr pool,
> -                       virStoragePoolDefPtr def)
> +static int
> +getSCSIHostNumber(virStoragePoolSourceAdapter adapter,
> +                  unsigned int *hostnum)
>  {
> -    virDevicePCIAddressPtr pooladdr =
> -        &pool->def->source.adapter.data.scsi_host.parentaddr;
> -    virDevicePCIAddressPtr defaddr =
> -        &def->source.adapter.data.scsi_host.parentaddr;
> -    int pool_unique_id =
> -        pool->def->source.adapter.data.scsi_host.unique_id;
> -    int def_unique_id =
> -        def->source.adapter.data.scsi_host.unique_id;
> -    if (pooladdr->domain == defaddr->domain &&
> -        pooladdr->bus == defaddr->bus &&
> -        pooladdr->slot == defaddr->slot &&
> -        pooladdr->function == defaddr->function &&
> -        pool_unique_id == def_unique_id) {
> -        return true;
> -    }
> -    return false;
> +    int ret = -1;
> +    unsigned int num;
> +    char *name = NULL;
> +
> +    if (adapter.data.scsi_host.has_parent) {
> +        virDevicePCIAddress addr = adapter.data.scsi_host.parentaddr;
> +        unsigned int unique_id = adapter.data.scsi_host.unique_id;
> +
> +        if (!(name = virGetSCSIHostNameByParentaddr(addr.domain,
> +                                                    addr.bus,
> +                                                    addr.slot,
> +                                                    addr.function,
> +                                                    unique_id)))
> +            goto cleanup;

This still has the same problem v1 does:
https://www.redhat.com/archives/libvir-list/2014-October/msg00117.html

A valid pool definition for an existing device can be refused because another
definition, the one we already accepted, is invalid. I think this is strange
behavior and I would rather allow duplicit pools if the user went through the
trouble of bypassing our checks.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141029/7f64adcc/attachment-0001.sig>