Re: [PATCH 2/3] multipathd: avoid io_err_stat crash during shutdown

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

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);
>  }

-- 
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer



--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux