[dm-devel] [PATCH 56/57] multipathd: push down lock in checkerloop()
Benjamin Marzinski
bmarzins at redhat.com
Tue May 3 22:17:34 UTC 2016
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.
-Ben
>
> Signed-off-by: Hannes Reinecke <hare at suse.de>
> ---
> 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
More information about the dm-devel
mailing list