| Message ID | 1610495575-8177-3-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | Multipath io_err_stat fixes | expand |
On Tue, 2021-01-12 at 17:52 -0600, Benjamin Marzinski wrote: > The checker thread is reponsible for enqueueing paths for the > io_err_stat thread to check. During shutdown, the io_err_stat thread > is > shut down and cleaned up before the checker thread. There is no code > to make sure that the checker thread isn't accessing the io_err_stat > pathvec or its mutex while they are being freed, which can lead to > memory corruption crashes. > > To solve this, get rid of the io_err_stat_pathvec structure, and > statically define the mutex. This means that the mutex is always > valid > to access, and the io_err_stat pathvec can only be accessed while > holding it. If the io_err_stat thread has already been cleaned up > when the checker tries to access the pathvec, it will be NULL, and > the > checker will simply fail to enqueue the path. > > This change also fixes a bug in free_io_err_pathvec(), which > previously > only attempted to free the pathvec if it was not set, instead of when > it > was set. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Looks good to me. A few minor notes below. Regards, Martin > --- > libmultipath/io_err_stat.c | 108 +++++++++++++++-------------------- > -- > 1 file changed, 44 insertions(+), 64 deletions(-) > > diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c > index 2e48ee81..4c6f7f08 100644 > --- a/libmultipath/io_err_stat.c > +++ b/libmultipath/io_err_stat.c > @@ -46,12 +46,6 @@ > #define io_err_stat_log(prio, fmt, args...) \ > condlog(prio, "io error statistic: " fmt, ##args) > > - > -struct io_err_stat_pathvec { > - pthread_mutex_t mutex; > - vector pathvec; > -}; > - > struct dio_ctx { > struct timespec io_starttime; > unsigned int blksize; > @@ -75,9 +69,10 @@ static pthread_t io_err_stat_thr; > > static pthread_mutex_t io_err_thread_lock = > PTHREAD_MUTEX_INITIALIZER; > static pthread_cond_t io_err_thread_cond = PTHREAD_COND_INITIALIZER; > +static pthread_mutex_t io_err_pathvec_lock = > PTHREAD_MUTEX_INITIALIZER; > static int io_err_thread_running = 0; > > -static struct io_err_stat_pathvec *paths; > +static vector io_err_pathvec; > struct vectors *vecs; > io_context_t ioctx; > > @@ -207,46 +202,28 @@ static void free_io_err_stat_path(struct > io_err_stat_path *p) > FREE(p); > } > > -static struct io_err_stat_pathvec *alloc_pathvec(void) > +static void cleanup_unlock(void *arg) Nitpick: we've got the cleanup_mutex() utility function for this now. > { > - struct io_err_stat_pathvec *p; > - int r; > - > - p = (struct io_err_stat_pathvec *)MALLOC(sizeof(*p)); > - if (!p) > - return NULL; > - p->pathvec = vector_alloc(); > - if (!p->pathvec) > - goto out_free_struct_pathvec; > - r = pthread_mutex_init(&p->mutex, NULL); > - if (r) > - goto out_free_member_pathvec; > - > - return p; > - > -out_free_member_pathvec: > - vector_free(p->pathvec); > -out_free_struct_pathvec: > - FREE(p); > - return NULL; > + pthread_mutex_unlock((pthread_mutex_t*) arg); > } > > -static void free_io_err_pathvec(struct io_err_stat_pathvec *p) > +static void free_io_err_pathvec(void) > { > struct io_err_stat_path *path; > int i; > > - if (!p) > - return; > - pthread_mutex_destroy(&p->mutex); > - if (!p->pathvec) { > - vector_foreach_slot(p->pathvec, path, i) { > - destroy_directio_ctx(path); > - free_io_err_stat_path(path); Note: We should call destroy_directio_ctx() (only) from free_io_err_stat_path(). > - } > - vector_free(p->pathvec); > + pthread_mutex_lock(&io_err_pathvec_lock); > + pthread_cleanup_push(cleanup_unlock, &io_err_pathvec_lock); > + if (!io_err_pathvec) > + goto out; > + vector_foreach_slot(io_err_pathvec, path, i) { > + destroy_directio_ctx(path); > + free_io_err_stat_path(path); > } > - FREE(p); > + vector_free(io_err_pathvec); > + io_err_pathvec = NULL; > +out: > + pthread_cleanup_pop(1); > } > > /* > @@ -258,13 +235,13 @@ static int enqueue_io_err_stat_by_path(struct > path *path) > { > struct io_err_stat_path *p; > > - pthread_mutex_lock(&paths->mutex); > - p = find_err_path_by_dev(paths->pathvec, path->dev); > + pthread_mutex_lock(&io_err_pathvec_lock); > + p = find_err_path_by_dev(io_err_pathvec, path->dev); > if (p) { > - pthread_mutex_unlock(&paths->mutex); > + pthread_mutex_unlock(&io_err_pathvec_lock); > return 0; > } > - pthread_mutex_unlock(&paths->mutex); > + pthread_mutex_unlock(&io_err_pathvec_lock); > > p = alloc_io_err_stat_path(); > if (!p) > @@ -276,18 +253,18 @@ static int enqueue_io_err_stat_by_path(struct > path *path) > > if (setup_directio_ctx(p)) > goto free_ioerr_path; > - pthread_mutex_lock(&paths->mutex); > - if (!vector_alloc_slot(paths->pathvec)) > + pthread_mutex_lock(&io_err_pathvec_lock); > + if (!vector_alloc_slot(io_err_pathvec)) > goto unlock_destroy; > - vector_set_slot(paths->pathvec, p); > - pthread_mutex_unlock(&paths->mutex); > + vector_set_slot(io_err_pathvec, p); > + pthread_mutex_unlock(&io_err_pathvec_lock); > > io_err_stat_log(2, "%s: enqueue path %s to check", > path->mpp->alias, path->dev); Another note: This is not a level 2 log message. IMO the log levels of the io_err_stat code are generally too "low"; the only messages we want to see at 2 would be if a path's "marginal" status changes. Internals of the algorithm should log at level 3 and 4. > return 0; > > unlock_destroy: > - pthread_mutex_unlock(&paths->mutex); > + pthread_mutex_unlock(&io_err_pathvec_lock); > destroy_directio_ctx(p); > free_ioerr_path: > free_io_err_stat_path(p); > @@ -412,9 +389,9 @@ static int delete_io_err_stat_by_addr(struct > io_err_stat_path *p) > { > int i; > > - i = find_slot(paths->pathvec, p); > + i = find_slot(io_err_pathvec, p); > if (i != -1) > - vector_del_slot(paths->pathvec, i); > + vector_del_slot(io_err_pathvec, i); > > destroy_directio_ctx(p); > free_io_err_stat_path(p); > @@ -585,7 +562,7 @@ static void poll_async_io_timeout(void) > > if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0) > return; > - vector_foreach_slot(paths->pathvec, pp, i) { > + vector_foreach_slot(io_err_pathvec, pp, i) { > for (j = 0; j < CONCUR_NR_EVENT; j++) { > rc = try_to_cancel_timeout_io(pp- > >dio_ctx_array + j, > &curr_time, pp->devname); > @@ -631,7 +608,7 @@ static void handle_async_io_done_event(struct > io_event *io_evt) > int rc = PATH_UNCHECKED; > int i, j; > > - vector_foreach_slot(paths->pathvec, pp, i) { > + vector_foreach_slot(io_err_pathvec, pp, i) { > for (j = 0; j < CONCUR_NR_EVENT; j++) { > ct = pp->dio_ctx_array + j; > if (&ct->io == io_evt->obj) { > @@ -665,19 +642,14 @@ static void service_paths(void) > struct io_err_stat_path *pp; > int i; > > - pthread_mutex_lock(&paths->mutex); > - vector_foreach_slot(paths->pathvec, pp, i) { > + pthread_mutex_lock(&io_err_pathvec_lock); > + vector_foreach_slot(io_err_pathvec, pp, i) { > send_batch_async_ios(pp); > process_async_ios_event(TIMEOUT_NO_IO_NSEC, pp- > >devname); We should actually use pthread_cleanup_push() here (update: I see you changed this in patch 3/3). We should also call pthread_testcancel() before calling io_getevents(), which is not cancellation point but might block. > poll_async_io_timeout(); > poll_io_err_stat(vecs, pp); > } > - pthread_mutex_unlock(&paths->mutex); > -} > - > -static void cleanup_unlock(void *arg) > -{ > - pthread_mutex_unlock((pthread_mutex_t*) arg); > + pthread_mutex_unlock(&io_err_pathvec_lock); > } > > static void cleanup_exited(__attribute__((unused)) void *arg) > @@ -736,9 +708,14 @@ int start_io_err_stat_thread(void *data) > io_err_stat_log(4, "io_setup failed"); > return 1; > } > - paths = alloc_pathvec(); > - if (!paths) > + > + pthread_mutex_lock(&io_err_pathvec_lock); > + io_err_pathvec = vector_alloc(); > + if (!io_err_pathvec) { > + pthread_mutex_unlock(&io_err_pathvec_lock); > goto destroy_ctx; > + } > + pthread_mutex_unlock(&io_err_pathvec_lock); > > setup_thread_attr(&io_err_stat_attr, 32 * 1024, 0); > pthread_mutex_lock(&io_err_thread_lock); > @@ -763,7 +740,10 @@ int start_io_err_stat_thread(void *data) > return 0; > > out_free: > - free_io_err_pathvec(paths); > + pthread_mutex_lock(&io_err_pathvec_lock); > + vector_free(io_err_pathvec); > + io_err_pathvec = NULL; > + pthread_mutex_unlock(&io_err_pathvec_lock); > destroy_ctx: > io_destroy(ioctx); > io_err_stat_log(0, "failed to start io_error statistic > thread"); > @@ -779,6 +759,6 @@ void stop_io_err_stat_thread(void) > pthread_cancel(io_err_stat_thr); > > pthread_join(io_err_stat_thr, NULL); > - free_io_err_pathvec(paths); > + free_io_err_pathvec(); > io_destroy(ioctx); > }
diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c index 2e48ee81..4c6f7f08 100644 --- a/libmultipath/io_err_stat.c +++ b/libmultipath/io_err_stat.c @@ -46,12 +46,6 @@ #define io_err_stat_log(prio, fmt, args...) \ condlog(prio, "io error statistic: " fmt, ##args) - -struct io_err_stat_pathvec { - pthread_mutex_t mutex; - vector pathvec; -}; - struct dio_ctx { struct timespec io_starttime; unsigned int blksize; @@ -75,9 +69,10 @@ static pthread_t io_err_stat_thr; static pthread_mutex_t io_err_thread_lock = PTHREAD_MUTEX_INITIALIZER; static pthread_cond_t io_err_thread_cond = PTHREAD_COND_INITIALIZER; +static pthread_mutex_t io_err_pathvec_lock = PTHREAD_MUTEX_INITIALIZER; static int io_err_thread_running = 0; -static struct io_err_stat_pathvec *paths; +static vector io_err_pathvec; struct vectors *vecs; io_context_t ioctx; @@ -207,46 +202,28 @@ static void free_io_err_stat_path(struct io_err_stat_path *p) FREE(p); } -static struct io_err_stat_pathvec *alloc_pathvec(void) +static void cleanup_unlock(void *arg) { - struct io_err_stat_pathvec *p; - int r; - - p = (struct io_err_stat_pathvec *)MALLOC(sizeof(*p)); - if (!p) - return NULL; - p->pathvec = vector_alloc(); - if (!p->pathvec) - goto out_free_struct_pathvec; - r = pthread_mutex_init(&p->mutex, NULL); - if (r) - goto out_free_member_pathvec; - - return p; - -out_free_member_pathvec: - vector_free(p->pathvec); -out_free_struct_pathvec: - FREE(p); - return NULL; + pthread_mutex_unlock((pthread_mutex_t*) arg); } -static void free_io_err_pathvec(struct io_err_stat_pathvec *p) +static void free_io_err_pathvec(void) { struct io_err_stat_path *path; int i; - if (!p) - return; - pthread_mutex_destroy(&p->mutex); - if (!p->pathvec) { - vector_foreach_slot(p->pathvec, path, i) { - destroy_directio_ctx(path); - free_io_err_stat_path(path); - } - vector_free(p->pathvec); + pthread_mutex_lock(&io_err_pathvec_lock); + pthread_cleanup_push(cleanup_unlock, &io_err_pathvec_lock); + if (!io_err_pathvec) + goto out; + vector_foreach_slot(io_err_pathvec, path, i) { + destroy_directio_ctx(path); + free_io_err_stat_path(path); } - FREE(p); + vector_free(io_err_pathvec); + io_err_pathvec = NULL; +out: + pthread_cleanup_pop(1); } /* @@ -258,13 +235,13 @@ static int enqueue_io_err_stat_by_path(struct path *path) { struct io_err_stat_path *p; - pthread_mutex_lock(&paths->mutex); - p = find_err_path_by_dev(paths->pathvec, path->dev); + pthread_mutex_lock(&io_err_pathvec_lock); + p = find_err_path_by_dev(io_err_pathvec, path->dev); if (p) { - pthread_mutex_unlock(&paths->mutex); + pthread_mutex_unlock(&io_err_pathvec_lock); return 0; } - pthread_mutex_unlock(&paths->mutex); + pthread_mutex_unlock(&io_err_pathvec_lock); p = alloc_io_err_stat_path(); if (!p) @@ -276,18 +253,18 @@ static int enqueue_io_err_stat_by_path(struct path *path) if (setup_directio_ctx(p)) goto free_ioerr_path; - pthread_mutex_lock(&paths->mutex); - if (!vector_alloc_slot(paths->pathvec)) + pthread_mutex_lock(&io_err_pathvec_lock); + if (!vector_alloc_slot(io_err_pathvec)) goto unlock_destroy; - vector_set_slot(paths->pathvec, p); - pthread_mutex_unlock(&paths->mutex); + vector_set_slot(io_err_pathvec, p); + pthread_mutex_unlock(&io_err_pathvec_lock); io_err_stat_log(2, "%s: enqueue path %s to check", path->mpp->alias, path->dev); return 0; unlock_destroy: - pthread_mutex_unlock(&paths->mutex); + pthread_mutex_unlock(&io_err_pathvec_lock); destroy_directio_ctx(p); free_ioerr_path: free_io_err_stat_path(p); @@ -412,9 +389,9 @@ static int delete_io_err_stat_by_addr(struct io_err_stat_path *p) { int i; - i = find_slot(paths->pathvec, p); + i = find_slot(io_err_pathvec, p); if (i != -1) - vector_del_slot(paths->pathvec, i); + vector_del_slot(io_err_pathvec, i); destroy_directio_ctx(p); free_io_err_stat_path(p); @@ -585,7 +562,7 @@ static void poll_async_io_timeout(void) if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0) return; - vector_foreach_slot(paths->pathvec, pp, i) { + vector_foreach_slot(io_err_pathvec, pp, i) { for (j = 0; j < CONCUR_NR_EVENT; j++) { rc = try_to_cancel_timeout_io(pp->dio_ctx_array + j, &curr_time, pp->devname); @@ -631,7 +608,7 @@ static void handle_async_io_done_event(struct io_event *io_evt) int rc = PATH_UNCHECKED; int i, j; - vector_foreach_slot(paths->pathvec, pp, i) { + vector_foreach_slot(io_err_pathvec, pp, i) { for (j = 0; j < CONCUR_NR_EVENT; j++) { ct = pp->dio_ctx_array + j; if (&ct->io == io_evt->obj) { @@ -665,19 +642,14 @@ static void service_paths(void) struct io_err_stat_path *pp; int i; - pthread_mutex_lock(&paths->mutex); - vector_foreach_slot(paths->pathvec, pp, i) { + pthread_mutex_lock(&io_err_pathvec_lock); + vector_foreach_slot(io_err_pathvec, pp, i) { send_batch_async_ios(pp); process_async_ios_event(TIMEOUT_NO_IO_NSEC, pp->devname); poll_async_io_timeout(); poll_io_err_stat(vecs, pp); } - pthread_mutex_unlock(&paths->mutex); -} - -static void cleanup_unlock(void *arg) -{ - pthread_mutex_unlock((pthread_mutex_t*) arg); + pthread_mutex_unlock(&io_err_pathvec_lock); } static void cleanup_exited(__attribute__((unused)) void *arg) @@ -736,9 +708,14 @@ int start_io_err_stat_thread(void *data) io_err_stat_log(4, "io_setup failed"); return 1; } - paths = alloc_pathvec(); - if (!paths) + + pthread_mutex_lock(&io_err_pathvec_lock); + io_err_pathvec = vector_alloc(); + if (!io_err_pathvec) { + pthread_mutex_unlock(&io_err_pathvec_lock); goto destroy_ctx; + } + pthread_mutex_unlock(&io_err_pathvec_lock); setup_thread_attr(&io_err_stat_attr, 32 * 1024, 0); pthread_mutex_lock(&io_err_thread_lock); @@ -763,7 +740,10 @@ int start_io_err_stat_thread(void *data) return 0; out_free: - free_io_err_pathvec(paths); + pthread_mutex_lock(&io_err_pathvec_lock); + vector_free(io_err_pathvec); + io_err_pathvec = NULL; + pthread_mutex_unlock(&io_err_pathvec_lock); destroy_ctx: io_destroy(ioctx); io_err_stat_log(0, "failed to start io_error statistic thread"); @@ -779,6 +759,6 @@ void stop_io_err_stat_thread(void) pthread_cancel(io_err_stat_thr); pthread_join(io_err_stat_thr, NULL); - free_io_err_pathvec(paths); + free_io_err_pathvec(); io_destroy(ioctx); }
The checker thread is reponsible for enqueueing paths for the io_err_stat thread to check. During shutdown, the io_err_stat thread is shut down and cleaned up before the checker thread. There is no code to make sure that the checker thread isn't accessing the io_err_stat pathvec or its mutex while they are being freed, which can lead to memory corruption crashes. To solve this, get rid of the io_err_stat_pathvec structure, and statically define the mutex. This means that the mutex is always valid to access, and the io_err_stat pathvec can only be accessed while holding it. If the io_err_stat thread has already been cleaned up when the checker tries to access the pathvec, it will be NULL, and the checker will simply fail to enqueue the path. This change also fixes a bug in free_io_err_pathvec(), which previously only attempted to free the pathvec if it was not set, instead of when it was set. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/io_err_stat.c | 108 +++++++++++++++---------------------- 1 file changed, 44 insertions(+), 64 deletions(-)