All of lore.kernel.org
 help / color / mirror / Atom feed
* multipathd: move 'filter_devnode' under vector lock
@ 2016-05-11 10:35 Hannes Reinecke
  2016-05-11 17:18 ` Benjamin Marzinski
  0 siblings, 1 reply; 3+ messages in thread
From: Hannes Reinecke @ 2016-05-11 10:35 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: Hannes Reinecke, dm-devel

Ben Marzinski pointed out that filter_devnode() is used
without any lock or configuration settings in uev_trigger(),
and hence might be invalid when processing events during
reconfiguration.
So move it into the individual functions and handle it
with the vector lock held.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 multipathd/main.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 58e8854..2c7486d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -580,6 +580,11 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
 	lock(vecs->lock);
 	pthread_testcancel();
+	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
+			   uev->kernel) > 0) {
+		ret = 0;
+		goto out_unlock;
+	}
 	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
 	if (pp) {
 		int r;
@@ -637,6 +642,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
 		free_path(pp);
 		ret = 1;
 	}
+out_unlock:
 	lock_cleanup_pop(vecs->lock);
 	return ret;
 }
@@ -780,22 +786,23 @@ fail:
 static int
 uev_remove_path (struct uevent *uev, struct vectors * vecs)
 {
-	struct path *pp;
-	int ret;
+	struct path *pp = NULL;
+	int ret = 0;
 
 	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;
+	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
+			   uev->kernel) == 0) {
+		pp = find_path_by_dev(vecs->pathvec, uev->kernel);
+		if (pp)
+			ret = ev_remove_path(pp, vecs);
+		else
+			/* Not an error; path might have been purged earlier */
+			condlog(0, "%s: path already removed", uev->kernel);
 	}
+	lock_cleanup_pop(vecs->lock);
 	return ret;
 }
 
@@ -905,7 +912,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 	ro = uevent_get_disk_ro(uev);
 
 	if (ro >= 0) {
-		struct path * pp;
+		struct path * pp = NULL;
 		struct multipath *mpp = NULL;
 
 		condlog(2, "%s: update path write_protect to '%d' (uevent)",
@@ -918,6 +925,10 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 		 * need to be at the same indentation level, hence
 		 * this slightly convoluted codepath.
 		 */
+		if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
+				   uev->kernel) > 0) {
+			goto out_unlock;
+		}
 		pp = find_path_by_dev(vecs->pathvec, uev->kernel);
 		if (pp) {
 			if (pp->initialized == INIT_REQUESTED_UDEV) {
@@ -937,11 +948,13 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 					uev->kernel, mpp->alias, retval);
 			}
 		}
+	out_unlock:
 		lock_cleanup_pop(vecs->lock);
 		if (!pp) {
-			condlog(0, "%s: spurious uevent, path not found",
-				uev->kernel);
-			return 1;
+			if (retval)
+				condlog(0, "%s: spurious uevent, path not found",
+					uev->kernel);
+			return retval;
 		}
 		if (retval == 2)
 			return uev_add_path(uev, vecs);
@@ -1059,10 +1072,6 @@ uev_trigger (struct uevent * uev, void * trigger_data)
 	/*
 	 * path add/remove event
 	 */
-	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
-			   uev->kernel) > 0)
-		goto out;
-
 	if (!strncmp(uev->action, "add", 3)) {
 		r = uev_add_path(uev, vecs);
 		goto out;
-- 
2.6.6

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: multipathd: move 'filter_devnode' under vector lock
  2016-05-11 10:35 multipathd: move 'filter_devnode' under vector lock Hannes Reinecke
@ 2016-05-11 17:18 ` Benjamin Marzinski
  2016-05-11 17:36   ` Christophe Varoqui
  0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Marzinski @ 2016-05-11 17:18 UTC (permalink / raw)
  To: Hannes Reinecke; +Cc: dm-devel, Hannes Reinecke, Christophe Varoqui

On Wed, May 11, 2016 at 12:35:41PM +0200, Hannes Reinecke wrote:
> Ben Marzinski pointed out that filter_devnode() is used
> without any lock or configuration settings in uev_trigger(),
> and hence might be invalid when processing events during
> reconfiguration.
> So move it into the individual functions and handle it
> with the vector lock held.

ACK

-Ben

> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  multipathd/main.c | 45 +++++++++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 58e8854..2c7486d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -580,6 +580,11 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
>  	pthread_cleanup_push(cleanup_lock, &vecs->lock);
>  	lock(vecs->lock);
>  	pthread_testcancel();
> +	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
> +			   uev->kernel) > 0) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
>  	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
>  	if (pp) {
>  		int r;
> @@ -637,6 +642,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs)
>  		free_path(pp);
>  		ret = 1;
>  	}
> +out_unlock:
>  	lock_cleanup_pop(vecs->lock);
>  	return ret;
>  }
> @@ -780,22 +786,23 @@ fail:
>  static int
>  uev_remove_path (struct uevent *uev, struct vectors * vecs)
>  {
> -	struct path *pp;
> -	int ret;
> +	struct path *pp = NULL;
> +	int ret = 0;
>  
>  	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;
> +	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
> +			   uev->kernel) == 0) {
> +		pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> +		if (pp)
> +			ret = ev_remove_path(pp, vecs);
> +		else
> +			/* Not an error; path might have been purged earlier */
> +			condlog(0, "%s: path already removed", uev->kernel);
>  	}
> +	lock_cleanup_pop(vecs->lock);
>  	return ret;
>  }
>  
> @@ -905,7 +912,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
>  	ro = uevent_get_disk_ro(uev);
>  
>  	if (ro >= 0) {
> -		struct path * pp;
> +		struct path * pp = NULL;
>  		struct multipath *mpp = NULL;
>  
>  		condlog(2, "%s: update path write_protect to '%d' (uevent)",
> @@ -918,6 +925,10 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
>  		 * need to be at the same indentation level, hence
>  		 * this slightly convoluted codepath.
>  		 */
> +		if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
> +				   uev->kernel) > 0) {
> +			goto out_unlock;
> +		}
>  		pp = find_path_by_dev(vecs->pathvec, uev->kernel);
>  		if (pp) {
>  			if (pp->initialized == INIT_REQUESTED_UDEV) {
> @@ -937,11 +948,13 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
>  					uev->kernel, mpp->alias, retval);
>  			}
>  		}
> +	out_unlock:
>  		lock_cleanup_pop(vecs->lock);
>  		if (!pp) {
> -			condlog(0, "%s: spurious uevent, path not found",
> -				uev->kernel);
> -			return 1;
> +			if (retval)
> +				condlog(0, "%s: spurious uevent, path not found",
> +					uev->kernel);
> +			return retval;
>  		}
>  		if (retval == 2)
>  			return uev_add_path(uev, vecs);
> @@ -1059,10 +1072,6 @@ uev_trigger (struct uevent * uev, void * trigger_data)
>  	/*
>  	 * path add/remove event
>  	 */
> -	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
> -			   uev->kernel) > 0)
> -		goto out;
> -
>  	if (!strncmp(uev->action, "add", 3)) {
>  		r = uev_add_path(uev, vecs);
>  		goto out;
> -- 
> 2.6.6

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: multipathd: move 'filter_devnode' under vector lock
  2016-05-11 17:18 ` Benjamin Marzinski
@ 2016-05-11 17:36   ` Christophe Varoqui
  0 siblings, 0 replies; 3+ messages in thread
From: Christophe Varoqui @ 2016-05-11 17:36 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: device-mapper development, Hannes Reinecke


[-- Attachment #1.1: Type: text/plain, Size: 5094 bytes --]

Merged

On Wed, May 11, 2016 at 7:18 PM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> On Wed, May 11, 2016 at 12:35:41PM +0200, Hannes Reinecke wrote:
> > Ben Marzinski pointed out that filter_devnode() is used
> > without any lock or configuration settings in uev_trigger(),
> > and hence might be invalid when processing events during
> > reconfiguration.
> > So move it into the individual functions and handle it
> > with the vector lock held.
>
> ACK
>
> -Ben
>
> >
> > Signed-off-by: Hannes Reinecke <hare@suse.com>
> > ---
> >  multipathd/main.c | 45 +++++++++++++++++++++++++++------------------
> >  1 file changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 58e8854..2c7486d 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -580,6 +580,11 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> >       pthread_cleanup_push(cleanup_lock, &vecs->lock);
> >       lock(vecs->lock);
> >       pthread_testcancel();
> > +     if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
> > +                        uev->kernel) > 0) {
> > +             ret = 0;
> > +             goto out_unlock;
> > +     }
> >       pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> >       if (pp) {
> >               int r;
> > @@ -637,6 +642,7 @@ uev_add_path (struct uevent *uev, struct vectors *
> vecs)
> >               free_path(pp);
> >               ret = 1;
> >       }
> > +out_unlock:
> >       lock_cleanup_pop(vecs->lock);
> >       return ret;
> >  }
> > @@ -780,22 +786,23 @@ fail:
> >  static int
> >  uev_remove_path (struct uevent *uev, struct vectors * vecs)
> >  {
> > -     struct path *pp;
> > -     int ret;
> > +     struct path *pp = NULL;
> > +     int ret = 0;
> >
> >       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;
> > +     if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
> > +                        uev->kernel) == 0) {
> > +             pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> > +             if (pp)
> > +                     ret = ev_remove_path(pp, vecs);
> > +             else
> > +                     /* Not an error; path might have been purged
> earlier */
> > +                     condlog(0, "%s: path already removed",
> uev->kernel);
> >       }
> > +     lock_cleanup_pop(vecs->lock);
> >       return ret;
> >  }
> >
> > @@ -905,7 +912,7 @@ uev_update_path (struct uevent *uev, struct vectors
> * vecs)
> >       ro = uevent_get_disk_ro(uev);
> >
> >       if (ro >= 0) {
> > -             struct path * pp;
> > +             struct path * pp = NULL;
> >               struct multipath *mpp = NULL;
> >
> >               condlog(2, "%s: update path write_protect to '%d'
> (uevent)",
> > @@ -918,6 +925,10 @@ uev_update_path (struct uevent *uev, struct vectors
> * vecs)
> >                * need to be at the same indentation level, hence
> >                * this slightly convoluted codepath.
> >                */
> > +             if (filter_devnode(conf->blist_devnode,
> conf->elist_devnode,
> > +                                uev->kernel) > 0) {
> > +                     goto out_unlock;
> > +             }
> >               pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> >               if (pp) {
> >                       if (pp->initialized == INIT_REQUESTED_UDEV) {
> > @@ -937,11 +948,13 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
> >                                       uev->kernel, mpp->alias, retval);
> >                       }
> >               }
> > +     out_unlock:
> >               lock_cleanup_pop(vecs->lock);
> >               if (!pp) {
> > -                     condlog(0, "%s: spurious uevent, path not found",
> > -                             uev->kernel);
> > -                     return 1;
> > +                     if (retval)
> > +                             condlog(0, "%s: spurious uevent, path not
> found",
> > +                                     uev->kernel);
> > +                     return retval;
> >               }
> >               if (retval == 2)
> >                       return uev_add_path(uev, vecs);
> > @@ -1059,10 +1072,6 @@ uev_trigger (struct uevent * uev, void *
> trigger_data)
> >       /*
> >        * path add/remove event
> >        */
> > -     if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
> > -                        uev->kernel) > 0)
> > -             goto out;
> > -
> >       if (!strncmp(uev->action, "add", 3)) {
> >               r = uev_add_path(uev, vecs);
> >               goto out;
> > --
> > 2.6.6
>

[-- Attachment #1.2: Type: text/html, Size: 7033 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-05-11 17:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-11 10:35 multipathd: move 'filter_devnode' under vector lock Hannes Reinecke
2016-05-11 17:18 ` Benjamin Marzinski
2016-05-11 17:36   ` Christophe Varoqui

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.