Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Martin Wilck <mwilck@suse.com>]

On Mon, 2020-10-19 at 21:20 -0500, Benjamin Marzinski wrote:
> On Fri, Oct 16, 2020 at 12:45:01PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > 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.
> > 
> 
> I don't see any problems with this, but I also don't think it's
> necssary
> to hold the log thread lock (logev_lock), just to add a message to
> the
> queue. It seems like protecting the log queue is the job of
> logq_lock.
> As long as log_safe() enqueues the message before flush_logqueue() is
> called in log_thread_stop(), it should be fine. This could be solved
> by
> simply having a static variable in log_pthread.c named something like
> log_area_enabled, that always accessed while holding the logq_lock,
> and
> is set to true when the log_area is created, and set to false just
> before calling the flush_logqueue() in log_thread_stop().

If we do this, we might as well use the variable "la" itself for that,
and make sure it's only accessed under the lock. It'd be fine, because
la is used if and only if the log thread is active, and spare us
another variable. I had actually considered that, thought it was too
invasive for the already big series. If you prefer this way, I can do
it. 

Martin


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