* [PATCH 0/6] libmultipath: check udev* func return value
@ 2020-09-15 4:38 lixiaokeng
2020-09-15 4:39 ` [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name lixiaokeng
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: lixiaokeng @ 2020-09-15 4:38 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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(-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
@ 2020-09-15 4:39 ` lixiaokeng
2020-09-18 7:34 ` Martin Wilck
2020-09-15 4:39 ` [PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo lixiaokeng
` (5 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: lixiaokeng @ 2020-09-15 4:39 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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;
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
2020-09-15 4:39 ` [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name lixiaokeng
@ 2020-09-15 4:39 ` lixiaokeng
2020-09-18 7:01 ` Martin Wilck
2020-09-15 4:40 ` [PATCH 3/6] libmultipath: check udev* return value in sysfs_get_tgt_nodename lixiaokeng
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: lixiaokeng @ 2020-09-15 4:39 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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,
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/6] libmultipath: check udev* return value in sysfs_get_tgt_nodename
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
2020-09-15 4:39 ` [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name lixiaokeng
2020-09-15 4:39 ` [PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo lixiaokeng
@ 2020-09-15 4:40 ` lixiaokeng
2020-09-15 4:41 ` [PATCH 4/6] libmultipath: check udev* return value in trigger_partitions_udev_change lixiaokeng
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: lixiaokeng @ 2020-09-15 4:40 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/6] libmultipath: check udev* return value in trigger_partitions_udev_change
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
` (2 preceding siblings ...)
2020-09-15 4:40 ` [PATCH 3/6] libmultipath: check udev* return value in sysfs_get_tgt_nodename lixiaokeng
@ 2020-09-15 4:41 ` lixiaokeng
2020-09-15 4:41 ` [PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev lixiaokeng
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: lixiaokeng @ 2020-09-15 4:41 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
` (3 preceding siblings ...)
2020-09-15 4:41 ` [PATCH 4/6] libmultipath: check udev* return value in trigger_partitions_udev_change lixiaokeng
@ 2020-09-15 4:41 ` lixiaokeng
2020-09-18 6:59 ` Martin Wilck
2020-09-15 4:42 ` [PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath lixiaokeng
2020-09-18 2:26 ` [PATCH 0/6] libmultipath: check udev* func return value Benjamin Marzinski
6 siblings, 1 reply; 15+ messages in thread
From: lixiaokeng @ 2020-09-15 4:41 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
` (4 preceding siblings ...)
2020-09-15 4:41 ` [PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev lixiaokeng
@ 2020-09-15 4:42 ` lixiaokeng
2020-09-18 6:58 ` Martin Wilck
2020-09-18 2:26 ` [PATCH 0/6] libmultipath: check udev* func return value Benjamin Marzinski
6 siblings, 1 reply; 15+ messages in thread
From: lixiaokeng @ 2020-09-15 4:42 UTC (permalink / raw)
To: Christophe Varoqui, Martin Wilck, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/6] libmultipath: check udev* func return value
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
` (5 preceding siblings ...)
2020-09-15 4:42 ` [PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath lixiaokeng
@ 2020-09-18 2:26 ` Benjamin Marzinski
2020-09-18 7:37 ` Martin Wilck
6 siblings, 1 reply; 15+ messages in thread
From: Benjamin Marzinski @ 2020-09-18 2:26 UTC (permalink / raw)
To: lixiaokeng
Cc: linfeilong, dm-devel mailing list, Martin Wilck, liuzhiqiang (I)
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(-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath
2020-09-15 4:42 ` [PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath lixiaokeng
@ 2020-09-18 6:58 ` Martin Wilck
0 siblings, 0 replies; 15+ messages in thread
From: Martin Wilck @ 2020-09-18 6:58 UTC (permalink / raw)
To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev
2020-09-15 4:41 ` [PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev lixiaokeng
@ 2020-09-18 6:59 ` Martin Wilck
0 siblings, 0 replies; 15+ messages in thread
From: Martin Wilck @ 2020-09-18 6:59 UTC (permalink / raw)
To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo
2020-09-15 4:39 ` [PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo lixiaokeng
@ 2020-09-18 7:01 ` Martin Wilck
0 siblings, 0 replies; 15+ messages in thread
From: Martin Wilck @ 2020-09-18 7:01 UTC (permalink / raw)
To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name
2020-09-15 4:39 ` [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name lixiaokeng
@ 2020-09-18 7:34 ` Martin Wilck
0 siblings, 0 replies; 15+ messages in thread
From: Martin Wilck @ 2020-09-18 7:34 UTC (permalink / raw)
To: lixiaokeng, Christophe Varoqui, Benjamin Marzinski,
dm-devel mailing list
Cc: linfeilong, liuzhiqiang (I)
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/6] libmultipath: check udev* func return value
2020-09-18 2:26 ` [PATCH 0/6] libmultipath: check udev* func return value Benjamin Marzinski
@ 2020-09-18 7:37 ` Martin Wilck
2020-09-18 8:39 ` lixiaokeng
0 siblings, 1 reply; 15+ messages in thread
From: Martin Wilck @ 2020-09-18 7:37 UTC (permalink / raw)
To: Benjamin Marzinski, lixiaokeng
Cc: dm-devel, list, linfeilong, liuzhiqiang (I)
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/6] libmultipath: check udev* func return value
2020-09-18 7:37 ` Martin Wilck
@ 2020-09-18 8:39 ` lixiaokeng
2020-09-18 11:15 ` Martin Wilck
0 siblings, 1 reply; 15+ messages in thread
From: lixiaokeng @ 2020-09-18 8:39 UTC (permalink / raw)
To: Martin Wilck, Benjamin Marzinski
Cc: linfeilong, dm-devel mailing list, liuzhiqiang (I)
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
>
>
> .
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/6] libmultipath: check udev* func return value
2020-09-18 8:39 ` lixiaokeng
@ 2020-09-18 11:15 ` Martin Wilck
0 siblings, 0 replies; 15+ messages in thread
From: Martin Wilck @ 2020-09-18 11:15 UTC (permalink / raw)
To: lixiaokeng, Benjamin Marzinski
Cc: dm-devel, list, linfeilong, liuzhiqiang (I)
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-09-18 11:15 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 4:38 [PATCH 0/6] libmultipath: check udev* func return value lixiaokeng
2020-09-15 4:39 ` [PATCH 1/6] libmultipath: check uedv* return value in sysfs_get_host_pci_name lixiaokeng
2020-09-18 7:34 ` Martin Wilck
2020-09-15 4:39 ` [PATCH 2/6] libmultipath: check udev* return value in ccw_sysfs_pathinfo lixiaokeng
2020-09-18 7:01 ` Martin Wilck
2020-09-15 4:40 ` [PATCH 3/6] libmultipath: check udev* return value in sysfs_get_tgt_nodename lixiaokeng
2020-09-15 4:41 ` [PATCH 4/6] libmultipath: check udev* return value in trigger_partitions_udev_change lixiaokeng
2020-09-15 4:41 ` [PATCH 5/6] libmultipath: check udev* renturn value in get_ctrl_blkdev lixiaokeng
2020-09-18 6:59 ` Martin Wilck
2020-09-15 4:42 ` [PATCH 6/6] libmultipath: check udev* return value in _find_path_by_syspath lixiaokeng
2020-09-18 6:58 ` Martin Wilck
2020-09-18 2:26 ` [PATCH 0/6] libmultipath: check udev* func return value Benjamin Marzinski
2020-09-18 7:37 ` Martin Wilck
2020-09-18 8:39 ` lixiaokeng
2020-09-18 11:15 ` Martin Wilck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).