Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Sasha Levin <Alexander.Levin@microsoft.com>
To:     "stable@vger.kernel.org" <stable@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     Rakesh Pandit <rakesh@tuxera.com>, Christoph Hellwig <hch@lst.de>,
        Sasha Levin <Alexander.Levin@microsoft.com>
Subject: [PATCH AUTOSEL for 4.9 002/293] nvme-pci: fix multiple ctrl removal
 scheduling
Thread-Topic: [PATCH AUTOSEL for 4.9 002/293] nvme-pci: fix multiple ctrl
 removal scheduling
Thread-Index: AQHTz5jiabZzN+tEhkKG9rDuoA+X1A==
Date:   Mon, 9 Apr 2018 00:22:47 +0000
Message-ID: <20180409002239.163177-2-alexander.levin@microsoft.com>
References: <20180409002239.163177-1-alexander.levin@microsoft.com>
In-Reply-To: <20180409002239.163177-1-alexander.levin@microsoft.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: microsoft.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 885d5ee0-f68d-472f-f658-08d59db08a3d
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2018 00:22:47.0654
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR2101MB1015
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Rakesh Pandit <rakesh@tuxera.com>

[ Upstream commit 82b057caefaff2a891f821a617d939f46e03e844 ]

Commit c5f6ce97c1210 tries to address multiple resets but fails as
work_busy doesn't involve any synchronization and can fail.  This is
reproducible easily as can be seen by WARNING below which is triggered
with line:

WARN_ON(dev->ctrl.state =3D=3D NVME_CTRL_RESETTING)

Allowing multiple resets can result in multiple controller removal as
well if different conditions inside nvme_reset_work fail and which
might deadlock on device_release_driver.

[  480.327007] WARNING: CPU: 3 PID: 150 at drivers/nvme/host/pci.c:1900 nvm=
e_reset_work+0x36c/0xec0
[  480.327008] Modules linked in: rfcomm fuse nf_conntrack_netbios_ns nf_co=
nntrack_broadcast...
[  480.327044]  btusb videobuf2_core ghash_clmulni_intel snd_hwdep cfg80211=
 acer_wmi hci_uart..
[  480.327065] CPU: 3 PID: 150 Comm: kworker/u16:2 Not tainted 4.12.0-rc1+ =
#13
[  480.327065] Hardware name: Acer Predator G9-591/Mustang_SLS, BIOS V1.10 =
03/03/2016
[  480.327066] Workqueue: nvme nvme_reset_work
[  480.327067] task: ffff880498ad8000 task.stack: ffffc90002218000
[  480.327068] RIP: 0010:nvme_reset_work+0x36c/0xec0
[  480.327069] RSP: 0018:ffffc9000221bdb8 EFLAGS: 00010246
[  480.327070] RAX: 0000000000460000 RBX: ffff880498a98128 RCX: dead0000000=
00200
[  480.327070] RDX: 0000000000000001 RSI: ffff8804b1028020 RDI: ffff880498a=
98128
[  480.327071] RBP: ffffc9000221be50 R08: 0000000000000000 R09: 00000000000=
00000
[  480.327071] R10: ffffc90001963ce8 R11: 000000000000020d R12: ffff880498a=
98000
[  480.327072] R13: ffff880498a53500 R14: ffff880498a98130 R15: ffff880498a=
98128
[  480.327072] FS:  0000000000000000(0000) GS:ffff8804c1cc0000(0000) knlGS:=
0000000000000000
[  480.327073] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  480.327074] CR2: 00007ffcf3c37f78 CR3: 0000000001e09000 CR4: 00000000003=
406e0
[  480.327074] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 00000000000=
00000
[  480.327075] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 00000000000=
00400
[  480.327075] Call Trace:
[  480.327079]  ? __switch_to+0x227/0x400
[  480.327081]  process_one_work+0x18c/0x3a0
[  480.327082]  worker_thread+0x4e/0x3b0
[  480.327084]  kthread+0x109/0x140
[  480.327085]  ? process_one_work+0x3a0/0x3a0
[  480.327087]  ? kthread_park+0x60/0x60
[  480.327102]  ret_from_fork+0x2c/0x40
[  480.327103] Code: e8 5a dc ff ff 85 c0 41 89 c1 0f.....

This patch addresses the problem by using state of controller to
decide whether reset should be queued or not as state change is
synchronizated using controller spinlock.  Also cancel_work_sync is
used to make sure remove cancels the reset_work and waits for it to
finish.  This patch also changes return value from -ENODEV to more
appropriate -EBUSY if nvme_reset fails to change state.

Fixes: c5f6ce97c1210 ("nvme: don't schedule multiple resets")
Signed-off-by: Rakesh Pandit <rakesh@tuxera.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
---
 drivers/nvme/host/pci.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e48ecb9303ca..8cc856ecec95 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1263,7 +1263,7 @@ static bool nvme_should_reset(struct nvme_dev *dev, u=
32 csts)
 	bool nssro =3D dev->subsystem && (csts & NVME_CSTS_NSSRO);
=20
 	/* If there is a reset ongoing, we shouldn't reset again. */
-	if (work_busy(&dev->reset_work))
+	if (dev->ctrl.state =3D=3D NVME_CTRL_RESETTING)
 		return false;
=20
 	/* We shouldn't reset unless the controller is on fatal error state
@@ -1755,7 +1755,7 @@ static void nvme_reset_work(struct work_struct *work)
 	struct nvme_dev *dev =3D container_of(work, struct nvme_dev, reset_work);
 	int result =3D -ENODEV;
=20
-	if (WARN_ON(dev->ctrl.state =3D=3D NVME_CTRL_RESETTING))
+	if (WARN_ON(dev->ctrl.state !=3D NVME_CTRL_RESETTING))
 		goto out;
=20
 	/*
@@ -1765,9 +1765,6 @@ static void nvme_reset_work(struct work_struct *work)
 	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
 		nvme_dev_disable(dev, false);
=20
-	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
-		goto out;
-
 	result =3D nvme_pci_enable(dev);
 	if (result)
 		goto out;
@@ -1841,8 +1838,8 @@ static int nvme_reset(struct nvme_dev *dev)
 {
 	if (!dev->ctrl.admin_q || blk_queue_dying(dev->ctrl.admin_q))
 		return -ENODEV;
-	if (work_busy(&dev->reset_work))
-		return -ENODEV;
+	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
+		return -EBUSY;
 	if (!queue_work(nvme_workq, &dev->reset_work))
 		return -EBUSY;
 	return 0;
@@ -1944,6 +1941,7 @@ static int nvme_probe(struct pci_dev *pdev, const str=
uct pci_device_id *id)
 	if (result)
 		goto release_pools;
=20
+	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING);
 	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
=20
 	queue_work(nvme_workq, &dev->reset_work);
@@ -1987,6 +1985,7 @@ static void nvme_remove(struct pci_dev *pdev)
=20
 	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
=20
+	cancel_work_sync(&dev->reset_work);
 	pci_set_drvdata(pdev, NULL);
=20
 	if (!pci_device_is_present(pdev)) {
--=20
2.15.1