On Mon, Feb 01, 2021 at 11:26:36PM -0600, Benjamin Marzinski wrote:
> On Mon, Feb 01, 2021 at 04:12:34PM +0100, Martin Wilck wrote:
> > On Mon, 2021-02-01 at 22:50 +0800, lixiaokeng wrote:
> > >
> > > > >
> > > > > cli_add_path
> > > > > ->ev_add_path
> > > > > ->add_map_with_path
> > > > > ->adopt_paths
> > > > > ->pathinfo
> > > > > ->filter_property
> > > > > ->return PATHINFO_SKIPPED,
> > > > > ->pp->mpp is NULL and not be set
> > > > > ->return 0
> > > >
> > > > This returns 0, but add_map_with_path() has this code to check
> > > > whether
> > > > the path passed to it was actually added to the new map:
> > > >
> > > > if (adopt_paths(vecs->pathvec, mpp) ||
> > > > find_slot(vecs->pathvec, pp) == -1)
> > > > goto out; -> return NULL
> > > >
> > > > So ev_add_path() should have seen a NULL return from
> > > > add_map_with_path(), should not have set start_waiter, and failed.
> > > >
> > >
> > > I'm sorry for a big mistake in my stack. As the code is optimized,
> > > pathinfo
> > > return PATHINFO_SKIPPED after finish filter_property when I use gdb.
> > > It
> > > happens acctualy in:
> > > 2141 if (pp->bus == SYSFS_BUS_SCSI &&
> > > 2142 pp->sg_id.proto_id == SCSI_PROTOCOL_USB
> > > &&
> > > 2143 !conf->allow_usb_devices) {
> > > 2144 condlog(3, "%s: skip USB device %s",
> > > pp->dev,
> > > 2145 pp->tgt_node_name);
> > > 2146 return PATHINFO_SKIPPED;
> > > 2147 }
> > > 2148 }
> > >
> > > Stack:
> > > cli_add_path
> > > ->ev_add_path
> > > ->add_map_with_path
> > > ->adopt_paths
> > > ->pathinfo
> > > ->pp->bus == SYSFS_BUS_SCSI
> > > ->return PATHINFO_SKIPPED,
> > > ->pp->mpp is NULL and not be set
> > > ->return 0
> > > ->mpath_pr_event_handle
> > > ->get_be64 //pp->mpp is dereference
> > >
> > > If you think my patch is ok, I will resend it.
> >
> > The same argument I made above still holds. "pp" wouldn't have been
> > added to mpp, and add_map_with_path() would fail and return NULL.
> > Also, if pathinfo() returns PATHINFO_SKIPPED for this device,
> > how comes that cli_add_path() called ev_add_path() for it? It should
> > have returned "blacklisted" instead.
>
> So, I think the main issue here is that filter_property appears to be
> broken. It only filters if uid_attribute is set, but that will never be
> set the first time it's called in pathinfo. This means that it will
> pass in the pathinfo call in cli_add_path, and the path will get stored
> in the pathvec.
Just to be a little more clear here, filter_property() will only return
MATCH_PROPERTY_BLIST_MISSING for missing udev properties, if the
uid_attribute is set and seen. We should probably make sure to set
uid_attribute before calling it.
-Ben
>
> However, it will fail in the pathinfo call from adopt_paths, so the path
> won't be added to the multipath device. This means adopt paths doesn't
> actually adopt any paths potentially, but that in itself doesn't cause
> it to fail. This check
>
> if (adopt_paths(vecs->pathvec, mpp) ||
> find_slot(vecs->pathvec, pp) == -1)
> goto out;
>
> passes, since we only check if the path is on the pathvec, not part of
> the multipath device, and since filter_property let the path past the
> first time, it is. So add_map_with_path() will create a multipath
> device, but the path won't be added to it, and pp->mpp == NULL.
>
> So, add_map_with_path() should probably check that we actually created a
> map that included the path that got added. But more importantly,
> filter_property shouldn't return different results the when it's called
> the first time. That would have avoid the entire situation.
>
> -Ben
>
>
> > Your patch would only be effective if it was possible that
> > add_map_with_path(vecs, pp, 1) returned an mpp != NULL, and at the same
> > time pp->mpp was NULL; I still don't understand how that can come to
> > pass.
> >
> > Have you tried my patch?
> >
> > Regards,
> > Martin
> >
> >
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
