nvme: validate controller state before rescheduling keep alive

Delete operations are seeing NULL pointer references in call_timer_fn. Tracking these back, the timer appears to be the keep alive timer.

nvme_keep_alive_work() which is tied to the timer that is cancelled by nvme_stop_keep_alive(), simply starts the keep alive io but doesn't wait for it's completion. So nvme_stop_keep_alive() only stops a timer when it's pending. When a keep alive is in flight, there is no timer running and the nvme_stop_keep_alive() will have no affect on the keep alive io. Thus, if the io completes successfully, the keep alive timer
will be rescheduled. In the failure case, delete is called, the
controller state is changed, the nvme_stop_keep_alive() is called while the io is outstanding, and the delete path continues on. The keep alive happens to successfully complete before the delete paths mark it as aborted as part of the queue termination, so the timer is restarted. The delete paths then tear down the controller, and later on the timer code fires and the timer entry is now corrupt.

Fix by validating the controller state before rescheduling the keep alive. Testing with the fix has confirmed the condition above was hit.

---
 drivers/nvme/host/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index fe957166c4a9..deb047514408 100644
@@ -843,7 +843,9 @@ static void nvme_keep_alive_end_io(struct request *rq, blk_status_t status)
 	}
 
 	ctrl->comp_seen = false;
-	schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+	if (ctrl->state == NVME_CTRL_LIVE ||
+	    ctrl->state == NVME_CTRL_CONNECTING)
+		schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
 }
 
 static int nvme_keep_alive(struct nvme_ctrl *ctrl)

2.13.7

[adding Sagi for keep alive issues]

So this flow is when a keep alive successful completion arrives and before we actually schedule the work we start to teardown the controller (and stop the keep alive work) correct? I guess that can happen...

I agree that it should be fine, but should we add a rmb before checking the state (and a wmb when setting the state) given that this is racy to begin with?

Trying to think if there is a cleaner way to address that...