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

[Benjamin Marzinski <bmarzins@redhat.com>]

On Mon, Oct 26, 2020 at 06:24:57PM +0100, Martin Wilck wrote:
> 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@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. 
> 
> 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?

No. Maybe I'm just being dumb, but couldn't log safe:

- grab the logq_lock
- check if the log_area is usable. We can use la for this.
	- If not, unlock, write to syslog and return
	- If so, you know that flush_logqueue() hasn't been run by
	  log_thread_stop() yet, meaning anything you add to the log
	  will get flushed by flush_logqueue, so enqueue the message
-unlock logq_lock and lock logev_lock
-signal that there are messages pending. If nobody is listening on the
 the other side, who cares, because log_thread_stop() will still flush
 the enqueued message
-unlock logev_lock

Am I missing something?

-Ben

>      
> Regards,
> Martin
> 
> 

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