[PATCH 0/6] libmultipath: check udev* func return value

[PATCH 0/6] libmultipath: check udev* func return value

[lixiaokeng]

Hi,
  The udev* function may return NULL,and it will be
dereferenced in str* and sscanf func. For example,
there is a coredump caused in add func, which show in
be7a043(commit id) in upstream-queue. We check the
return value to avoid dereference NULL.

repo: openSUSE/multipath-tools
repo link: https://github.com/openSUSE/multipath-tools
branch: upstream-queue

lixiaokeng (6):
  libmultipath: check uedv* return value in sysfs_get_host_pci_name
  libmultipath: check udev* return value in ccw_sysfs_pathinfo
  libmultipath: check udev* return value in sysfs_get_tgt_nodename
  libmultipath: check udev* return value in
    trigger_partitions_udev_change
  libmultipath: check udev* renturn value in get_ctrl_blkdev
  libmultipath: check udev* return value in _find_path_by_syspath

 libmultipath/configure.c    |  4 +++-
 libmultipath/discovery.c    |  9 +++++++--
 libmultipath/foreign/nvme.c | 10 +++++++---
 3 files changed, 17 insertions(+), 6 deletions(-)

[PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name

[lixiaokeng]

udev_device_get_sysname may return NULL. We check the return value.

Signed-off-by:Lixiaokeng<Lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/discovery.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3f1b1d71..4264b0da 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -519,6 +519,9 @@ int sysfs_get_host_pci_name(const struct path *pp, char *pci_name)
 		 */
 		value = udev_device_get_sysname(parent);

+		if (!value)
+			return 1;
+
 		strncpy(pci_name, value, SLOT_NAME_SIZE);
 		udev_device_unref(hostdev);
 		return 0;
-- 

Re: [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name

[Martin Wilck]

On Tue, 2020-09-15 at 12:39 +0800, lixiaokeng wrote:
> udev_device_get_sysname may return NULL. We check the return value.
> 
> Signed-off-by:Lixiaokeng<Lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  libmultipath/discovery.c | 3 +++
>  1 file changed, 3 insertions(+)

The patch is correct, but please watch out for spelling mistakes,
in particular in the subject.

Martin

[PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo

[lixiaokeng]

We check the return value of udev_device_get_sysname.

Signed-off-by:Lixiaokeng<lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/discovery.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 4264b0da..27cb67f8 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1471,6 +1471,8 @@ ccw_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable)
 	 * host / bus / target / lun
 	 */
 	attr_path = udev_device_get_sysname(parent);
+	if (attr_path)
+		return PATHINFO_FAILED;
 	pp->sg_id.lun = 0;
 	if (sscanf(attr_path, "%i.%i.%x",
 		   &pp->sg_id.host_no,
-- 

Re: [PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo

[Martin Wilck]

On Tue, 2020-09-15 at 12:39 +0800, lixiaokeng wrote:
> We check the return value of udev_device_get_sysname.
> 
> Signed-off-by:Lixiaokeng<lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  libmultipath/discovery.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 4264b0da..27cb67f8 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1471,6 +1471,8 @@ ccw_sysfs_pathinfo (struct path *pp, const
> struct _vector *hwtable)
>  	 * host / bus / target / lun
>  	 */
>  	attr_path = udev_device_get_sysname(parent);
> +	if (attr_path)
> +		return PATHINFO_FAILED;
>  	pp->sg_id.lun = 0;
>  	if (sscanf(attr_path, "%i.%i.%x",
>  		   &pp->sg_id.host_no,

This looks wrong.

[PATCH 3/6] libmultipath: check udev* return value in sysfs_get_tgt_nodename

[lixiaokeng]

We check the return value of udev_device_get_parent and
udev_device_get_sysname.

Signed-off-by:lixiaokeng<lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/discovery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 27cb67f8..c490dddb 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -356,7 +356,7 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
 		tgtdev = udev_device_get_parent(parent);
 		while (tgtdev) {
 			tgtname = udev_device_get_sysname(tgtdev);
-			if (sscanf(tgtname, "end_device-%d:%d",
+			if (tgtname && sscanf(tgtname, "end_device-%d:%d",
 				   &host, &tgtid) == 2)
 				break;
 			tgtdev = udev_device_get_parent(tgtdev);
@@ -389,7 +389,7 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
 	/* Check for FibreChannel */
 	tgtdev = udev_device_get_parent(parent);
 	value = udev_device_get_sysname(tgtdev);
-	if (sscanf(value, "rport-%d:%d-%d",
+	if (value && sscanf(value, "rport-%d:%d-%d",
 		   &host, &channel, &tgtid) == 3) {
 		tgtdev = udev_device_new_from_subsystem_sysname(udev,
 				"fc_remote_ports", value);
-- 

[PATCH 4/6] libmultipath: check udev* return value in trigger_partitions_udev_change

[lixiaokeng]

We check the return value  of udev_device_get_devtype before
dereference it.

Signed-off-by:Lixiaokeng<Lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/configure.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 972cf5fe..7263707c 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -515,6 +515,7 @@ static void trigger_partitions_udev_change(struct udev_device *dev,
 {
 	struct udev_enumerate *part_enum;
 	struct udev_list_entry *item;
+	const char *devtype;

 	part_enum = udev_enumerate_new(udev);
 	if (!part_enum)
@@ -535,7 +536,8 @@ static void trigger_partitions_udev_change(struct udev_device *dev,
 		if (!part)
 			continue;

-		if (!strcmp("partition", udev_device_get_devtype(part))) {
+		devtype = udev_device_get_devtype(part);
+		if (devtype && !strcmp("partition", devtype)) {
 			condlog(4, "%s: triggering %s event for %s", __func__,
 				action, syspath);
 			sysfs_attr_set_value(part, "uevent", action, len);
-- 

[PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev

[lixiaokeng]

We check return value of udev_device_get_devtype before
dereference it.

Signed-off-by:Lixiaokeng<lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/foreign/nvme.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index 0bc5106e..a0668713 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -538,6 +538,7 @@ struct udev_device *get_ctrl_blkdev(const struct context *ctx,
 	struct udev_list_entry *item;
 	struct udev_device *blkdev = NULL;
 	struct udev_enumerate *enm = udev_enumerate_new(ctx->udev);
+	const char *devtype;

 	if (enm == NULL)
 		return NULL;
@@ -562,7 +563,9 @@ struct udev_device *get_ctrl_blkdev(const struct context *ctx,
 					   udev_list_entry_get_name(item));
 		if (tmp == NULL)
 			continue;
-		if (!strcmp(udev_device_get_devtype(tmp), "disk")) {
+
+		devtype = udev_device_get_devtype(tmp);
+		if (!devtype && !strcmp(devtype, "disk")) {
 			blkdev = tmp;
 			break;
 		} else
-- 

Re: [PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev

[Martin Wilck]

On Tue, 2020-09-15 at 12:41 +0800, lixiaokeng wrote:
> We check return value of udev_device_get_devtype before
> dereference it.
> 
> Signed-off-by:Lixiaokeng<lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  libmultipath/foreign/nvme.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/foreign/nvme.c
> b/libmultipath/foreign/nvme.c
> index 0bc5106e..a0668713 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -538,6 +538,7 @@ struct udev_device *get_ctrl_blkdev(const struct
> context *ctx,
>  	struct udev_list_entry *item;
>  	struct udev_device *blkdev = NULL;
>  	struct udev_enumerate *enm = udev_enumerate_new(ctx->udev);
> +	const char *devtype;
> 
>  	if (enm == NULL)
>  		return NULL;
> @@ -562,7 +563,9 @@ struct udev_device *get_ctrl_blkdev(const struct
> context *ctx,
>  					   udev_list_entry_get_name(ite
> m));
>  		if (tmp == NULL)
>  			continue;
> -		if (!strcmp(udev_device_get_devtype(tmp), "disk")) {
> +
> +		devtype = udev_device_get_devtype(tmp);
> +		if (!devtype && !strcmp(devtype, "disk")) {
>  			blkdev = tmp;
>  			break;
>  		} else

This looks wrong.

[PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath

[lixiaokeng]

We check udev_device_get_syspath return value before
dereference it.

Signed-off-by: Lixiaokeng<lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Linfeilong <linfeilong@huawei.com>
---
 libmultipath/foreign/nvme.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index a0668713..28e0d192 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -482,6 +482,7 @@ _find_path_by_syspath(struct nvme_map *map, const char *syspath)
 	struct nvme_pathgroup *pg;
 	char real[PATH_MAX];
 	const char *ppath;
+	const char *psyspath;
 	int i;

 	ppath = realpath(syspath, real);
@@ -493,8 +494,8 @@ _find_path_by_syspath(struct nvme_map *map, const char *syspath)
 	vector_foreach_slot(&map->pgvec, pg, i) {
 		struct nvme_path *path = nvme_pg_to_path(pg);

-		if (!strcmp(ppath,
-			    udev_device_get_syspath(path->udev)))
+		psyspath = udev_device_get_syspath(path->udev);
+		if (!psyspath && !strcmp(ppath, psyspath))
 			return path;
 	}
 	condlog(4, "%s: %s: %s not found", __func__, THIS, ppath);
-- 

Re: [PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath

[Martin Wilck]

On Tue, 2020-09-15 at 12:42 +0800, lixiaokeng wrote:
> We check udev_device_get_syspath return value before
> dereference it.
> 
> Signed-off-by: Lixiaokeng<lixiaokeng@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> ---
>  libmultipath/foreign/nvme.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/foreign/nvme.c
> b/libmultipath/foreign/nvme.c
> index a0668713..28e0d192 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -482,6 +482,7 @@ _find_path_by_syspath(struct nvme_map *map, const
> char *syspath)
>  	struct nvme_pathgroup *pg;
>  	char real[PATH_MAX];
>  	const char *ppath;
> +	const char *psyspath;
>  	int i;
> 
>  	ppath = realpath(syspath, real);
> @@ -493,8 +494,8 @@ _find_path_by_syspath(struct nvme_map *map, const
> char *syspath)
>  	vector_foreach_slot(&map->pgvec, pg, i) {
>  		struct nvme_path *path = nvme_pg_to_path(pg);
> 
> -		if (!strcmp(ppath,
> -			    udev_device_get_syspath(path->udev)))
> +		psyspath = udev_device_get_syspath(path->udev);
> +		if (!psyspath && !strcmp(ppath, psyspath))
>  			return path;
>  	}
>  	condlog(4, "%s: %s: %s not found", __func__, THIS, ppath);

This looks wrong to me.

Re: [PATCH 0/6] libmultipath: check udev* func return value

[Benjamin Marzinski]

On Tue, Sep 15, 2020 at 12:38:27PM +0800, lixiaokeng wrote:
> Hi,
>   The udev* function may return NULL,and it will be
> dereferenced in str* and sscanf func. For example,
> there is a coredump caused in add func, which show in
> be7a043(commit id) in upstream-queue. We check the
> return value to avoid dereference NULL.
> 
> repo: openSUSE/multipath-tools
> repo link: https://github.com/openSUSE/multipath-tools
> branch: upstream-queue
> 
For the set
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>


> lixiaokeng (6):
>   libmultipath: check uedv* return value in sysfs_get_host_pci_name
>   libmultipath: check udev* return value in ccw_sysfs_pathinfo
>   libmultipath: check udev* return value in sysfs_get_tgt_nodename
>   libmultipath: check udev* return value in
>     trigger_partitions_udev_change
>   libmultipath: check udev* renturn value in get_ctrl_blkdev
>   libmultipath: check udev* return value in _find_path_by_syspath
> 
>  libmultipath/configure.c    |  4 +++-
>  libmultipath/discovery.c    |  9 +++++++--
>  libmultipath/foreign/nvme.c | 10 +++++++---
>  3 files changed, 17 insertions(+), 6 deletions(-)

Re: [PATCH 0/6] libmultipath: check udev* func return value

[Martin Wilck]

On Thu, 2020-09-17 at 21:26 -0500, Benjamin Marzinski wrote:
> On Tue, Sep 15, 2020 at 12:38:27PM +0800, lixiaokeng wrote:
> > Hi,
> >   The udev* function may return NULL,and it will be
> > dereferenced in str* and sscanf func. For example,
> > there is a coredump caused in add func, which show in
> > be7a043(commit id) in upstream-queue. We check the
> > return value to avoid dereference NULL.
> > 
> > repo: openSUSE/multipath-tools
> > repo link: https://github.com/openSUSE/multipath-tools
> > branch: upstream-queue
> > 
> For the set
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

Well, the set got the logic wrong in 3 out of 6 patches :-)

In general, as these fixes are all very similar, I would prefer
bundling them together into one patch, and I would like to make sure
(and assert in the commit message) that the set fixes
*all* possible NULL pointer dereferences related to accessing udev
properties (I haven't checked whether this is the case for the set).

Also, while I'd ack this set if the logic was correct, I think that in
a way it's papering over the actual problem. udev_device_get_XYZ()
functions fail *only* if the underlying sysfs directory has vanished
(well, perhaps for out-of-memory, too, but let's put that aside for a
moment). It that case, we should actually not just return an error code
- we should realize that there's a problem with the device (it probably
has been deleted from the system and shouldn't be used any more in any
way), and that we need to deal with it somehow. This is a fundamental
problem for all user space programs that use udev_devices for more than
a few CPU cycles. That doesn't mean the set is wrong, but we should
keep this in mind. I have something cooking for a more complete
solution, which isn't ready yet.

Finally, @lixiaokeng, please check your inbox once more. You still
haven't fixed #11/14 of your previous series.

Regards,
Martin

Re: [PATCH 0/6] libmultipath: check udev* func return value

[lixiaokeng]

Hi Martin,

On 2020/9/18 15:37, Martin Wilck wrote:
> On Thu, 2020-09-17 at 21:26 -0500, Benjamin Marzinski wrote:
>> On Tue, Sep 15, 2020 at 12:38:27PM +0800, lixiaokeng wrote:
>>> Hi,
>>>   The udev* function may return NULL,and it will be
>>> dereferenced in str* and sscanf func. For example,
>>> there is a coredump caused in add func, which show in
>>> be7a043(commit id) in upstream-queue. We check the
>>> return value to avoid dereference NULL.
>>>
>>> repo: openSUSE/multipath-tools
>>> repo link: https://github.com/openSUSE/multipath-tools
>>> branch: upstream-queue
>>>
>> For the set
>> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Well, the set got the logic wrong in 3 out of 6 patches :-)
> 
> In general, as these fixes are all very similar, I would prefer
> bundling them together into one patch, and I would like to make sure
> (and assert in the commit message) that the set fixes
> *all* possible NULL pointer dereferences related to accessing udev
> properties (I haven't checked whether this is the case for the set).
> 
> Also, while I'd ack this set if the logic was correct, I think that in
> a way it's papering over the actual problem. udev_device_get_XYZ()
> functions fail *only* if the underlying sysfs directory has vanished
> (well, perhaps for out-of-memory, too, but let's put that aside for a
> moment). It that case, we should actually not just return an error code

   Here we use multipath-tools basing on iscsi. When iscsi logout, the path
will disappear in sysfs and a uevent will cause. Before uevent processed,
some coredump will happen but if coredump is solved the multipathd will
deal with the disappeared path latter.
  In this set, the funcs will not be processed when multipath-tools bases
on iscsi. However, I think multipathd will deal with the disappeared path
like basing on iscsi. So I just check them. Anyway, if you have any better
idea, please tell me.

> - we should realize that there's a problem with the device (it probably
> has been deleted from the system and shouldn't be used any more in any
> way), and that we need to deal with it somehow. This is a fundamental
> problem for all user space programs that use udev_devices for more than
> a few CPU cycles. That doesn't mean the set is wrong, but we should
> keep this in mind. I have something cooking for a more complete
> solution, which isn't ready yet.
> 
> Finally, @lixiaokeng, please check your inbox once more. You still
> haven't fixed #11/14 of your previous series.
> 

I will modify patch 11/14 and send it.

> Regards,
> Martin
> 
> 
> .
> 

Re: [PATCH 0/6] libmultipath: check udev* func return value

[Martin Wilck]

On Fri, 2020-09-18 at 16:39 +0800, lixiaokeng wrote:
> 
>    Here we use multipath-tools basing on iscsi. When iscsi logout,
> the path
> will disappear in sysfs and a uevent will cause. Before uevent
> processed,
> some coredump will happen but if coredump is solved the multipathd
> will
> deal with the disappeared path latter.
>   In this set, the funcs will not be processed when multipath-tools
> bases
> on iscsi. However, I think multipathd will deal with the disappeared
> path
> like basing on iscsi. So I just check them. Anyway, if you have any
> better
> idea, please tell me.

Of course we need to avoid the segfaults. As I said, please fix your
patches and resend. It's good to know that just avoiding the NULL
dereferences fixes the issues you were seeing.

Perhaps it would be sufficient to set INIT_REMOVED on a path device
when something like this happens. But I need to think it through.

Martin