]>
Commit | Line | Data |
---|---|---|
7f4326d1 WB |
1 | From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 |
2 | From: Fabian Ebner <f.ebner@proxmox.com> | |
3 | Date: Wed, 25 May 2022 13:59:38 +0200 | |
4 | Subject: [PATCH] PVE-Backup: ensure jobs in di_list are referenced | |
5 | ||
6 | Ensures that qmp_backup_cancel doesn't pick a job that's already been | |
7 | freed. With unlucky timings it seems possible that: | |
8 | 1. job_exit -> job_completed -> job_finalize_single starts | |
9 | 2. pvebackup_co_complete_stream gets spawned in completion callback | |
10 | 3. job finalize_single finishes -> job's refcount hits zero -> job is | |
11 | freed | |
12 | 4. qmp_backup_cancel comes in and locks backup_state.backup_mutex | |
13 | before pvebackup_co_complete_stream can remove the job from the | |
14 | di_list | |
15 | 5. qmp_backup_cancel will pick a job that's already been freed | |
16 | ||
d03e1b3c | 17 | Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> |
7f4326d1 | 18 | Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> |
d03e1b3c FE |
19 | [FE: adapt for new job lock mechanism replacing AioContext locks] |
20 | Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> | |
7f4326d1 | 21 | --- |
d03e1b3c FE |
22 | pve-backup.c | 22 +++++++++++++++++++--- |
23 | 1 file changed, 19 insertions(+), 3 deletions(-) | |
7f4326d1 | 24 | |
4fd0fa7f | 25 | diff --git a/pve-backup.c b/pve-backup.c |
d03e1b3c | 26 | index fde3554133..0cf30e1ced 100644 |
4fd0fa7f TL |
27 | --- a/pve-backup.c |
28 | +++ b/pve-backup.c | |
d03e1b3c | 29 | @@ -316,6 +316,13 @@ static void coroutine_fn pvebackup_co_complete_stream(void *opaque) |
7f4326d1 WB |
30 | } |
31 | } | |
32 | ||
33 | + if (di->job) { | |
d03e1b3c FE |
34 | + WITH_JOB_LOCK_GUARD() { |
35 | + job_unref_locked(&di->job->job); | |
36 | + di->job = NULL; | |
37 | + } | |
7f4326d1 WB |
38 | + } |
39 | + | |
40 | // remove self from job list | |
41 | backup_state.di_list = g_list_remove(backup_state.di_list, di); | |
42 | ||
d03e1b3c FE |
43 | @@ -491,6 +498,11 @@ static void create_backup_jobs_bh(void *opaque) { |
44 | aio_context_release(aio_context); | |
7f4326d1 WB |
45 | |
46 | di->job = job; | |
47 | + if (job) { | |
d03e1b3c FE |
48 | + WITH_JOB_LOCK_GUARD() { |
49 | + job_ref_locked(&job->job); | |
50 | + } | |
7f4326d1 WB |
51 | + } |
52 | ||
53 | if (!job || local_err) { | |
54 | error_setg(errp, "backup_job_create failed: %s", | |
d03e1b3c | 55 | @@ -518,11 +530,15 @@ static void create_backup_jobs_bh(void *opaque) { |
7bd4d864 | 56 | di->target = NULL; |
7f4326d1 | 57 | } |
7bd4d864 FE |
58 | |
59 | - if (!canceled && di->job) { | |
7f4326d1 | 60 | + if (di->job) { |
d03e1b3c FE |
61 | WITH_JOB_LOCK_GUARD() { |
62 | - job_cancel_sync_locked(&di->job->job, true); | |
63 | + if (!canceled) { | |
64 | + job_cancel_sync_locked(&di->job->job, true); | |
65 | + canceled = true; | |
66 | + } | |
67 | + job_unref_locked(&di->job->job); | |
68 | + di->job = NULL; | |
69 | } | |
7bd4d864 FE |
70 | - canceled = true; |
71 | } | |
7f4326d1 WB |
72 | } |
73 | } |