[PATCH] qemu: code protection for qemuBlockJobEventProcessLegacy
From: Chongyun Wu <wucy11@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@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
On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81@chinatelecom.cn wrote:
From: Chongyun Wu <wucy11@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@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
On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81@chinatelecom.cn wrote:
From: Chongyun Wu <wucy11@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
On 2021/7/1 15:24, Peter Krempa wrote: problem. So I just want to do some protection at the code level. Thanks~
Signed-off-by: Chongyun Wu <wucy11@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
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@chinatelecom.cn wrote:
From: Chongyun Wu <wucy11@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@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.
participants (3)
-
Chongyun Wu -
huangy81＠chinatelecom.cn -
Peter Krempa