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

[Martin Wilck]

On Mon, 2020-10-26 at 17:22 +0100, Martin Wilck wrote:
> On Mon, 2020-10-19 at 21:20 -0500, Benjamin Marzinski wrote:
> > On Fri, Oct 16, 2020 at 12:45:01PM +0200, mwilck at suse.com wrote:
> > > From: Martin Wilck <mwilck at 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. 

OTOH, we take logev_lock in log_safe() anyway (to set
log_messages_pending). I doubt that it makes a big difference if we
take the two locks sequentially, or nested. The previous code actually
took the logev_lock twice, before and after logq_lock. Assuming that
contention is rather rare, I believe my code might actually perform
better than before. 

In your previous review 
https://www.redhat.com/archives/dm-devel/2020-September/msg00631.html
you pointed out that you considered it important that log_safe() works
even after the thread was stopped. Making this work implies that
log_safe() needs to check if the thread is up. So we either have to
take logev_lock twice, or take logq_lock while holding logev_lock.

Bottom line: I think my patch is correct. We could add another patch on
top that moves logq_lock() into log.c, protecting the "la" variable,
but the nesting would still need to be the same.

Does this make sense?
     
Regards,
Martin