From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christophe Varoqui Subject: Re: multipathd: move 'filter_devnode' under vector lock Date: Wed, 11 May 2016 19:36:39 +0200 Message-ID: References: <1462962941-9429-1-git-send-email-hare@suse.de> <20160511171839.GC26117@octiron.msp.redhat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6157541445626531926==" Return-path: In-Reply-To: <20160511171839.GC26117@octiron.msp.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Benjamin Marzinski Cc: device-mapper development , Hannes Reinecke List-Id: dm-devel.ids --===============6157541445626531926== Content-Type: multipart/alternative; boundary=001a11411da0f366c50532947b4b --001a11411da0f366c50532947b4b Content-Type: text/plain; charset=UTF-8 Merged On Wed, May 11, 2016 at 7:18 PM, Benjamin Marzinski 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 > > --- > > 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 > --001a11411da0f366c50532947b4b Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Merged

On Wed, May 11, 2016 at 7:18 PM, Benjamin Marzinski <bmarz= ins@redhat.com> wrote:
On Wed, May 11, 2016 at 12:35:41PM +0200, Hannes Reinecke wrot= e:
> 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 <ha= re@suse.com>
> ---
>=C2=A0 multipathd/main.c | 45 +++++++++++++++++++++++++++--------------= ----
>=C2=A0 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)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_cleanup_push(cleanup_lock, &vecs= ->lock);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0lock(vecs->lock);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_testcancel();
> +=C2=A0 =C2=A0 =C2=A0if (filter_devnode(conf->blist_devnode, conf-&= gt;elist_devnode,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 uev->kernel) > 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out_unlock;
> +=C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pp =3D find_path_by_dev(vecs->pathvec, ue= v->kernel);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (pp) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int r;
> @@ -637,6 +642,7 @@ uev_add_path (struct uevent *uev, struct vectors *= vecs)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0free_path(pp); >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D 1;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +out_unlock:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0lock_cleanup_pop(vecs->lock);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
>=C2=A0 }
> @@ -780,22 +786,23 @@ fail:
>=C2=A0 static int
>=C2=A0 uev_remove_path (struct uevent *uev, struct vectors * vecs)
>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0struct path *pp;
> -=C2=A0 =C2=A0 =C2=A0int ret;
> +=C2=A0 =C2=A0 =C2=A0struct path *pp =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0int ret =3D 0;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0condlog(2, "%s: remove path (uevent)&qu= ot;, uev->kernel);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_cleanup_push(cleanup_lock, &vecs= ->lock);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0lock(vecs->lock);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_testcancel();
> -=C2=A0 =C2=A0 =C2=A0pp =3D find_path_by_dev(vecs->pathvec, uev->= ;kernel);
> -=C2=A0 =C2=A0 =C2=A0if (pp)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D ev_remove_pat= h(pp, vecs);
> -=C2=A0 =C2=A0 =C2=A0lock_cleanup_pop(vecs->lock);
> -=C2=A0 =C2=A0 =C2=A0if (!pp) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Not an error; path= might have been purged earlier */
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0condlog(0, "%s: = path already removed", uev->kernel);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
> +=C2=A0 =C2=A0 =C2=A0if (filter_devnode(conf->blist_devnode, conf-&= gt;elist_devnode,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 uev->kernel) =3D=3D 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pp =3D find_path_by_d= ev(vecs->pathvec, uev->kernel);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (pp)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0ret =3D ev_remove_path(pp, vecs);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0/* Not an error; path might have been purged earlier */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0condlog(0, "%s: path already removed", uev->kernel); >=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0lock_cleanup_pop(vecs->lock);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
>=C2=A0 }
>
> @@ -905,7 +912,7 @@ uev_update_path (struct uevent *uev, struct vector= s * vecs)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0ro =3D uevent_get_disk_ro(uev);
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ro >=3D 0) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct path * pp;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct path * pp =3D = NULL;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct multipath= *mpp =3D NULL;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0condlog(2, "= ;%s: update path write_protect to '%d' (uevent)",
> @@ -918,6 +925,10 @@ uev_update_path (struct uevent *uev, struct vecto= rs * vecs)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * need to be at= the same indentation level, hence
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * this slightly= convoluted codepath.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (filter_devnode(co= nf->blist_devnode, conf->elist_devnode,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 uev->kernel) > 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0goto out_unlock;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pp =3D find_path= _by_dev(vecs->pathvec, uev->kernel);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (pp) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0if (pp->initialized =3D=3D INIT_REQUESTED_UDEV) {
> @@ -937,11 +948,13 @@ uev_update_path (struct uevent *uev, struct vect= ors * vecs)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uev-&g= t;kernel, mpp->alias, retval);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0out_unlock:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lock_cleanup_pop= (vecs->lock);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!pp) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0condlog(0, "%s: spurious uevent, path not found",
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uev->kernel);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return 1;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (retval)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0condlog(0, "%s: spurious uevent, pa= th not found",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uev->kern= el);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return retval;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (retval =3D= =3D 2)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0return uev_add_path(uev, vecs);
> @@ -1059,10 +1072,6 @@ uev_trigger (struct uevent * uev, void * trigge= r_data)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * path add/remove event
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> -=C2=A0 =C2=A0 =C2=A0if (filter_devnode(conf->blist_devnode, conf-&= gt;elist_devnode,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 uev->kernel) > 0)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
> -
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!strncmp(uev->action, "add"= , 3)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r =3D uev_add_pa= th(uev, vecs);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
> --
> 2.6.6

--001a11411da0f366c50532947b4b-- --===============6157541445626531926== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============6157541445626531926==--