linux-kernel - Re: [PATCH 1/2] nvme: only allow entering LIVE from CONNECTING state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6894c914-a734-4a7b-97cb-e9a344fa5560@flourine.local>
Date: Tue, 29 Apr 2025 15:55:56 +0200
From: Daniel Wagner <dwagner@...e.de>
To: Hannes Reinecke <hare@...e.de>
Cc: Guenter Roeck <linux@...ck-us.net>, Daniel Wagner <wagi@...nel.org>, 
	Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>, 
	Sagi Grimberg <sagi@...mberg.me>, James Smart <james.smart@...adcom.com>, 
	Shinichiro Kawasaki <shinichiro.kawasaki@....com>, linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] nvme: only allow entering LIVE from CONNECTING state

On Mon, Apr 28, 2025 at 03:21:18PM +0200, Hannes Reinecke wrote:
> On 4/28/25 14:44, Daniel Wagner wrote:
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index b502ac07483b..d3c4eacf607f 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -4493,7 +4493,8 @@ static void nvme_fw_act_work(struct work_struct *work)
> >                  msleep(100);
> >          }
> > 
> > -       if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_LIVE))
> > +       if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING) ||
> > +           !nvme_change_ctrl_state(ctrl, NVME_CTRL_LIVE))
> >                  return;
> > 
> >          nvme_unquiesce_io_queues(ctrl);
> 
> I would rather have a separate state for firmware activation.
> (Ab-)using the 'RESETTING' state here has direct implications
> with the error handler, as for the error handler 'RESETTING'
> means that the error handler has been scheduled.
> Which is not true for firmware activation.

Okay, so something like this here (untested, working on it)?


diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b502ac07483b..32482712d0f2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -565,6 +565,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 	case NVME_CTRL_LIVE:
 		switch (old_state) {
 		case NVME_CTRL_CONNECTING:
+		case NVME_CTRL_FW_ACTIVATION:
 			changed = true;
 			fallthrough;
 		default:
@@ -575,6 +576,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		switch (old_state) {
 		case NVME_CTRL_NEW:
 		case NVME_CTRL_LIVE:
+		case NVME_CTRL_FW_ACTIVATION:
 			changed = true;
 			fallthrough;
 		default:
@@ -596,6 +598,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_RESETTING:
 		case NVME_CTRL_CONNECTING:
+		case NVME_CTRL_FW_ACTIVATION:
 			changed = true;
 			fallthrough;
 		default:
@@ -621,6 +624,15 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 			break;
 		}
 		break;
+	case NVME_CTRL_FW_ACTIVATION:
+		switch (old_state) {
+		case NVME_CTRL_LIVE:
+			changed = true;
+			fallthrough;
+		default:
+			break;
+		}
+		break;
 	default:
 		break;
 	}
@@ -4529,7 +4541,7 @@ static bool nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 		 * recovery actions from interfering with the controller's
 		 * firmware activation.
 		 */
-		if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) {
+		if (nvme_change_ctrl_state(ctrl, NVME_CTRL_FW_ACTIVATION)) {
 			requeue = false;
 			queue_work(nvme_wq, &ctrl->fw_act_work);
 		}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 51e078642127..3a383225afed 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -247,6 +247,7 @@ static inline u16 nvme_req_qid(struct request *req)
  *				shutdown or removal. In this case we forcibly
  *				kill all inflight I/O as they have no chance to
  *				complete
+ * @NVME_CTRL_FW_ACTIVATION:	Controller is in firmware activation state.
  */
 enum nvme_ctrl_state {
 	NVME_CTRL_NEW,
@@ -256,6 +257,7 @@ enum nvme_ctrl_state {
 	NVME_CTRL_DELETING,
 	NVME_CTRL_DELETING_NOIO,
 	NVME_CTRL_DEAD,
+	NVME_CTRL_FW_ACTIVATION,
 };

 struct nvme_fault_inject {