All of lore.kernel.org
 help / color / mirror / Atom feed
From: lixiaokeng <lixiaokeng@huawei.com>
To: Benjamin Marzinski <bmarzins@redhat.com>,
	Martin Wilck <mwilck@suse.com>,
	Christophe Varoqui <christophe.varoqui@opensvc.com>,
	dm-devel@redhat.com
Cc: linfeilong@huawei.com, liuzhiqiang26@huawei.com
Subject: [PATCH 2/5] libmultipath fix NULL dereference in select_action
Date: Tue, 18 Aug 2020 21:06:36 +0800	[thread overview]
Message-ID: <285a42f1-69c2-ce2e-e30c-d12fb6edc859@huawei.com> (raw)
In-Reply-To: <5ef5f58e-3a27-8959-3abb-4b4c401cc309@huawei.com>

I got a multipath segfault while running iscsi login/logout and following scripts in parallel:

#!/bin/bash
interval=1
while true
do
              multipath -F &> /dev/null
              multipath -r &> /dev/null
              multipath -v2 &> /dev/null
              multipath -ll &> /dev/null
              sleep $interval
done

This is the debuginfo:
Core was generated by `multipath -v2'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007fc5c5c8dedf in select_action (mpp=0x55a94f1f8980, curmp=<optimized out>,
    force_reload=<optimized out>) at configure.c:709
709		if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) { {

(gdb) bt
#0  0x00007fc5c5c8dedf in select_action (mpp=0x55a94f1f8980, curmp=<optimized out>,
    force_reload=<optimized out>) at configure.c:709
#1  0x00007fc5c5c8fd0f in coalesce_paths (vecs=0x7ffff1c1c220, newmp=0x0, refwwid=0x0,
    force_reload=0, cmd=CMD_CREATE) at configure.c:1090
#2  0x000055a94e9f524d in configure (devpath=<optimized out>, dev_type=DEV_NONE,
    cmd=CMD_CREATE, conf=0x55a94f1b92d0) at main.c:757
#3  main (argc=<optimized out>, argv=<optimized out>) at main.c:1100
(gdb) p *cmpp
$1 = {
  wwid = "3600140566411a6d4873434fba988066f\000\070\060\066\066f", '\000' <repeats 88 times>,
  ...
  paths = 0x0, pg = 0x0, dmi = 0x55a94f22c3f0, alias = 0x55a94f205fb0 "mpathc",
  alias_prefix = 0x0, selector = 0x0, features = 0x0, hwhandler = 0x0, mpe = 0x0,
  hwe = 0x0, waiter = 0, stat_switchgroup = 0, stat_path_failures = 0, stat_map_loads = 0,
  ...
  generic_mp = {ops = 0x7fc5c5cada40 <dm_gen_multipath_ops>}}

There are many NULL pointers in cmpp such as selector, features, hwhandler and so on.
Here these on mpp is not NULL  but we can not be sure that won't be in mpp, so we
add checking pointers of both mpp and cmpp before using them.

The changes like "mpp->features != cmpp->features" make sure that mpp->action is not
set to ACT_RELOAD when they both equal NULL. Other changes check pointers don't equal
NULL. When only one is NULL, we think there is some difference between mpp and cmpp,
so mpp->action should be set to ACT_RELOAD.

Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 libmultipath/configure.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 96c79610..e02e7ef4 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -726,16 +726,20 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}

 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
+	    mpp->features != cmpp->features &&
+	    (!mpp->features || !cmpp->features ||
 	    !!strstr(mpp->features, "queue_if_no_path") !=
-	    !!strstr(cmpp->features, "queue_if_no_path")) {
+	    !!strstr(cmpp->features, "queue_if_no_path"))) {
 		mpp->action =  ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (no_path_retry change)",
 			mpp->alias);
 		return;
 	}
-	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
+	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON || !cmpp->hwhandler ||
 	     strcmp(cmpp->hwhandler, "0") == 0) &&
-	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
+	     mpp->hwhandler != cmpp->hwhandler &&
+	     (!mpp->hwhandler || !cmpp->hwhandler||
+	     strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
 	     strncmp(cmpp->hwhandler, mpp->hwhandler,
 		    strlen(mpp->hwhandler)))) {
 		mpp->action = ACT_RELOAD;
@@ -745,8 +749,10 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}

 	if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
+	    mpp->features != cmpp->features &&
+	    (!mpp->features || !cmpp->features ||
 	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
-	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
+	    !!strstr(cmpp->features, "retain_attached_hw_handler")) &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
 		mpp->action = ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
@@ -754,8 +760,16 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 		return;
 	}

-	cmpp_feat = STRDUP(cmpp->features);
-	mpp_feat = STRDUP(mpp->features);
+	if (!cmpp->features ) {
+		cmpp_feat = NULL;
+	} else {
+		cmpp_feat = STRDUP(cmpp->features);
+	}
+	if (!mpp->features ) {
+		mpp_feat = NULL;
+	} else {
+		mpp_feat = STRDUP(mpp->features);
+	}
 	if (cmpp_feat && mpp_feat) {
 		remove_feature(&mpp_feat, "queue_if_no_path");
 		remove_feature(&mpp_feat, "retain_attached_hw_handler");
@@ -770,8 +784,8 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	FREE(cmpp_feat);
 	FREE(mpp_feat);

-	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
-		    strlen(mpp->selector))) {
+	if (mpp->selector != cmpp->selector && (!cmpp->selector || !mpp->selector ||
+		    strncmp(cmpp->selector, mpp->selector,strlen(mpp->selector)))) {
 		mpp->action = ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (selector change)",
 			mpp->alias);
-- 

  parent reply	other threads:[~2020-08-18 13:06 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-18 12:59 [PATCH 0/5] multipath-tools series: coredump and memory leak bugfix lixiaokeng
2020-08-18 13:02 ` [PATCH 1/5 v4] libmultipath fix a memory leak in set_ble_device lixiaokeng
2020-08-18 15:57   ` Martin Wilck
2020-08-18 13:06 ` lixiaokeng [this message]
2020-08-18 16:28   ` [PATCH 2/5] libmultipath fix NULL dereference in select_action Martin Wilck
2020-08-21  4:41     ` lixiaokeng
2020-08-18 13:08 ` [PATCH 3/5] multipathd: add reclear_pp_from_mpp in ev_remove_path lixiaokeng
2020-08-18 16:36   ` Martin Wilck
2020-08-20 14:51     ` lixiaokeng
2020-08-20 15:22       ` Martin Wilck
2020-08-24 13:07         ` lixiaokeng
2020-08-18 13:09 ` [PATCH 4/5] multipathd: disable queueing for recreated map in uev_remove_map lixiaokeng
2020-08-18 19:23   ` Martin Wilck
2020-08-19  8:48     ` lixiaokeng
2020-08-19  9:27   ` Martin Wilck
2020-08-18 13:11 ` [PATCH 5/5] libmultipath fix daemon memory leak in disassemble_map lixiaokeng
2020-08-19  1:42   ` lixiaokeng
2020-08-19  1:50 ` lixiaokeng
2020-08-19  8:26   ` Martin Wilck
2020-08-19  8:45     ` lixiaokeng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=285a42f1-69c2-ce2e-e30c-d12fb6edc859@huawei.com \
    --to=lixiaokeng@huawei.com \
    --cc=bmarzins@redhat.com \
    --cc=christophe.varoqui@opensvc.com \
    --cc=dm-devel@redhat.com \
    --cc=linfeilong@huawei.com \
    --cc=liuzhiqiang26@huawei.com \
    --cc=mwilck@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.