On Wed, Dec 16, 2020 at 07:17:08PM +0100, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> log_safe() could race with log_thread_stop(); simply
> checking the value of log_thr has never been safe. By converting the
> mutexes to static initializers, we avoid having to destroy them, and thus
> possibly accessing a destroyed mutex in log_safe(). Furthermore, taking
> both the logev_lock and the logq_lock makes sure the logarea isn't freed
> while we are writing to it.
>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> libmultipath/log_pthread.c | 48 +++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 3a2566a..0d48c52 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -17,31 +17,42 @@
>
> static pthread_t log_thr;
>
> -static pthread_mutex_t logq_lock;
> -static pthread_mutex_t logev_lock;
> -static pthread_cond_t logev_cond;
> +/* logev_lock must not be taken with logq_lock held */
> +static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_mutex_t logev_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t logev_cond = PTHREAD_COND_INITIALIZER;
>
> static int logq_running;
> static int log_messages_pending;
>
> void log_safe (int prio, const char * fmt, va_list ap)
> {
> + bool running;
> +
> if (prio > LOG_DEBUG)
> prio = LOG_DEBUG;
>
> - if (log_thr == (pthread_t)0) {
> - vsyslog(prio, fmt, ap);
> - return;
> - }
> + /*
> + * logev_lock protects logq_running. By holding it, we avoid a race
> + * with log_thread_stop() -> log_close(), which would free the logarea.
> + */
> + pthread_mutex_lock(&logev_lock);
> + pthread_cleanup_push(cleanup_mutex, &logev_lock);
> + running = logq_running;
>
> - pthread_mutex_lock(&logq_lock);
> - log_enqueue(prio, fmt, ap);
> - pthread_mutex_unlock(&logq_lock);
> + if (running) {
> + pthread_mutex_lock(&logq_lock);
> + pthread_cleanup_push(cleanup_mutex, &logq_lock);
> + log_enqueue(prio, fmt, ap);
> + pthread_cleanup_pop(1);
>
> - pthread_mutex_lock(&logev_lock);
> - log_messages_pending = 1;
> - pthread_cond_signal(&logev_cond);
> - pthread_mutex_unlock(&logev_lock);
> + log_messages_pending = 1;
> + pthread_cond_signal(&logev_cond);
> + }
> + pthread_cleanup_pop(1);
> +
> + if (!running)
> + vsyslog(prio, fmt, ap);
> }
>
> static void flush_logqueue (void)
> @@ -103,9 +114,6 @@ void log_thread_start (pthread_attr_t *attr)
> int running = 0;
>
> logdbg(stderr,"enter log_thread_start\n");
> - pthread_mutex_init(&logq_lock, NULL);
> - pthread_mutex_init(&logev_lock, NULL);
> - pthread_cond_init(&logev_cond, NULL);
>
> if (log_init("multipathd", 0)) {
> fprintf(stderr,"can't initialize log buffer\n");
> @@ -154,13 +162,9 @@ void log_thread_stop (void)
> }
> pthread_cleanup_pop(1);
>
> - flush_logqueue();
> if (running)
> pthread_join(log_thr, NULL);
>
> - pthread_mutex_destroy(&logq_lock);
> - pthread_mutex_destroy(&logev_lock);
> - pthread_cond_destroy(&logev_cond);
> -
> + flush_logqueue();
> log_close();
> }
> --
> 2.29.0
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
