1:55 a.m.
From: Chongyun Wu <wucy11(a)chinatelecom.cn>
pointer disk might be null in some special cases or new
usage scenarios, therefore code protection is needed to
prevent segment faults.
Signed-off-by: Chongyun Wu <wucy11(a)chinatelecom.cn>
---
src/qemu/qemu_blockjob.c | 43 ++++++++++++++++++++++++-------------------
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index faf9a9f..00506b9 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -781,12 +781,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
{
virDomainDiskDef *disk = job->disk;
- VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d",
- disk->dst,
- NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
- job->type,
- job->state,
- job->newstate);
+ if (disk)
+ VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d",
+ disk->dst,
+ NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
+ job->type,
+ job->state,
+ job->newstate);
if (job->newstate == -1)
return;
@@ -804,26 +805,30 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
break;
case VIR_DOMAIN_BLOCK_JOB_READY:
- disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
- qemuDomainSaveStatus(vm);
+ if (disk) {
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
+ qemuDomainSaveStatus(vm);
+ }
break;
case VIR_DOMAIN_BLOCK_JOB_FAILED:
case VIR_DOMAIN_BLOCK_JOB_CANCELED:
- if (disk->mirror) {
- virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
+ if (disk) {
+ if (disk->mirror) {
+ virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
- /* Ideally, we would restore seclabels on the backing chain here
- * but we don't know if somebody else is not using parts of it.
- * Remove security driver metadata so that they are not leaked. */
- qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror);
+ /* Ideally, we would restore seclabels on the backing chain here
+ * but we don't know if somebody else is not using parts of it.
+ * Remove security driver metadata so that they are not leaked. */
+ qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror);
- virObjectUnref(disk->mirror);
- disk->mirror = NULL;
+ virObjectUnref(disk->mirror);
+ disk->mirror = NULL;
+ }
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
+ disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
+ qemuBlockJobUnregister(job, vm);
}
- disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
- disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
- qemuBlockJobUnregister(job, vm);
break;
case VIR_DOMAIN_BLOCK_JOB_LAST:
--
1.8.3.1
2:24 a.m.
On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81(a)chinatelecom.cn wrote:
From: Chongyun Wu <wucy11(a)chinatelecom.cn>
pointer disk might be null in some special cases or new
usage scenarios, therefore code protection is needed to
prevent segment faults.
Please elaborate on when that happens. Generally the legacy block job
handler should no longer be in action with new versions, elaborate
please also where you are seeing this.
Signed-off-by: Chongyun Wu <wucy11(a)chinatelecom.cn>
---
src/qemu/qemu_blockjob.c | 43 ++++++++++++++++++++++++-------------------
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index faf9a9f..00506b9 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -781,12 +781,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
{
virDomainDiskDef *disk = job->disk;
- VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d",
- disk->dst,
- NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
- job->type,
- job->state,
- job->newstate);
+ if (disk)
+ VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d",
+ disk->dst,
+ NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
+ job->type,
+ job->state,
+ job->newstate);
The legacy block job handler makes no real sense if disk is NULL ...
if (job->newstate == -1)
return;
@@ -804,26 +805,30 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
break;
case VIR_DOMAIN_BLOCK_JOB_READY:
- disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
- qemuDomainSaveStatus(vm);
+ if (disk) {
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
+ qemuDomainSaveStatus(vm);
... so the whole function can be skipped ...
+ }
break;
case VIR_DOMAIN_BLOCK_JOB_FAILED:
case VIR_DOMAIN_BLOCK_JOB_CANCELED:
- if (disk->mirror) {
- virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
+ if (disk) {
+ if (disk->mirror) {
+ virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
... rather than special casing everything.
- /* Ideally, we would restore seclabels on the backing chain here
- * but we don't know if somebody else is not using parts of it.
- * Remove security driver metadata so that they are not leaked. */
- qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror);
+ /* Ideally, we would restore seclabels on the backing chain here
+ * but we don't know if somebody else is not using parts of it.
+ * Remove security driver metadata so that they are not leaked. */
+ qemuBlockRemoveImageMetadata(driver, vm, disk->dst,
disk->mirror);
- virObjectUnref(disk->mirror);
- disk->mirror = NULL;
+ virObjectUnref(disk->mirror);
+ disk->mirror = NULL;
+ }
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
+ disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
+ qemuBlockJobUnregister(job, vm);
}
- disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
- disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
- qemuBlockJobUnregister(job, vm);
break;
case VIR_DOMAIN_BLOCK_JOB_LAST:
--
1.8.3.1
3:37 a.m.
On 2021/7/1 15:24, Peter Krempa wrote:
On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81(a)chinatelecom.cn
wrote:
> From: Chongyun Wu <wucy11(a)chinatelecom.cn>
>
> pointer disk might be null in some special cases or new
> usage scenarios, therefore code protection is needed to
> prevent segment faults.
Please elaborate on when that happens. Generally the legacy block job
handler should no longer be in action with new versions, elaborate
please also where you are seeing this.
Thanks for your comments. I found it when I
develop a new feature which make
quemu2.12 support the rbd block hot migration with blockcopy command, I changed
some processes and triggered this crash, and normal use will not have this
problem. So I just want to do some protection at the code level. Thanks~
>
> Signed-off-by: Chongyun Wu <wucy11(a)chinatelecom.cn>
> ---
> src/qemu/qemu_blockjob.c | 43 ++++++++++++++++++++++++-------------------
> 1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index faf9a9f..00506b9 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -781,12 +781,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
> {
> virDomainDiskDef *disk = job->disk;
>
> - VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d",
> - disk->dst,
> - NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
> - job->type,
> - job->state,
> - job->newstate);
> + if (disk)
> + VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d,
newstate=%d",
> + disk->dst,
> +
NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
> + job->type,
> + job->state,
> + job->newstate);
The legacy block job handler makes no real sense if disk is NULL ...
Yes, you are
right.
The reason I used to do this is that if ob->state is
VIR_DOMAIN_BLOCK_JOB_COMPLETED, you can ignore whether the disk is empty.
But what you said also makes sens, maybe I can do the judgement at the beginning.
>
> if (job->newstate == -1)
> return;
> @@ -804,26 +805,30 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
> break;
>
> case VIR_DOMAIN_BLOCK_JOB_READY:
> - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> - qemuDomainSaveStatus(vm);
> + if (disk) {
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> + qemuDomainSaveStatus(vm);
... so the whole function can be skipped ...
> + }
> break;
>
> case VIR_DOMAIN_BLOCK_JOB_FAILED:
> case VIR_DOMAIN_BLOCK_JOB_CANCELED:
> - if (disk->mirror) {
> - virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
> + if (disk) {
> + if (disk->mirror) {
> + virDomainLockImageDetach(driver->lockManager, vm,
disk->mirror);
... rather than special casing everything.
>
> - /* Ideally, we would restore seclabels on the backing chain here
> - * but we don't know if somebody else is not using parts of it.
> - * Remove security driver metadata so that they are not leaked. */
> - qemuBlockRemoveImageMetadata(driver, vm, disk->dst,
disk->mirror);
> + /* Ideally, we would restore seclabels on the backing chain here
> + * but we don't know if somebody else is not using parts of it.
> + * Remove security driver metadata so that they are not leaked. */
> + qemuBlockRemoveImageMetadata(driver, vm, disk->dst,
disk->mirror);
>
> - virObjectUnref(disk->mirror);
> - disk->mirror = NULL;
> + virObjectUnref(disk->mirror);
> + disk->mirror = NULL;
> + }
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> + qemuBlockJobUnregister(job, vm);
> }
> - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> - qemuBlockJobUnregister(job, vm);
> break;
>
> case VIR_DOMAIN_BLOCK_JOB_LAST:
> --
> 1.8.3.1
>
--
Best Regard,
Chongyun Wu
7 a.m.
On Thu, Jul 01, 2021 at 16:37:29 +0800, Chongyun Wu wrote:
On 2021/7/1 15:24, Peter Krempa wrote:
> On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81(a)chinatelecom.cn wrote:
> > From: Chongyun Wu <wucy11(a)chinatelecom.cn>
> >
> > pointer disk might be null in some special cases or new
> > usage scenarios, therefore code protection is needed to
> > prevent segment faults.
>
> Please elaborate on when that happens. Generally the legacy block job
> handler should no longer be in action with new versions, elaborate
> please also where you are seeing this.
Thanks for your comments. I found it when I develop a new feature which make
quemu2.12 support the rbd block hot migration with blockcopy command, I changed
Well qemu 2.12 is very old at this point, we techincally support it but
if you are using latest libvirt I'd strongly suggest also using a more
modern qemu version.
Also note, that blockcopy to network destinations is supposed to
properly work with qemu 4.2 and newer which uses -blockdev to configure
disks.
some processes and triggered this crash, and normal use will not have
this
You mean you've modified libvirt which lead to a crash?
I'm still curious to what happened to trigger the issue. In many cases a
fix such like this fixes the symptom and not the root cause for the
problem.
problem. So I just want to do some protection at the code level.
Thanks~
>
> >
> > Signed-off-by: Chongyun Wu <wucy11(a)chinatelecom.cn>
> > ---
> > src/qemu/qemu_blockjob.c | 43 ++++++++++++++++++++++++-------------------
> > 1 file changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > index faf9a9f..00506b9 100644
> > --- a/src/qemu/qemu_blockjob.c
> > +++ b/src/qemu/qemu_blockjob.c
> > @@ -781,12 +781,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver,
> > {
> > virDomainDiskDef *disk = job->disk;
> > - VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d,
newstate=%d",
> > - disk->dst,
> > -
NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
> > - job->type,
> > - job->state,
> > - job->newstate);
> > + if (disk)
> > + VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d,
newstate=%d",
> > + disk->dst,
> > +
NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)),
> > + job->type,
> > + job->state,
> > + job->newstate);
>
> The legacy block job handler makes no real sense if disk is NULL ...
Yes, you are right.
The reason I used to do this is that if ob->state is
VIR_DOMAIN_BLOCK_JOB_COMPLETED, you can ignore whether the disk is empty.
But what you said also makes sens, maybe I can do the judgement at the beginning.
Well in the legacy block job handlers it shouldn't be possible that a
VIR_DOMAIN_BLOCK_JOB_COMPLETED event is delivered when disk is already
NULL. That's why I'm asking what exact steps lead to the crash so that I
can investigate also the possible root cause of the problem.
1240
days inactive
1240
days old
3 comments
3 participants
participants (3)
-
Chongyun Wu -
huangy81＠chinatelecom.cn -
Peter Krempa