[dm-devel] [PATCH 17/32] libmultipath: fix tur checker double locking

[Benjamin Marzinski]

On Sun, Aug 05, 2018 at 03:40:30PM +0000, Bart Van Assche wrote:
> On Wed, 2018-08-01 at 15:57 -0500, Benjamin Marzinski wrote:
> > I realize that this locking was added to make an analyzer happy,
> > presumably because sometimes ct->devt was being accessed while ct->devt
> > was held, and sometimes not.  The tur checker locking will be cleaned
> > up in a later patch to deal with this.
> 
> Are you perhaps referring to commit 873be9fef222 ("libmultipath/checkers/tur:
> Serialize tur_checker_context.devt accesses")? Your assumption about how DRD
> works is wrong. DRD doesn't care about which mutex or other synchronization
> primitives are used to serialize accesses to data that is shared between
> threads. All it cares about is that concurrent accesses to shared data are
> serialized. I'm afraid that your patch reintroduces the race conditions that
> were fixed by commit 873be9fef222. Have you considered to fix this without
> removing the locking?

I'm confused here. If the data is only read by multiple threads, I don't
see why we need to synchronize it. Unless I'm missing something, my
patch does make sure that the only time ct->devt is written is when
there is only one thread running that has access to ct.

-       if (fstat(c->fd, &sb) == 0) {
...
+       if (uatomic_read(&ct->holders) == 1 && ct->devt[0] == 0 &&
+           fstat(c->fd, &sb) == 0) {

holders will always be 2 when a tur checker thread is running (well, the
thread will decrement it when it finishes, but it will not touch ct
after that action). Since only the thread calling libcheck_init can
start the tur checker thread using this context, if there is no thread
running at the start of libcheck_check(), then no thread can be started
until libcheck_check() starts it.

Ideally, ct->devt would be set in libcheck_init(), but we don't have the
information there, so setting it once, on the first time we call
libcheck_check() seems like a reasonable, and safe, answer.  I certainly
prefer it to adding recusive locking where it isn't needed.

-Ben

> 
> Bart.