[patch] dm-mpath pg_init could race with destructor

[patch] dm-mpath pg_init could race with destructor

[Lars Marowsky-Bree]

This patch addresses a race found by Ed.

From his description:

(1) This use case involves a call to switch_pg_num() before an io
suspend.  The path group switch is delayed.  When multipath_presuspend()
is called as part of the io suspend, the call to queue_work(kmultipathd,
&m->process_queued_ios) from multipath_presuspend() can cause a pg_init
io to be sent even if there are no other ios involved.

Why send a pg_init request at this point if there are no queued ios?
This was in fact my solution, to only send a pg_init io if there are
queued ios.

(2) This use case involves already having a pg_init io outstanding (and
one or more queued ios) at the time an io suspend occurs.  Assume
further that (1) all paths are down to the device and (2) the device's
multipath configuration includes the "queue_if_no_path" feature.  The
call to queue_work(kmultipathd, &m->process_queued_ios) will lead to
process_queued_ios() dispatching any/all queued ios -- even though the
pg_init request is still outstanding for the device.  If all of these
ios complete before the pg_init io completes, it is possible that the
multipath destructor is called as part of either swapping the device's
old map or closing the mapped device.

A (3) safety check was added by myself: No point in issueing a pg_init
if we don't have a valid path. Where would we sent it to? This shouldn't
occur, because __switch_pg() (which is the only place which can set
m->pg_init_required) is only called if we do have a valid path. However,
could user-space with bad timing intervene and fail that path before we
have run pg_init - in which case pg_init_required would still be 1, but,
we don't have a valid path -> the dereferencing pgpath might not be
smart.

From: Lars Marowsky-Bree <lmb@suse.de>
Subject: dm-mpath calling pg_init could race with destructor
References: 88654

Need to make sure no pg_init is in-flight before the destructor is
called.

Index: linux-2.6.5/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.5.orig/drivers/md/dm-mpath.c	2005-06-14 21:36:12.514021267 +0200
+++ linux-2.6.5/drivers/md/dm-mpath.c	2005-06-15 09:07:57.556654693 +0200
@@ -388,6 +388,7 @@ static void process_queued_ios(void *dat
 	struct pgpath *pgpath;
 	unsigned init_required, must_queue = 0;
 	unsigned long flags;
+	unsigned queue_size;
 
 	spin_lock_irqsave(&m->lock, flags);
 
@@ -400,15 +401,30 @@ static void process_queued_ios(void *dat
 	    (!pgpath && m->queue_if_no_path && !m->suspended))
 		must_queue = 1;
 
-	if (m->pg_init_required && !m->pg_init_in_progress) {
+	if (m->pg_init_required && !m->pg_init_in_progress && pgpath) {
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
 	} else
 		init_required = 0;
+
+	if (m->pg_init_in_progress)
+		must_queue = 1;
+
+	/* No point in doing anything if we don't have any queued IO.
+	 * This also closes a race: As our internal pg_init is not
+	 * accounted for in the DM pending count, we need to make sure
+	 * we only issue it if we know the pending > 0, or else
+	 * the destructor might be called and dm_pg_init_complete()
+	 * might panic.
+	 */
+	queue_size = m->queue_size;
 	
 	spin_unlock_irqrestore(&m->lock, flags);
 
+	if (queue_size == 0)
+		return;
+
 	if (init_required)
 		hwh->type->pg_init(hwh, pgpath->pg->bypassed, &pgpath->path);
 

Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business	 -- Charles Darwin
"Ignorance more frequently begets confidence than does knowledge"

Re: [patch] dm-mpath pg_init could race with destructor

[Mike Christie]

Lars Marowsky-Bree wrote:
> Why send a pg_init request at this point if there are no queued ios?
> This was in fact my solution, to only send a pg_init io if there are
> queued ios.
> 

Are there management situations where some admin wants to manually 
switch. If he was going to take a controller temporarily offline for 
maitnenance for example? Will multipath tools and dm-mutlipath be used 
to do this type of management, or will it be some vendor specific tool 
that sends a SG_IO directly?

It would not be possible to just have dm-multipath increment some 
refcount like pending so you cannot destroy things while a pg_init IO is 
in flight would it?

Re: [patch] dm-mpath pg_init could race with destructor

[Lars Marowsky-Bree]

On 2005-06-15T13:21:34, Mike Christie <michaelc@cs.wisc.edu> wrote:

> >Why send a pg_init request at this point if there are no queued ios?
> >This was in fact my solution, to only send a pg_init io if there are
> >queued ios.
> >
> Are there management situations where some admin wants to manually 
> switch. If he was going to take a controller temporarily offline for 
> maitnenance for example? Will multipath tools and dm-mutlipath be used 
> to do this type of management, or will it be some vendor specific tool 
> that sends a SG_IO directly?

I don't think that's a real problem. We will switch the group, but just
on the first IO.

That had already happened anyway (because otherwise no
process_queued_ios() would have been queued). The only place where this
really makes a difference is the queue_work() in presuspend: here, a
process_queued_ios() would be qeueued even if no IO was pending, which
could lead to a pg_init being issued, and then the object being free'd -
bang.

> It would not be possible to just have dm-multipath increment some 
> refcount like pending so you cannot destroy things while a pg_init IO is 
> in flight would it?

See my other mails.

The problem is indeed that the DM multipath's internal operations are
not accounted for in the top-level struct mapped_device pending field.
However, trying to get at that from within dm-mpath.c is non-trivial and
a major conceptual change, it appears.

It's sort-of related to "try to give a sane log message from within
dm-mpath, which actually mentions the map / dm dev we belong to".  The
trigger_event() "fix" is also very much related.

The layering is pretty well isolated, which is good, but can make such
changes difficult.

Long-term, that fix will be needed: DM personalities need to be more
involved in their reference counting. 


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business	 -- Charles Darwin
"Ignorance more frequently begets confidence than does knowledge"