[PATCH] qemu: Don't crash when getting targets for a multipath

Michal Privoznik posted 1 patch 3 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/faecc9b02ddaa3b5dc41d05e7d94864b346a4081.1584634957.git.mprivozn@redhat.com
src/qemu/qemu_domain.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

[PATCH] qemu: Don't crash when getting targets for a multipath

Posted by Michal Privoznik 3 weeks ago
In one of my previous commits I've introduced code that creates
all devices for given (possible) multipath target. But I've made
a mistake there - the code accesses src->path without checking if
the disk source is local. Note that the path is NULL if the
source is not local.

Fixes: a30078cb832646177defd256e77c632905f1e6d0
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0e2252f6cf..ef17829235 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15862,19 +15862,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
             }
 
             tmpPath = g_strdup(next->path);
+
+            if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
+                errno != ENOSYS && errno != EBADF) {
+                virReportSystemError(errno,
+                                     _("Unable to get devmapper targets for %s"),
+                                     next->path);
+                return -1;
+            }
         }
 
         if (virStringListAdd(&paths, tmpPath) < 0)
             return -1;
 
-        if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
-            errno != ENOSYS && errno != EBADF) {
-            virReportSystemError(errno,
-                                 _("Unable to get devmapper targets for %s"),
-                                 next->path);
-            return -1;
-        }
-
         if (virStringListMerge(&paths, &targetPaths) < 0)
             return -1;
     }
-- 
2.24.1

Re: [PATCH] qemu: Don't crash when getting targets for a multipath

Posted by Peter Krempa 3 weeks ago
On Thu, Mar 19, 2020 at 17:22:37 +0100, Michal Privoznik wrote:
> In one of my previous commits I've introduced code that creates
> all devices for given (possible) multipath target. But I've made
> a mistake there - the code accesses src->path without checking if
> the disk source is local. Note that the path is NULL if the
> source is not local.

Well next->path 'may' be NULL if it's not local, but in this case it is
local because NVMe disks are local, but they don't have the path.

> 
> Fixes: a30078cb832646177defd256e77c632905f1e6d0
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Once the commit message makes sense:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 0e2252f6cf..ef17829235 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15862,19 +15862,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>              }
>  
>              tmpPath = g_strdup(next->path);
> +
> +            if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
> +                errno != ENOSYS && errno != EBADF) {
> +                virReportSystemError(errno,
> +                                     _("Unable to get devmapper targets for %s"),
> +                                     next->path);
> +                return -1;
> +            }
>          }
>  
>          if (virStringListAdd(&paths, tmpPath) < 0)
>              return -1;
>  
> -        if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
> -            errno != ENOSYS && errno != EBADF) {
> -            virReportSystemError(errno,
> -                                 _("Unable to get devmapper targets for %s"),
> -                                 next->path);
> -            return -1;
> -        }
> -
>          if (virStringListMerge(&paths, &targetPaths) < 0)
>              return -1;

Is this supposed to go after 'tmpPath' in the list? Otherwise you might
want to move it toghether with the call to virDevMapperGetTargets.

Re: [PATCH] qemu: Don't crash when getting targets for a multipath

Posted by Michal Prívozník 3 weeks ago
On 19. 3. 2020 17:43, Peter Krempa wrote:
> On Thu, Mar 19, 2020 at 17:22:37 +0100, Michal Privoznik wrote:
>> In one of my previous commits I've introduced code that creates
>> all devices for given (possible) multipath target. But I've made
>> a mistake there - the code accesses src->path without checking if
>> the disk source is local. Note that the path is NULL if the
>> source is not local.
> 
> Well next->path 'may' be NULL if it's not local, but in this case it is
> local because NVMe disks are local, but they don't have the path.

Local as in virStorageSourceIsLocalStorage() == true. And for NVMe we
return false exactly because src->path has to be NULL (it can't be set
to any meaningfull path).

How about this formulation:

Note that the path is NULL if the
source is not local as viewed by
virStorageSourceIsLocalStorage().

> 
>>
>> Fixes: a30078cb832646177defd256e77c632905f1e6d0
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Once the commit message makes sense:
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 0e2252f6cf..ef17829235 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -15862,19 +15862,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>>              }
>>  
>>              tmpPath = g_strdup(next->path);
>> +
>> +            if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
>> +                errno != ENOSYS && errno != EBADF) {
>> +                virReportSystemError(errno,
>> +                                     _("Unable to get devmapper targets for %s"),
>> +                                     next->path);
>> +                return -1;
>> +            }
>>          }
>>  
>>          if (virStringListAdd(&paths, tmpPath) < 0)
>>              return -1;
>>  
>> -        if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
>> -            errno != ENOSYS && errno != EBADF) {
>> -            virReportSystemError(errno,
>> -                                 _("Unable to get devmapper targets for %s"),
>> -                                 next->path);
>> -            return -1;
>> -        }
>> -
>>          if (virStringListMerge(&paths, &targetPaths) < 0)
>>              return -1;
> 
> Is this supposed to go after 'tmpPath' in the list? Otherwise you might
> want to move it toghether with the call to virDevMapperGetTargets.
> 

Good point, the order doesn't matter.

Michal

Re: [PATCH] qemu: Don't crash when getting targets for a multipath

Posted by Peter Krempa 3 weeks ago
On Thu, Mar 19, 2020 at 18:39:58 +0100, Michal Privoznik wrote:
> On 19. 3. 2020 17:43, Peter Krempa wrote:
> > On Thu, Mar 19, 2020 at 17:22:37 +0100, Michal Privoznik wrote:
> >> In one of my previous commits I've introduced code that creates
> >> all devices for given (possible) multipath target. But I've made
> >> a mistake there - the code accesses src->path without checking if
> >> the disk source is local. Note that the path is NULL if the
> >> source is not local.
> > 
> > Well next->path 'may' be NULL if it's not local, but in this case it is
> > local because NVMe disks are local, but they don't have the path.
> 
> Local as in virStorageSourceIsLocalStorage() == true. And for NVMe we
> return false exactly because src->path has to be NULL (it can't be set
> to any meaningfull path).

Yes. The meaning of virStorageSourceIsLocalStorage shifted after NVMe
disks were added.

> 
> How about this formulation:
> 
> Note that the path is NULL if the
> source is not local as viewed by
> virStorageSourceIsLocalStorage().

That is still misleading, because network disks can have non-null path.

I'd prefer:

'next->path' is NULL/doesn't make sense for VIR_STORAGE_TYPE_NVME

Re: [PATCH] qemu: Don't crash when getting targets for a multipath

Posted by Ján Tomko 3 weeks ago
On a Thursday in 2020, Michal Privoznik wrote:
>In one of my previous commits I've introduced code that creates
>all devices for given (possible) multipath target. But I've made
>a mistake there - the code accesses src->path without checking if
>the disk source is local. Note that the path is NULL if the
>source is not local.
>
>Fixes: a30078cb832646177defd256e77c632905f1e6d0
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_domain.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano