ibmvfc: Fix invalid state machine BUG_ON

Message ID	20210413001009.902400-1-tyreld@linux.ibm.com
State	New
Headers	show Return-Path: <linux-scsi-owner@kernel.org> From: Tyrel Datwyler <tyreld@linux.ibm.com> To: james.bottomley@hansenpartnership.com Cc: martin.petersen@oracle.com, linux-scsi@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, brking@linux.ibm.com, Brian King <brking@linux.vnet.ibm.com>, Tyrel Datwyler <tyreld@linux.ibm.com> Subject: [PATCH] ibmvfc: Fix invalid state machine BUG_ON Date: Mon, 12 Apr 2021 18:10:09 -0600 Message-Id: <20210413001009.902400-1-tyreld@linux.ibm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	ibmvfc: Fix invalid state machine BUG_ON \| expand ibmvfc: Fix invalid state machine BUG_ON

Message ID

20210413001009.902400-1-tyreld@linux.ibm.com

State

New

Headers

From: Tyrel Datwyler <tyreld@linux.ibm.com>
To: james.bottomley@hansenpartnership.com
Cc: martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
	brking@linux.ibm.com, Brian King <brking@linux.vnet.ibm.com>,
	Tyrel Datwyler <tyreld@linux.ibm.com>
Subject: [PATCH] ibmvfc: Fix invalid state machine BUG_ON
Date: Mon, 12 Apr 2021 18:10:09 -0600
Message-Id: <20210413001009.902400-1-tyreld@linux.ibm.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

ibmvfc: Fix invalid state machine BUG_ON | expand

Commit Message

Tyrel Datwyler April 13, 2021, 12:10 a.m. UTC

From: Brian King <brking@linux.vnet.ibm.com>

This fixes an issue hitting the BUG_ON in ibmvfc_do_work. When
going through a host action of IBMVFC_HOST_ACTION_RESET,
we change the action to IBMVFC_HOST_ACTION_TGT_DEL,
then drop the host lock, and reset the CRQ, which changes
the host state to IBMVFC_NO_CRQ. If, prior to setting the
host state to IBMVFC_NO_CRQ, ibmvfc_init_host is called,
it can then end up changing the host action to IBMVFC_HOST_ACTION_INIT.
If we then change the host state to IBMVFC_NO_CRQ, we will then
hit the BUG_ON. This patch makes a couple of changes to avoid this.
It leaves the host action to be IBMVFC_HOST_ACTION_RESET
or IBMVFC_HOST_ACTION_REENABLE until after we drop the host
lock and reset or reenable the CRQ. It also hardens the
host state machine to ensure we cannot leave the reset / reenable
state until we've finished processing the reset or reenable.

Fixes: 73ee5d867287 ("[SCSI] ibmvfc: Fix soft lockup on resume")
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
[tyreld: added fixes tag]
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 drivers/scsi/ibmvscsi/ibmvfc.c | 53 ++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 19 deletions(-)

Comments

Martin K. Petersen April 13, 2021, 5:28 a.m. UTC | #1

Tyrel,

> This fixes an issue hitting the BUG_ON in ibmvfc_do_work. When going

> through a host action of IBMVFC_HOST_ACTION_RESET, we change the

> action to IBMVFC_HOST_ACTION_TGT_DEL, then drop the host lock, and

> reset the CRQ, which changes the host state to IBMVFC_NO_CRQ.


[...]

Applied to 5.13/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Martin K. Petersen April 16, 2021, 2:51 a.m. UTC | #2

On Mon, 12 Apr 2021 18:10:09 -0600, Tyrel Datwyler wrote:

> This fixes an issue hitting the BUG_ON in ibmvfc_do_work. When

> going through a host action of IBMVFC_HOST_ACTION_RESET,

> we change the action to IBMVFC_HOST_ACTION_TGT_DEL,

> then drop the host lock, and reset the CRQ, which changes

> the host state to IBMVFC_NO_CRQ. If, prior to setting the

> host state to IBMVFC_NO_CRQ, ibmvfc_init_host is called,

> it can then end up changing the host action to IBMVFC_HOST_ACTION_INIT.

> If we then change the host state to IBMVFC_NO_CRQ, we will then

> hit the BUG_ON. This patch makes a couple of changes to avoid this.

> It leaves the host action to be IBMVFC_HOST_ACTION_RESET

> or IBMVFC_HOST_ACTION_REENABLE until after we drop the host

> lock and reset or reenable the CRQ. It also hardens the

> host state machine to ensure we cannot leave the reset / reenable

> state until we've finished processing the reset or reenable.


Applied to 5.13/scsi-queue, thanks!

[1/1] ibmvfc: Fix invalid state machine BUG_ON
      https://git.kernel.org/mkp/scsi/c/15cfef8623a4

-- 
Martin K. Petersen	Oracle Linux Engineering

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 61831f2fdb30..f813608d74cc 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -603,8 +603,17 @@  static void ibmvfc_set_host_action(struct ibmvfc_host *vhost,
 		if (vhost->action == IBMVFC_HOST_ACTION_ALLOC_TGTS)
 			vhost->action = action;
 		break;
+	case IBMVFC_HOST_ACTION_REENABLE:
+	case IBMVFC_HOST_ACTION_RESET:
+		vhost->action = action;
+		break;
 	case IBMVFC_HOST_ACTION_INIT:
 	case IBMVFC_HOST_ACTION_TGT_DEL:
+	case IBMVFC_HOST_ACTION_LOGO:
+	case IBMVFC_HOST_ACTION_QUERY_TGTS:
+	case IBMVFC_HOST_ACTION_TGT_DEL_FAILED:
+	case IBMVFC_HOST_ACTION_NONE:
+	default:
 		switch (vhost->action) {
 		case IBMVFC_HOST_ACTION_RESET:
 		case IBMVFC_HOST_ACTION_REENABLE:
@@ -614,15 +623,6 @@  static void ibmvfc_set_host_action(struct ibmvfc_host *vhost,
 			break;
 		}
 		break;
-	case IBMVFC_HOST_ACTION_LOGO:
-	case IBMVFC_HOST_ACTION_QUERY_TGTS:
-	case IBMVFC_HOST_ACTION_TGT_DEL_FAILED:
-	case IBMVFC_HOST_ACTION_NONE:
-	case IBMVFC_HOST_ACTION_RESET:
-	case IBMVFC_HOST_ACTION_REENABLE:
-	default:
-		vhost->action = action;
-		break;
 	}
 }
 
@@ -5373,30 +5373,45 @@  static void ibmvfc_do_work(struct ibmvfc_host *vhost)
 	case IBMVFC_HOST_ACTION_INIT_WAIT:
 		break;
 	case IBMVFC_HOST_ACTION_RESET:
-		vhost->action = IBMVFC_HOST_ACTION_TGT_DEL;
 		list_splice_init(&vhost->purge, &purge);
 		spin_unlock_irqrestore(vhost->host->host_lock, flags);
 		ibmvfc_complete_purge(&purge);
 		rc = ibmvfc_reset_crq(vhost);
+
 		spin_lock_irqsave(vhost->host->host_lock, flags);
-		if (rc == H_CLOSED)
+		if (!rc || rc == H_CLOSED)
 			vio_enable_interrupts(to_vio_dev(vhost->dev));
-		if (rc || (rc = ibmvfc_send_crq_init(vhost)) ||
-		    (rc = vio_enable_interrupts(to_vio_dev(vhost->dev)))) {
-			ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
-			dev_err(vhost->dev, "Error after reset (rc=%d)\n", rc);
+		if (vhost->action == IBMVFC_HOST_ACTION_RESET) {
+			/* The only action we could have changed to would have been reenable,
+			 in which case, we skip the rest of this path and wait until
+			 we've done the re-enable before sending the crq init */
+
+			vhost->action = IBMVFC_HOST_ACTION_TGT_DEL;
+
+			if (rc || (rc = ibmvfc_send_crq_init(vhost)) ||
+			    (rc = vio_enable_interrupts(to_vio_dev(vhost->dev)))) {
+				ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
+				dev_err(vhost->dev, "Error after reset (rc=%d)\n", rc);
+			}
 		}
 		break;
 	case IBMVFC_HOST_ACTION_REENABLE:
-		vhost->action = IBMVFC_HOST_ACTION_TGT_DEL;
 		list_splice_init(&vhost->purge, &purge);
 		spin_unlock_irqrestore(vhost->host->host_lock, flags);
 		ibmvfc_complete_purge(&purge);
 		rc = ibmvfc_reenable_crq_queue(vhost);
+
 		spin_lock_irqsave(vhost->host->host_lock, flags);
-		if (rc || (rc = ibmvfc_send_crq_init(vhost))) {
-			ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
-			dev_err(vhost->dev, "Error after enable (rc=%d)\n", rc);
+		if (vhost->action == IBMVFC_HOST_ACTION_REENABLE) {
+			/* The only action we could have changed to would have been reset,
+			 in which case, we skip the rest of this path and wait until
+			 we've done the reset before sending the crq init */
+
+			vhost->action = IBMVFC_HOST_ACTION_TGT_DEL;
+			if (rc || (rc = ibmvfc_send_crq_init(vhost))) {
+				ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
+				dev_err(vhost->dev, "Error after enable (rc=%d)\n", rc);
+			}
 		}
 		break;
 	case IBMVFC_HOST_ACTION_LOGO:

ibmvfc: Fix invalid state machine BUG_ON

Commit Message

Comments

Patch