only allow unprivileged passthrough for commands without effects v4

only allow unprivileged passthrough for commands without effects v4

[Christoph Hellwig]

Hi all,

this series first fixes a few minor issues in the CES log support in the
host and target drivers and then uses the log to deny unprivileged
passthrough of commands that have effects, where the only practically
relevant effect is the modification of contents of the data stored in the
namespace.

Changes since v3:
 - clear NVME_CMD_EFFECTS_CSE_MASK for I/O commands

Changes since v2:
 - drop various cleanups and aim for a minimum viable fix for 6.2
 - fix the NVME_CMD_EFFECTS_CSE_MASK definition
 - don't allow unprivilged passthrough without a Commands Supported and
   Effects log

Changes since v1:
 - make sure ctrl->effects an nshead->effects is always available
 - initializse known effects at init time
 - remove the use_workqueue field in nvmet_req
 - fix up a commit message

Diffstat:
 drivers/nvme/host/core.c        |   32 ++++++++++++++++++++++++++------
 drivers/nvme/host/ioctl.c       |   28 ++++++++++++++++++++++++----
 drivers/nvme/target/admin-cmd.c |   37 +++++++++++++++++++++----------------
 drivers/nvme/target/passthru.c  |   11 +++++------
 include/linux/nvme.h            |    4 +++-
 5 files changed, 79 insertions(+), 33 deletions(-)

[PATCH 1/6] nvme: fix the NVME_CMD_EFFECTS_CSE_MASK definition

[Christoph Hellwig]

3 << 16 does not generate the correct mask for bits 16, 17 and 18.
Use the GENMASK macro to generate the correct mask instead.

Fixes: 84fef62d135b ("nvme: check admin passthru command effects")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
---
 include/linux/nvme.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d6be2a6861000e..d1cd53f2b6abd9 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -7,6 +7,7 @@
 #ifndef _LINUX_NVME_H
 #define _LINUX_NVME_H
 
+#include <linux/bits.h>
 #include <linux/types.h>
 #include <linux/uuid.h>
 
@@ -639,7 +640,7 @@ enum {
 	NVME_CMD_EFFECTS_NCC		= 1 << 2,
 	NVME_CMD_EFFECTS_NIC		= 1 << 3,
 	NVME_CMD_EFFECTS_CCC		= 1 << 4,
-	NVME_CMD_EFFECTS_CSE_MASK	= 3 << 16,
+	NVME_CMD_EFFECTS_CSE_MASK	= GENMASK(18, 16),
 	NVME_CMD_EFFECTS_UUID_SEL	= 1 << 19,
 };
 
-- 
2.35.1

[PATCH 2/6] nvmet: use NVME_CMD_EFFECTS_CSUPP instead of open coding it

[Christoph Hellwig]

Use NVME_CMD_EFFECTS_CSUPP instead of open coding it and assign a
single value to multiple array entries instead of repeated assignments.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 35 ++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 53a004ea320c1f..111a5cb6403fb0 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -164,26 +164,29 @@ static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 
 static void nvmet_get_cmd_effects_nvm(struct nvme_effects_log *log)
 {
-	log->acs[nvme_admin_get_log_page]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_identify]		= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_abort_cmd]		= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_set_features]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_get_features]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_async_event]	= cpu_to_le32(1 << 0);
-	log->acs[nvme_admin_keep_alive]		= cpu_to_le32(1 << 0);
-
-	log->iocs[nvme_cmd_read]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_write]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_flush]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_dsm]			= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_write_zeroes]	= cpu_to_le32(1 << 0);
+	log->acs[nvme_admin_get_log_page] =
+	log->acs[nvme_admin_identify] =
+	log->acs[nvme_admin_abort_cmd] =
+	log->acs[nvme_admin_set_features] =
+	log->acs[nvme_admin_get_features] =
+	log->acs[nvme_admin_async_event] =
+	log->acs[nvme_admin_keep_alive] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
+
+	log->iocs[nvme_cmd_read] =
+	log->iocs[nvme_cmd_write] =
+	log->iocs[nvme_cmd_flush] =
+	log->iocs[nvme_cmd_dsm]	=
+	log->iocs[nvme_cmd_write_zeroes] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
 
 static void nvmet_get_cmd_effects_zns(struct nvme_effects_log *log)
 {
-	log->iocs[nvme_cmd_zone_append]		= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_zone_mgmt_send]	= cpu_to_le32(1 << 0);
-	log->iocs[nvme_cmd_zone_mgmt_recv]	= cpu_to_le32(1 << 0);
+	log->iocs[nvme_cmd_zone_append] =
+	log->iocs[nvme_cmd_zone_mgmt_send] =
+	log->iocs[nvme_cmd_zone_mgmt_recv] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
 
 static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
-- 
2.35.1

[PATCH 3/6] nvmet: set the LBCC bit for commands that modify data

[Christoph Hellwig]

Write, Write Zeroes, Zone append and a Zone Reset through
Zone Management Send modify the logical block content of a namespace,
so make sure the LBCC bit is reported for them.

Fixes: b5d0b38c0475 ("nvmet: add Command Set Identifier support")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 111a5cb6403fb0..6a54ed6fb12144 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -174,17 +174,19 @@ static void nvmet_get_cmd_effects_nvm(struct nvme_effects_log *log)
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 
 	log->iocs[nvme_cmd_read] =
-	log->iocs[nvme_cmd_write] =
 	log->iocs[nvme_cmd_flush] =
 	log->iocs[nvme_cmd_dsm]	=
-	log->iocs[nvme_cmd_write_zeroes] =
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
+	log->iocs[nvme_cmd_write] =
+	log->iocs[nvme_cmd_write_zeroes] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC);
 }
 
 static void nvmet_get_cmd_effects_zns(struct nvme_effects_log *log)
 {
 	log->iocs[nvme_cmd_zone_append] =
 	log->iocs[nvme_cmd_zone_mgmt_send] =
+		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC);
 	log->iocs[nvme_cmd_zone_mgmt_recv] =
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
-- 
2.35.1

[PATCH 4/6] nvmet: don't defer passthrough commands with trivial effects to the workqueue

[Christoph Hellwig]

Mask out the "Command Supported" and "Logical Block Content Change" bits
and only defer execution of commands that have non-trivial effects to
the workqueue for synchronous execution.  This allows to execture admin
commands asynchronously on controllers that provide a Command Supported
and Effects log page, and will keep allowing to execute Write commands
asynchronously once command effects on I/O commands are taken into
account.

Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/target/passthru.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 79af5140af8bfe..adc0958755d66f 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -334,14 +334,13 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 	}
 
 	/*
-	 * If there are effects for the command we are about to execute, or
-	 * an end_req function we need to use nvme_execute_passthru_rq()
-	 * synchronously in a work item seeing the end_req function and
-	 * nvme_passthru_end() can't be called in the request done callback
-	 * which is typically in interrupt context.
+	 * If a command needs post-execution fixups, or there are any
+	 * non-trivial effects, make sure to execute the command synchronously
+	 * in a workqueue so that nvme_passthru_end gets called.
 	 */
 	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
-	if (req->p.use_workqueue || effects) {
+	if (req->p.use_workqueue ||
+	    (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))) {
 		INIT_WORK(&req->p.work, nvmet_passthru_execute_cmd_work);
 		req->p.rq = rq;
 		queue_work(nvmet_wq, &req->p.work);
-- 
2.35.1

[PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Christoph Hellwig]

To be able to use the Commands Supported and Effects Log for allowing
unprivileged passtrough, it needs to be corretly reported for I/O
commands as well.  Return the I/O command effects from
nvme_command_effects, and also add a default list of effects for the
NVM command set.  For other command sets, the Commands Supported and
Effects log is required to be present already.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/core.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e26b085a007aea..21d79c186d8c14 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1074,6 +1074,18 @@ static u32 nvme_known_admin_effects(u8 opcode)
 	return 0;
 }
 
+static u32 nvme_known_nvm_effects(u8 opcode)
+{
+	switch (opcode) {
+	case nvme_cmd_write:
+	case nvme_cmd_write_zeroes:
+	case nvme_cmd_write_uncor:
+		 return NVME_CMD_EFFECTS_LBCC;
+	default:
+		return 0;
+	}
+}
+
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 {
 	u32 effects = 0;
@@ -1081,16 +1093,24 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 	if (ns) {
 		if (ns->head->effects)
 			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
+			effects |= nvme_known_nvm_effects(opcode);
 		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
 			dev_warn_once(ctrl->device,
-				"IO command:%02x has unhandled effects:%08x\n",
+				"IO command:%02x has unusual effects:%08x\n",
 				opcode, effects);
-		return 0;
-	}
 
-	if (ctrl->effects)
-		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-	effects |= nvme_known_admin_effects(opcode);
+		/*
+		 * NVME_CMD_EFFECTS_CSE_MASK causes a freeze all I/O queues,
+		 * which would deadlock when done on an I/O command.  Note that
+		 * We already warn about an unusual effect above.
+		 */
+		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
+	} else {
+		if (ctrl->effects)
+			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
+		effects |= nvme_known_admin_effects(opcode);
+	}
 
 	return effects;
 }
-- 
2.35.1

[PATCH 6/6] nvme: consult the CSE log page for unprivileged passthrough

[Christoph Hellwig]

Commands like Write Zeros can change the contents of a namespaces without
actually transferring data.  To protect against this, check the Commands
Supported and Effects log is supported by the controller for any
unprivileg command passthrough and refuse unprivileged passthrough if the
command has any effects that can change data or metadata.

Note: While the Commands Support and Effects log page has only been
mandatory since NVMe 2.0, it is widely supported because Windows requires
it for any command passthrough from userspace.

Fixes: e4fbcf32c860 ("nvme: identify-namespace without CAP_SYS_ADMIN")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/ioctl.c | 28 ++++++++++++++++++++++++----
 include/linux/nvme.h      |  1 +
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 9ddda571f0461f..a8639919237e6a 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -11,6 +11,8 @@
 static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 		fmode_t mode)
 {
+	u32 effects;
+
 	if (capable(CAP_SYS_ADMIN))
 		return true;
 
@@ -43,11 +45,29 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	}
 
 	/*
-	 * Only allow I/O commands that transfer data to the controller if the
-	 * special file is open for writing, but always allow I/O commands that
-	 * transfer data from the controller.
+	 * Check if the controller provides a Commands Supported and Effects log
+	 * and marks this command as supported.  If not reject unprivileged
+	 * passthrough.
+	 */
+	effects = nvme_command_effects(ns->ctrl, ns, c->common.opcode);
+	if (!(effects & NVME_CMD_EFFECTS_CSUPP))
+		return false;
+
+	/*
+	 * Don't allow passthrough for command that have intrusive (or unknown)
+	 * effects.
+	 */
+	if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
+			NVME_CMD_EFFECTS_UUID_SEL |
+			NVME_CMD_EFFECTS_SCOPE_MASK))
+		return false;
+
+	/*
+	 * Only allow I/O commands that transfer data to the controller or that
+	 * change the logical block contents if the file descriptor is open for
+	 * writing.
 	 */
-	if (nvme_is_write(c))
+	if (nvme_is_write(c) || (effects & NVME_CMD_EFFECTS_LBCC))
 		return mode & FMODE_WRITE;
 	return true;
 }
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d1cd53f2b6abd9..4fad4aa245fb06 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -642,6 +642,7 @@ enum {
 	NVME_CMD_EFFECTS_CCC		= 1 << 4,
 	NVME_CMD_EFFECTS_CSE_MASK	= GENMASK(18, 16),
 	NVME_CMD_EFFECTS_UUID_SEL	= 1 << 19,
+	NVME_CMD_EFFECTS_SCOPE_MASK	= GENMASK(31, 20),
 };
 
 struct nvme_effects_log {
-- 
2.35.1

Re: [PATCH 1/6] nvme: fix the NVME_CMD_EFFECTS_CSE_MASK definition

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 2/6] nvmet: use NVME_CMD_EFFECTS_CSUPP instead of open coding it

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 3/6] nvmet: set the LBCC bit for commands that modify data

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 4/6] nvmet: don't defer passthrough commands with trivial effects to the workqueue

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>


On 12/23/22 09:18, Christoph Hellwig wrote:
> Mask out the "Command Supported" and "Logical Block Content Change" bits
> and only defer execution of commands that have non-trivial effects to
> the workqueue for synchronous execution.  This allows to execture admin

allows to execute admin

> commands asynchronously on controllers that provide a Command Supported
> and Effects log page, and will keep allowing to execute Write commands
> asynchronously once command effects on I/O commands are taken into
> account.
> 
> Fixes: c1fef73f793b ("nvmet: add passthru code to process commands")
> Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/target/passthru.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index 79af5140af8bfe..adc0958755d66f 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -334,14 +334,13 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
>   	}
>   
>   	/*
> -	 * If there are effects for the command we are about to execute, or
> -	 * an end_req function we need to use nvme_execute_passthru_rq()
> -	 * synchronously in a work item seeing the end_req function and
> -	 * nvme_passthru_end() can't be called in the request done callback
> -	 * which is typically in interrupt context.
> +	 * If a command needs post-execution fixups, or there are any
> +	 * non-trivial effects, make sure to execute the command synchronously
> +	 * in a workqueue so that nvme_passthru_end gets called.
>   	 */
>   	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
> -	if (req->p.use_workqueue || effects) {
> +	if (req->p.use_workqueue ||
> +	    (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))) {
>   		INIT_WORK(&req->p.work, nvmet_passthru_execute_cmd_work);
>   		req->p.rq = rq;
>   		queue_work(nvmet_wq, &req->p.work);

Other than that,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Sagi Grimberg]

On 12/23/22 09:18, Christoph Hellwig wrote:
> To be able to use the Commands Supported and Effects Log for allowing
> unprivileged passtrough, it needs to be corretly reported for I/O
> commands as well.  Return the I/O command effects from
> nvme_command_effects, and also add a default list of effects for the
> NVM command set.  For other command sets, the Commands Supported and
> Effects log is required to be present already.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
>   drivers/nvme/host/core.c | 32 ++++++++++++++++++++++++++------
>   1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e26b085a007aea..21d79c186d8c14 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1074,6 +1074,18 @@ static u32 nvme_known_admin_effects(u8 opcode)
>   	return 0;
>   }
>   
> +static u32 nvme_known_nvm_effects(u8 opcode)
> +{
> +	switch (opcode) {
> +	case nvme_cmd_write:
> +	case nvme_cmd_write_zeroes:
> +	case nvme_cmd_write_uncor:
> +		 return NVME_CMD_EFFECTS_LBCC;
> +	default:
> +		return 0;
> +	}
> +}
> +
>   u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
>   {
>   	u32 effects = 0;
> @@ -1081,16 +1093,24 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
>   	if (ns) {
>   		if (ns->head->effects)
>   			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
> +		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
> +			effects |= nvme_known_nvm_effects(opcode);
>   		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
>   			dev_warn_once(ctrl->device,
> -				"IO command:%02x has unhandled effects:%08x\n",
> +				"IO command:%02x has unusual effects:%08x\n",
>   				opcode, effects);
> -		return 0;
> -	}
>   
> -	if (ctrl->effects)
> -		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
> -	effects |= nvme_known_admin_effects(opcode);
> +		/*
> +		 * NVME_CMD_EFFECTS_CSE_MASK causes a freeze all I/O queues,
> +		 * which would deadlock when done on an I/O command.  Note that
> +		 * We already warn about an unusual effect above.
> +		 */

I think it would be helpful to warn that we are masking it out? The
warning above does not directly relate to the cse mask, but rather 
anthing outside of NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC ?

> +		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
> +	} else {
> +		if (ctrl->effects)
> +			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
> +		effects |= nvme_known_admin_effects(opcode);
> +	}
>   
>   	return effects;
>   }

Re: [PATCH 6/6] nvme: consult the CSE log page for unprivileged passthrough

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Christoph Hellwig]

On Sun, Dec 25, 2022 at 12:26:35PM +0200, Sagi Grimberg wrote:
>> +		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
>> +			effects |= nvme_known_nvm_effects(opcode);
>>   		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
>>   			dev_warn_once(ctrl->device,
>> -				"IO command:%02x has unhandled effects:%08x\n",
>> +				"IO command:%02x has unusual effects:%08x\n",
>>   				opcode, effects);
>> -		return 0;
>> -	}
>>   -	if (ctrl->effects)
>> -		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
>> -	effects |= nvme_known_admin_effects(opcode);
>> +		/*
>> +		 * NVME_CMD_EFFECTS_CSE_MASK causes a freeze all I/O queues,
>> +		 * which would deadlock when done on an I/O command.  Note that
>> +		 * We already warn about an unusual effect above.
>> +		 */
>
> I think it would be helpful to warn that we are masking it out? The
> warning above does not directly relate to the cse mask, but rather anthing 
> outside of NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC ?

Well, all these effects are unusual for I/O commands, and all the
effects really are hints to the host.  But if you think we can do
better, can you send an incremental fixup?  We'll need to get
these fixes in ASAP to fix the gapping security hole, or just
revert the unprivileged passthrough for 6.2.

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Sagi Grimberg]

>>> +		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
>>> +			effects |= nvme_known_nvm_effects(opcode);
>>>    		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
>>>    			dev_warn_once(ctrl->device,
>>> -				"IO command:%02x has unhandled effects:%08x\n",
>>> +				"IO command:%02x has unusual effects:%08x\n",
>>>    				opcode, effects);
>>> -		return 0;
>>> -	}
>>>    -	if (ctrl->effects)
>>> -		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
>>> -	effects |= nvme_known_admin_effects(opcode);
>>> +		/*
>>> +		 * NVME_CMD_EFFECTS_CSE_MASK causes a freeze all I/O queues,
>>> +		 * which would deadlock when done on an I/O command.  Note that
>>> +		 * We already warn about an unusual effect above.
>>> +		 */
>>
>> I think it would be helpful to warn that we are masking it out? The
>> warning above does not directly relate to the cse mask, but rather anthing
>> outside of NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC ?
> 
> Well, all these effects are unusual for I/O commands, and all the
> effects really are hints to the host.  But if you think we can do
> better, can you send an incremental fixup?  We'll need to get
> these fixes in ASAP to fix the gapping security hole, or just
> revert the unprivileged passthrough for 6.2.

That's fine, No need to hold this for 6.2

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Christoph Hellwig]

On Wed, Dec 28, 2022 at 03:49:59PM +0200, Sagi Grimberg wrote:
> That's fine, No need to hold this for 6.2

Thanks.  Keith, Kanchan - any comments on this version of the series?

Re: only allow unprivileged passthrough for commands without effects v4

[Keith Busch]

Series looks good to go.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Unless you're already on it, I'd be happy to take a shot at the
improvements we discussed earlier for the next merge window.

Re: only allow unprivileged passthrough for commands without effects v4

[Christoph Hellwig]

On Wed, Dec 28, 2022 at 09:04:28AM -0700, Keith Busch wrote:
> Series looks good to go.
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> 
> Unless you're already on it, I'd be happy to take a shot at the
> improvements we discussed earlier for the next merge window.

I still have the current versions of those stashed away from the
last iteration, but as they'd need a rebase anyway I'll happily leave
that to you.

Re: [PATCH 5/6] nvme: also return I/O command effects from nvme_command_effects

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

On Wed, Dec 28, 2022 at 04:12:55PM +0100, Christoph Hellwig wrote:
>On Wed, Dec 28, 2022 at 03:49:59PM +0200, Sagi Grimberg wrote:
>> That's fine, No need to hold this for 6.2
>
>Thanks.  Keith, Kanchan - any comments on this version of the series?

This continues to look good. I am all for having this in.
Thanks for the fix.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]