Re: [PATCH 56/57] multipathd: push down lock in checkerloop()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 03, 2016 at 05:17:34PM -0500, Benjamin Marzinski wrote:
> On Wed, Apr 27, 2016 at 01:10:57PM +0200, Hannes Reinecke wrote:
> > Instead of grabbing the lock at the start of the checkerloop
> > and releasing it at the end we should be holding it only
> > during the time when we actually need it.
> 
> I'm pretty sure that this can cause crashes if multipathd reconfigures
> when it's just started servicing a uevent. It goes like this. The
> uevq_thr thread calls uev_trigger(), which checks the running_state and
> sees that it's DAEMON_IDLE.  The uxlsnr_thr thread gets a
> reconfiguration request, which eventually calls set_config_state().
> set_config_state() sees the state is DAEMON_IDLE, sets it to
> DAEMON_CONFIGURE, and returns without waiting.  This wakes up the child
> thread, which runs reconfigure().  reconfigure() sets conf to NULL. Then
> the uevq_thr thread in uev_trigger() calls filter_devnode()
> dereferencing conf->blist_devnode and conf->elist_devnode. conf is NULL,
> and thus it crashes.
> 
> The short version is that multipathd has been using the vecs lock to
> protect conf.  With your change to uev_trigger, you access conf outside
> of the vecs lock. This isn't a problem in checkerloop(), since you can't
> reconfigure while multipathd is in the DAEMON_RUNNING state, so that is
> protecting conf. You need to do the same
> set_config_state(DAEMON_RUNNING) in uev_trigger to make this safe.

Or not. ev_add_map calls set_config_state(DAEMON_CONFIGURE), so this
would deadlock if you called  set_config_state(DAEMON_RUNNING) in
uev_trigger. At any rate, either reconfigures need to be blocked while
running uev_trigger, or we need some sort of reference counting on the
config structure so that we don't free it when we do a reconfigure,
until everyone has switched to the new structure.

-Ben
> 
> -Ben
> 
> > 
> > Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> > ---
> >  multipathd/main.c | 136 ++++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 87 insertions(+), 49 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 41b5a49..132101f 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -416,7 +416,11 @@ uev_add_map (struct uevent * uev, struct vectors * vecs)
> >  			return 1;
> >  		}
> >  	}
> > +	pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +	lock(vecs->lock);
> > +	pthread_testcancel();
> >  	rc = ev_add_map(uev->kernel, alias, vecs);
> > +	lock_cleanup_pop(vecs->lock);
> >  	FREE(alias);
> >  	return rc;
> >  }
> > @@ -512,6 +516,10 @@ uev_remove_map (struct uevent * uev, struct vectors * vecs)
> >  		return 0;
> >  	}
> >  	minor = uevent_get_minor(uev);
> > +
> > +	pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +	lock(vecs->lock);
> > +	pthread_testcancel();
> >  	mpp = find_mp_by_minor(vecs->mpvec, minor);
> >  
> >  	if (!mpp) {
> > @@ -528,10 +536,12 @@ uev_remove_map (struct uevent * uev, struct vectors * vecs)
> >  	orphan_paths(vecs->pathvec, mpp);
> >  	remove_map_and_stop_waiter(mpp, vecs, 1);
> >  out:
> > +	lock_cleanup_pop(vecs->lock);
> >  	FREE(alias);
> >  	return 0;
> >  }
> >  
> > +/* Called from CLI handler */
> >  int
> >  ev_remove_map (char * devname, char * alias, int minor, struct vectors * vecs)
> >  {
> > @@ -567,6 +577,9 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
> >  		return 1;
> >  	}
> >  
> > +	pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +	lock(vecs->lock);
> > +	pthread_testcancel();
> >  	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> >  	if (pp) {
> >  		int r;
> > @@ -594,8 +607,10 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
> >  				ret = 1;
> >  			}
> >  		}
> > -		return ret;
> >  	}
> > +	lock_cleanup_pop(vecs->lock);
> > +	if (pp)
> > +		return ret;
> >  
> >  	/*
> >  	 * get path vital state
> > @@ -608,6 +623,9 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
> >  		condlog(3, "%s: failed to get path info", uev->kernel);
> >  		return 1;
> >  	}
> > +	pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +	lock(vecs->lock);
> > +	pthread_testcancel();
> >  	ret = store_path(vecs->pathvec, pp);
> >  	if (!ret) {
> >  		pp->checkint = conf->checkint;
> > @@ -619,7 +637,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
> >  		free_path(pp);
> >  		ret = 1;
> >  	}
> > -
> > +	lock_cleanup_pop(vecs->lock);
> >  	return ret;
> >  }
> >  
> > @@ -687,12 +705,12 @@ rescan:
> >  			 */
> >  			start_waiter = 1;
> >  		}
> > -		else
> > +		if (!start_waiter)
> >  			goto fail; /* leave path added to pathvec */
> >  	}
> >  
> > -	/* persistent reseravtion check*/
> > -	mpath_pr_event_handle(pp);	
> > +	/* persistent reservation check*/
> > +	mpath_pr_event_handle(pp);
> >  
> >  	/*
> >  	 * push the map to the device-mapper
> > @@ -720,7 +738,7 @@ retry:
> >  		 * deal with asynchronous uevents :((
> >  		 */
> >  		if (mpp->action == ACT_RELOAD && retries-- > 0) {
> > -			condlog(0, "%s: uev_add_path sleep", mpp->alias);
> > +			condlog(0, "%s: ev_add_path sleep", mpp->alias);
> >  			sleep(1);
> >  			update_mpp_paths(mpp, vecs->pathvec);
> >  			goto rescan;
> > @@ -749,8 +767,7 @@ retry:
> >  		condlog(2, "%s [%s]: path added to devmap %s",
> >  			pp->dev, pp->dev_t, mpp->alias);
> >  		return 0;
> > -	}
> > -	else
> > +	} else
> >  		goto fail;
> >  
> >  fail_map:
> > @@ -764,17 +781,22 @@ static int
> >  uev_remove_path (struct uevent *uev, struct vectors * vecs)
> >  {
> >  	struct path *pp;
> > +	int ret;
> >  
> >  	condlog(2, "%s: remove path (uevent)", uev->kernel);
> > +	pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +	lock(vecs->lock);
> > +	pthread_testcancel();
> >  	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> > -
> > +	if (pp)
> > +		ret = ev_remove_path(pp, vecs);
> > +	lock_cleanup_pop(vecs->lock);
> >  	if (!pp) {
> >  		/* Not an error; path might have been purged earlier */
> >  		condlog(0, "%s: path already removed", uev->kernel);
> >  		return 0;
> >  	}
> > -
> > -	return ev_remove_path(pp, vecs);
> > +	return ret;
> >  }
> >  
> >  int
> > @@ -877,35 +899,50 @@ static int
> >  uev_update_path (struct uevent *uev, struct vectors * vecs)
> >  {
> >  	int ro, retval = 0;
> > -	struct path * pp;
> > -
> > -	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> > -	if (!pp) {
> > -		condlog(0, "%s: spurious uevent, path not found",
> > -			uev->kernel);
> > -		return 1;
> > -	}
> > -
> > -	if (pp->initialized == INIT_REQUESTED_UDEV)
> > -		return uev_add_path(uev, vecs);
> >  
> >  	ro = uevent_get_disk_ro(uev);
> >  
> >  	if (ro >= 0) {
> > +		struct path * pp;
> > +		struct multipath *mpp = NULL;
> > +
> >  		condlog(2, "%s: update path write_protect to '%d' (uevent)",
> >  			uev->kernel, ro);
> > -		if (pp->mpp) {
> > -			if (pp->mpp->wait_for_udev) {
> > -				pp->mpp->wait_for_udev = 2;
> > -				return 0;
> > +		pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +		lock(vecs->lock);
> > +		pthread_testcancel();
> > +		/*
> > +		 * pthread_mutex_lock() and pthread_mutex_unlock()
> > +		 * need to be at the same indentation level, hence
> > +		 * this slightly convoluted codepath.
> > +		 */
> > +		pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> > +		if (pp) {
> > +			if (pp->initialized == INIT_REQUESTED_UDEV) {
> > +				retval = 2;
> > +			} else {
> > +				mpp = pp->mpp;
> > +				if (mpp && mpp->wait_for_udev) {
> > +					mpp->wait_for_udev = 2;
> > +					mpp = NULL;
> > +					retval = 0;
> > +				}
> >  			}
> > +			if (mpp) {
> > +				retval = reload_map(vecs, mpp, 0);
> >  
> > -			retval = reload_map(vecs, pp->mpp, 0);
> > -
> > -			condlog(2, "%s: map %s reloaded (retval %d)",
> > -				uev->kernel, pp->mpp->alias, retval);
> > +				condlog(2, "%s: map %s reloaded (retval %d)",
> > +					uev->kernel, mpp->alias, retval);
> > +			}
> >  		}
> > -
> > +		lock_cleanup_pop(vecs->lock);
> > +		if (!pp) {
> > +			condlog(0, "%s: spurious uevent, path not found",
> > +				uev->kernel);
> > +			return 1;
> > +		}
> > +		if (retval == 2)
> > +			return uev_add_path(uev, vecs);
> >  	}
> >  
> >  	return retval;
> > @@ -1002,10 +1039,6 @@ uev_trigger (struct uevent * uev, void * trigger_data)
> >  	if (running_state == DAEMON_SHUTDOWN)
> >  		return 0;
> >  
> > -	pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > -	lock(vecs->lock);
> > -	pthread_testcancel();
> > -
> >  	/*
> >  	 * device map event
> >  	 * Add events are ignored here as the tables
> > @@ -1044,7 +1077,6 @@ uev_trigger (struct uevent * uev, void * trigger_data)
> >  	}
> >  
> >  out:
> > -	lock_cleanup_pop(vecs->lock);
> >  	return r;
> >  }
> >  
> > @@ -1627,17 +1659,6 @@ checkerloop (void *ap)
> >  
> >  		if (gettimeofday(&start_time, NULL) != 0)
> >  			start_time.tv_sec = 0;
> > -
> > -		rc = set_config_state(DAEMON_RUNNING);
> > -		if (rc == ETIMEDOUT) {
> > -			condlog(4, "timeout waiting for DAEMON_IDLE");
> > -			continue;
> > -		}
> > -
> > -		pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > -		lock(vecs->lock);
> > -		pthread_testcancel();
> > -		strict_timing = conf->strict_timing;
> >  		if (start_time.tv_sec && last_time.tv_sec) {
> >  			timersub(&start_time, &last_time, &diff_time);
> >  			condlog(4, "tick (%lu.%06lu secs)",
> > @@ -1653,28 +1674,45 @@ checkerloop (void *ap)
> >  		if (use_watchdog)
> >  			sd_notify(0, "WATCHDOG=1");
> >  #endif
> > +		rc = set_config_state(DAEMON_RUNNING);
> > +		if (rc == ETIMEDOUT) {
> > +			condlog(4, "timeout waiting for DAEMON_IDLE");
> > +			continue;
> > +		}
> > +		strict_timing = conf->strict_timing;
> >  		if (vecs->pathvec) {
> > +			pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +			lock(vecs->lock);
> > +			pthread_testcancel();
> >  			vector_foreach_slot (vecs->pathvec, pp, i) {
> >  				num_paths += check_path(vecs, pp, ticks);
> >  			}
> > +			lock_cleanup_pop(vecs->lock);
> >  		}
> >  		if (vecs->mpvec) {
> > +			pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +			lock(vecs->lock);
> > +			pthread_testcancel();
> >  			defered_failback_tick(vecs->mpvec);
> >  			retry_count_tick(vecs->mpvec);
> >  			missing_uev_wait_tick(vecs);
> > +			lock_cleanup_pop(vecs->lock);
> >  		}
> >  		if (count)
> >  			count--;
> >  		else {
> > +			pthread_cleanup_push(cleanup_lock, &vecs->lock);
> > +			lock(vecs->lock);
> > +			pthread_testcancel();
> >  			condlog(4, "map garbage collection");
> >  			mpvec_garbage_collector(vecs);
> >  			count = MAPGCINT;
> > +			lock_cleanup_pop(vecs->lock);
> >  		}
> >  
> > -		lock_cleanup_pop(vecs->lock);
> >  		diff_time.tv_usec = 0;
> >  		if (start_time.tv_sec &&
> > -		    gettimeofday(&end_time, NULL)) {
> > +		    gettimeofday(&end_time, NULL) == 0) {
> >  			timersub(&end_time, &start_time, &diff_time);
> >  			if (num_paths) {
> >  				condlog(3, "checked %d path%s in %lu.%06lu secs",
> > -- 
> > 2.6.6
> 
> --
> dm-devel mailing list
> dm-devel@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/dm-devel

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



[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux