Message ID | 20211104145334.1346363-14-eesposit@redhat.com |
---|---|
State | New |
Headers | show |
Series | job: replace AioContext lock with job_mutex | expand |
04.11.2021 17:53, Emanuele Giuseppe Esposito wrote: > Both blockdev.c and job-qmp.c have TOC/TOU conditions, because > they first search for the job and then perform an action on it. > Therefore, we need to do the search + action under the same > job mutex critical section. > > Note: at this stage, job_{lock/unlock} and job lock guard macros > are *nop*. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > blockdev.c | 9 +++++++++ > job-qmp.c | 8 ++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index c5a835d9ed..0bd79757fc 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3327,12 +3327,14 @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, > assert(id != NULL); > > *aio_context = NULL; > + job_lock(); JOB_LOCK_GUARD() will look better in this case > > job = block_job_get(id); > > if (!job) { > error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, > "Block job '%s' not found", id); > + job_unlock(); > return NULL; > } > > @@ -3353,6 +3355,7 @@ void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) > > block_job_set_speed(job, speed, errp); > aio_context_release(aio_context); > + job_unlock(); You add job_unlock(), but not job_lock() ? Something is wrong. And anyway, I thin JOB_LOCK_GUARD / WITH_JOB_LOCK_GUARD are generally safer > } > > void qmp_block_job_cancel(const char *device, > @@ -3379,6 +3382,7 @@ void qmp_block_job_cancel(const char *device, > job_user_cancel(&job->job, force, errp); > out: > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_block_job_pause(const char *device, Error **errp) > @@ -3393,6 +3397,7 @@ void qmp_block_job_pause(const char *device, Error **errp) > trace_qmp_block_job_pause(job); > job_user_pause(&job->job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_block_job_resume(const char *device, Error **errp) > @@ -3407,6 +3412,7 @@ void qmp_block_job_resume(const char *device, Error **errp) > trace_qmp_block_job_resume(job); > job_user_resume(&job->job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_block_job_complete(const char *device, Error **errp) > @@ -3421,6 +3427,7 @@ void qmp_block_job_complete(const char *device, Error **errp) > trace_qmp_block_job_complete(job); > job_complete(&job->job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_block_job_finalize(const char *id, Error **errp) > @@ -3444,6 +3451,7 @@ void qmp_block_job_finalize(const char *id, Error **errp) > aio_context = blk_get_aio_context(job->blk); > job_unref(&job->job); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_block_job_dismiss(const char *id, Error **errp) > @@ -3460,6 +3468,7 @@ void qmp_block_job_dismiss(const char *id, Error **errp) > job = &bjob->job; > job_dismiss(&job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_change_backing_file(const char *device, > diff --git a/job-qmp.c b/job-qmp.c > index a355dc2954..8f07c51db8 100644 > --- a/job-qmp.c > +++ b/job-qmp.c > @@ -35,10 +35,12 @@ static Job *find_job(const char *id, AioContext **aio_context, Error **errp) > Job *job; > > *aio_context = NULL; > + job_lock(); > > job = job_get(id); > if (!job) { > error_setg(errp, "Job not found"); > + job_unlock(); > return NULL; > } > > @@ -60,6 +62,7 @@ void qmp_job_cancel(const char *id, Error **errp) > trace_qmp_job_cancel(job); > job_user_cancel(job, true, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_job_pause(const char *id, Error **errp) > @@ -74,6 +77,7 @@ void qmp_job_pause(const char *id, Error **errp) > trace_qmp_job_pause(job); > job_user_pause(job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_job_resume(const char *id, Error **errp) > @@ -88,6 +92,7 @@ void qmp_job_resume(const char *id, Error **errp) > trace_qmp_job_resume(job); > job_user_resume(job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_job_complete(const char *id, Error **errp) > @@ -102,6 +107,7 @@ void qmp_job_complete(const char *id, Error **errp) > trace_qmp_job_complete(job); > job_complete(job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_job_finalize(const char *id, Error **errp) > @@ -125,6 +131,7 @@ void qmp_job_finalize(const char *id, Error **errp) > aio_context = job->aio_context; > job_unref(job); > aio_context_release(aio_context); > + job_unlock(); > } > > void qmp_job_dismiss(const char *id, Error **errp) > @@ -139,6 +146,7 @@ void qmp_job_dismiss(const char *id, Error **errp) > trace_qmp_job_dismiss(job); > job_dismiss(&job, errp); > aio_context_release(aio_context); > + job_unlock(); > } > > static JobInfo *job_query_single(Job *job, Error **errp) >
18.12.2021 15:11, Vladimir Sementsov-Ogievskiy wrote: > 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote: >> Both blockdev.c and job-qmp.c have TOC/TOU conditions, because >> they first search for the job and then perform an action on it. >> Therefore, we need to do the search + action under the same >> job mutex critical section. >> >> Note: at this stage, job_{lock/unlock} and job lock guard macros >> are *nop*. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> blockdev.c | 9 +++++++++ >> job-qmp.c | 8 ++++++++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/blockdev.c b/blockdev.c >> index c5a835d9ed..0bd79757fc 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -3327,12 +3327,14 @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, >> assert(id != NULL); >> *aio_context = NULL; >> + job_lock(); > > JOB_LOCK_GUARD() will look better in this case > >> job = block_job_get(id); >> if (!job) { >> error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, >> "Block job '%s' not found", id); >> + job_unlock(); >> return NULL; >> } >> @@ -3353,6 +3355,7 @@ void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) >> block_job_set_speed(job, speed, errp); >> aio_context_release(aio_context); >> + job_unlock(); > > You add job_unlock(), but not job_lock() ? Something is wrong. And anyway, I thin JOB_LOCK_GUARD / WITH_JOB_LOCK_GUARD are generally safer Ah, I understand now what's going on. If comment "Returns with job_lock held on success" appear in this patch, it would be more obvious.
diff --git a/blockdev.c b/blockdev.c index c5a835d9ed..0bd79757fc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3327,12 +3327,14 @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, assert(id != NULL); *aio_context = NULL; + job_lock(); job = block_job_get(id); if (!job) { error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, "Block job '%s' not found", id); + job_unlock(); return NULL; } @@ -3353,6 +3355,7 @@ void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) block_job_set_speed(job, speed, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_block_job_cancel(const char *device, @@ -3379,6 +3382,7 @@ void qmp_block_job_cancel(const char *device, job_user_cancel(&job->job, force, errp); out: aio_context_release(aio_context); + job_unlock(); } void qmp_block_job_pause(const char *device, Error **errp) @@ -3393,6 +3397,7 @@ void qmp_block_job_pause(const char *device, Error **errp) trace_qmp_block_job_pause(job); job_user_pause(&job->job, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_block_job_resume(const char *device, Error **errp) @@ -3407,6 +3412,7 @@ void qmp_block_job_resume(const char *device, Error **errp) trace_qmp_block_job_resume(job); job_user_resume(&job->job, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_block_job_complete(const char *device, Error **errp) @@ -3421,6 +3427,7 @@ void qmp_block_job_complete(const char *device, Error **errp) trace_qmp_block_job_complete(job); job_complete(&job->job, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_block_job_finalize(const char *id, Error **errp) @@ -3444,6 +3451,7 @@ void qmp_block_job_finalize(const char *id, Error **errp) aio_context = blk_get_aio_context(job->blk); job_unref(&job->job); aio_context_release(aio_context); + job_unlock(); } void qmp_block_job_dismiss(const char *id, Error **errp) @@ -3460,6 +3468,7 @@ void qmp_block_job_dismiss(const char *id, Error **errp) job = &bjob->job; job_dismiss(&job, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_change_backing_file(const char *device, diff --git a/job-qmp.c b/job-qmp.c index a355dc2954..8f07c51db8 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -35,10 +35,12 @@ static Job *find_job(const char *id, AioContext **aio_context, Error **errp) Job *job; *aio_context = NULL; + job_lock(); job = job_get(id); if (!job) { error_setg(errp, "Job not found"); + job_unlock(); return NULL; } @@ -60,6 +62,7 @@ void qmp_job_cancel(const char *id, Error **errp) trace_qmp_job_cancel(job); job_user_cancel(job, true, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_job_pause(const char *id, Error **errp) @@ -74,6 +77,7 @@ void qmp_job_pause(const char *id, Error **errp) trace_qmp_job_pause(job); job_user_pause(job, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_job_resume(const char *id, Error **errp) @@ -88,6 +92,7 @@ void qmp_job_resume(const char *id, Error **errp) trace_qmp_job_resume(job); job_user_resume(job, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_job_complete(const char *id, Error **errp) @@ -102,6 +107,7 @@ void qmp_job_complete(const char *id, Error **errp) trace_qmp_job_complete(job); job_complete(job, errp); aio_context_release(aio_context); + job_unlock(); } void qmp_job_finalize(const char *id, Error **errp) @@ -125,6 +131,7 @@ void qmp_job_finalize(const char *id, Error **errp) aio_context = job->aio_context; job_unref(job); aio_context_release(aio_context); + job_unlock(); } void qmp_job_dismiss(const char *id, Error **errp) @@ -139,6 +146,7 @@ void qmp_job_dismiss(const char *id, Error **errp) trace_qmp_job_dismiss(job); job_dismiss(&job, errp); aio_context_release(aio_context); + job_unlock(); } static JobInfo *job_query_single(Job *job, Error **errp)
Both blockdev.c and job-qmp.c have TOC/TOU conditions, because they first search for the job and then perform an action on it. Therefore, we need to do the search + action under the same job mutex critical section. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- blockdev.c | 9 +++++++++ job-qmp.c | 8 ++++++++ 2 files changed, 17 insertions(+)