[PATCH v3 0/9] Do not allow set-partition immediate mode

[PATCH v3 0/9] Do not allow set-partition immediate mode

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Blocking immediate mode in set-partition info triggered a
refactoring of the send path of userspace mailbox commands.

The v1 to address the issue was a single patch [1] that inserted
a new immediate mode check in the send path where the payload was
available for examining. That was not in a validation function.

The v2 patchset [2] added refactoring of the send path so that
validation work can all spawn from cxl_validate_cmd_from_user().

Here, v3 offers a finer level of refactoring:

Patches 1-7: Refactor existing code so that all validation work
	can spawn from cxl_validate_cmd_from_user().

	The movement intends to cleanly rip the work of building a
	mailbox command away from handle_mailbox_command_from_user()
	and give it to cxl_validate_cmd_from_user().

Patch 8: Blocks the immediate mode of the set-partition command.
Patch 9: Removes CXL_PMEM exclusive commands restriction.

[1] https://lore.kernel.org/linux-cxl/20220103202100.784194-1-alison.schofield@intel.com/
[2] https://lore.kernel.org/linux-cxl/a4daafc4b537a0b1a673c55300da7747784c4573.1645817416.git.alison.schofield@intel.com/

Changes in v3:
- Split up the 'Centralize the validation...' patch into 6 pieces.
Patch: cxl/mbox: Move cxl_mem_command construction to helper funcs
- Safely initialize the cxl_mem_command structs. (Dan)
- Remove unneeded memcpy (Dan)
Patch: cxl/mbox: Block immediate mode in SET_PARTITION_INFO command
- No Changes
Patch: cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list
- No Changes

Changes in v2:
- Refactor the send path of userspace mbox cmds. (Dan, Ben)
- Patch 3 commit log - update the need to block. (Dan)
- Return -EBUSY (not -EINVAL), when blocking immediate mode. (Ben)
- Remove unneeded cast of void (payload_in). (dan)
- s/u64/__le64 in struct cxl_mbox_set_partition_info. (Dan)

Alison Schofield (9):
  cxl/mbox: Move cxl_mem_command construction to helper funcs
  cxl/mbox: Move raw command warning to raw command validation
  cxl/mbox: Move build of user mailbox cmd to a helper function
  cxl/mbox: Construct a users cxl_mbox_cmd in the validation path
  cxl/mbox: Remove dependency on cxl_mem_command for a debug msg
  cxl/mbox: Make handle_mailbox_cmd_from_user() use a mbox param
  cxl/mbox: Move cxl_mem_command param to a local variable
  cxl/mbox: Block immediate mode in SET_PARTITION_INFO command
  cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list

 drivers/cxl/core/mbox.c | 311 +++++++++++++++++++++++++---------------
 drivers/cxl/cxlmem.h    |   7 +
 drivers/cxl/pmem.c      |   1 -
 3 files changed, 200 insertions(+), 119 deletions(-)


base-commit: 9b688fc651b9d2b633e8d959454670aba1c39162
-- 
2.31.1

[PATCH v3 1/9] cxl/mbox: Move cxl_mem_command construction to helper funcs

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Sanitizing and constructing a cxl_mem_command from a userspace
command is part of the validation process prior to submitting
the command to a CXL device. Move this work to helper functions:
cxl_to_mem_cmd(), cxl_to_mem_cmd_raw().

This declutters cxl_validate_cmd_from_user() in preparation for
adding new validation steps.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 158 +++++++++++++++++++++-------------------
 1 file changed, 85 insertions(+), 73 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index be61a0d8016b..6612d73c37a8 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -207,6 +207,84 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
 	return true;
 }
 
+static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds,
+			      const struct cxl_send_command *send_cmd,
+			      struct cxl_mem_command *mem_cmd)
+{
+	if (send_cmd->raw.rsvd)
+		return -EINVAL;
+	/*
+	 * Unlike supported commands, the output size of RAW commands
+	 * gets passed along without further checking, so it must be
+	 * validated here.
+	 */
+	if (send_cmd->out.size > cxlds->payload_size)
+		return -EINVAL;
+
+	if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode))
+		return -EPERM;
+
+	*mem_cmd = (struct cxl_mem_command) {
+		.info = {
+			.id = CXL_MEM_COMMAND_ID_RAW,
+			.size_in = send_cmd->in.size,
+			.size_out = send_cmd->out.size,
+		},
+		.opcode = send_cmd->raw.opcode
+	};
+
+	return 0;
+}
+
+static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
+			  const struct cxl_send_command *send_cmd,
+			  struct cxl_mem_command *mem_cmd)
+{
+	const struct cxl_command_info *info;
+	struct cxl_mem_command *c;
+
+	if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK)
+		return -EINVAL;
+
+	if (send_cmd->rsvd)
+		return -EINVAL;
+
+	if (send_cmd->in.rsvd || send_cmd->out.rsvd)
+		return -EINVAL;
+
+	/* Convert user's command into the internal representation */
+	c = &cxl_mem_commands[send_cmd->id];
+	info = &c->info;
+
+	/* Check that the command is enabled for hardware */
+	if (!test_bit(info->id, cxlds->enabled_cmds))
+		return -ENOTTY;
+
+	/* Check that the command is not claimed for exclusive kernel use */
+	if (test_bit(info->id, cxlds->exclusive_cmds))
+		return -EBUSY;
+
+	/* Check the input buffer is the expected size */
+	if (info->size_in >= 0 && info->size_in != send_cmd->in.size)
+		return -ENOMEM;
+
+	/* Check the output buffer is at least large enough */
+	if (info->size_out >= 0 && send_cmd->out.size < info->size_out)
+		return -ENOMEM;
+
+	*mem_cmd = (struct cxl_mem_command) {
+		.info = {
+			.id = info->id,
+			.flags = info->flags,
+			.size_in = send_cmd->in.size,
+			.size_out = send_cmd->out.size,
+		},
+		.opcode = c->opcode
+	};
+
+	return 0;
+}
+
 /**
  * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND.
  * @cxlds: The device data for the operation
@@ -230,8 +308,7 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
 				      const struct cxl_send_command *send_cmd,
 				      struct cxl_mem_command *out_cmd)
 {
-	const struct cxl_command_info *info;
-	struct cxl_mem_command *c;
+	int rc;
 
 	if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX)
 		return -ENOTTY;
@@ -244,78 +321,13 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
 	if (send_cmd->in.size > cxlds->payload_size)
 		return -EINVAL;
 
-	/*
-	 * Checks are bypassed for raw commands but a WARN/taint will occur
-	 * later in the callchain
-	 */
-	if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW) {
-		const struct cxl_mem_command temp = {
-			.info = {
-				.id = CXL_MEM_COMMAND_ID_RAW,
-				.flags = 0,
-				.size_in = send_cmd->in.size,
-				.size_out = send_cmd->out.size,
-			},
-			.opcode = send_cmd->raw.opcode
-		};
+	/* Sanitize and construct a cxl_mem_command */
+	if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW)
+		rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, out_cmd);
+	else
+		rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd);
 
-		if (send_cmd->raw.rsvd)
-			return -EINVAL;
-
-		/*
-		 * Unlike supported commands, the output size of RAW commands
-		 * gets passed along without further checking, so it must be
-		 * validated here.
-		 */
-		if (send_cmd->out.size > cxlds->payload_size)
-			return -EINVAL;
-
-		if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode))
-			return -EPERM;
-
-		memcpy(out_cmd, &temp, sizeof(temp));
-
-		return 0;
-	}
-
-	if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK)
-		return -EINVAL;
-
-	if (send_cmd->rsvd)
-		return -EINVAL;
-
-	if (send_cmd->in.rsvd || send_cmd->out.rsvd)
-		return -EINVAL;
-
-	/* Convert user's command into the internal representation */
-	c = &cxl_mem_commands[send_cmd->id];
-	info = &c->info;
-
-	/* Check that the command is enabled for hardware */
-	if (!test_bit(info->id, cxlds->enabled_cmds))
-		return -ENOTTY;
-
-	/* Check that the command is not claimed for exclusive kernel use */
-	if (test_bit(info->id, cxlds->exclusive_cmds))
-		return -EBUSY;
-
-	/* Check the input buffer is the expected size */
-	if (info->size_in >= 0 && info->size_in != send_cmd->in.size)
-		return -ENOMEM;
-
-	/* Check the output buffer is at least large enough */
-	if (info->size_out >= 0 && send_cmd->out.size < info->size_out)
-		return -ENOMEM;
-
-	memcpy(out_cmd, c, sizeof(*c));
-	out_cmd->info.size_in = send_cmd->in.size;
-	/*
-	 * XXX: out_cmd->info.size_out will be controlled by the driver, and the
-	 * specified number of bytes @send_cmd->out.size will be copied back out
-	 * to userspace.
-	 */
-
-	return 0;
+	return rc;
 }
 
 int cxl_query_cmd(struct cxl_memdev *cxlmd,
-- 
2.31.1

[PATCH v3 2/9] cxl/mbox: Move raw command warning to raw command validation

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

This move serves two purposes: 1) Emit the warning in the raw
command validation path, and 2) Remove the dependency on the
struct cxl_mem_command in handle_mailbox_cmd_from_user() in
preparation for a refactor of that function.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 6612d73c37a8..28131c6f7fcf 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -224,6 +224,8 @@ static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds,
 	if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode))
 		return -EPERM;
 
+	dev_WARN_ONCE(cxlds->dev, true, "raw command path used\n");
+
 	*mem_cmd = (struct cxl_mem_command) {
 		.info = {
 			.id = CXL_MEM_COMMAND_ID_RAW,
@@ -424,9 +426,6 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
 		cxl_command_names[cmd->info.id].name, mbox_cmd.opcode,
 		cmd->info.size_in);
 
-	dev_WARN_ONCE(dev, cmd->info.id == CXL_MEM_COMMAND_ID_RAW,
-		      "raw command path used\n");
-
 	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
 	if (rc)
 		goto out;
-- 
2.31.1

[PATCH v3 3/9] cxl/mbox: Move build of user mailbox cmd to a helper function

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

In preparation for moving the construction of a mailbox command
to the validation path, extract the work into a helper function.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 60 +++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 28131c6f7fcf..d4233cfb2f99 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -207,6 +207,38 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
 	return true;
 }
 
+static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
+			   struct cxl_mbox_cmd *mbox, u16 opcode,
+			   size_t in_size, size_t out_size, u64 in_payload)
+{
+	*mbox = (struct cxl_mbox_cmd) {
+		.opcode = opcode,
+		.size_in = in_size,
+		.size_out = out_size,
+	};
+
+	if (!in_size)
+		goto size_out;
+
+	mbox->payload_in = vmemdup_user(u64_to_user_ptr(in_payload), in_size);
+	if (!mbox->payload_in)
+		return PTR_ERR(mbox->payload_in);
+
+size_out:
+	/* Prepare to handle a full payload for variable sized output */
+	if (out_size < 0)
+		mbox->size_out = cxlds->payload_size;
+
+	if (mbox->size_out) {
+		mbox->payload_out = kvzalloc(mbox->size_out, GFP_KERNEL);
+		if (!mbox->payload_out) {
+			kvfree(mbox->payload_in);
+			return -ENOMEM;
+		}
+	}
+	return 0;
+}
+
 static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds,
 			      const struct cxl_send_command *send_cmd,
 			      struct cxl_mem_command *mem_cmd)
@@ -397,27 +429,13 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
 					s32 *size_out, u32 *retval)
 {
 	struct device *dev = cxlds->dev;
-	struct cxl_mbox_cmd mbox_cmd = {
-		.opcode = cmd->opcode,
-		.size_in = cmd->info.size_in,
-		.size_out = cmd->info.size_out,
-	};
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
-	if (cmd->info.size_out) {
-		mbox_cmd.payload_out = kvzalloc(cmd->info.size_out, GFP_KERNEL);
-		if (!mbox_cmd.payload_out)
-			return -ENOMEM;
-	}
-
-	if (cmd->info.size_in) {
-		mbox_cmd.payload_in = vmemdup_user(u64_to_user_ptr(in_payload),
-						   cmd->info.size_in);
-		if (IS_ERR(mbox_cmd.payload_in)) {
-			kvfree(mbox_cmd.payload_out);
-			return PTR_ERR(mbox_cmd.payload_in);
-		}
-	}
+	rc = cxl_to_mbox_cmd(cxlds, &mbox_cmd, cmd->opcode, cmd->info.size_in,
+			     cmd->info.size_out, in_payload);
+	if (rc)
+		return rc;
 
 	dev_dbg(dev,
 		"Submitting %s command for user\n"
@@ -471,10 +489,6 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
 	if (rc)
 		return rc;
 
-	/* Prepare to handle a full payload for variable sized output */
-	if (c.info.size_out < 0)
-		c.info.size_out = cxlds->payload_size;
-
 	rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload,
 					  send.out.payload, &send.out.size,
 					  &send.retval);
-- 
2.31.1

[PATCH v3 4/9] cxl/mbox: Construct a users cxl_mbox_cmd in the validation path

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Since the validation of a mailbox command is done as it is built,
move that work out of the dispatch path and into the validation
path.

This is a step in refactoring the handling of user space mailbox
commands. The intent is to have all the validation work originate
in cxl_validate_cmd_from_user().

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d4233cfb2f99..205e671307c3 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -324,6 +324,7 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
  * @cxlds: The device data for the operation
  * @send_cmd: &struct cxl_send_command copied in from userspace.
  * @out_cmd: Sanitized and populated &struct cxl_mem_command.
+ * @mbox_cmd: Sanitized and populated &struct cxl_mbox_cmd.
  *
  * Return:
  *  * %0	- @out_cmd is ready to send.
@@ -340,7 +341,8 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
  */
 static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
 				      const struct cxl_send_command *send_cmd,
-				      struct cxl_mem_command *out_cmd)
+				      struct cxl_mem_command *out_cmd,
+				      struct cxl_mbox_cmd *mbox_cmd)
 {
 	int rc;
 
@@ -361,6 +363,14 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
 	else
 		rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd);
 
+	if (rc)
+		return rc;
+
+	/* Sanitize and construct a cxl_mbox_cmd */
+	rc = cxl_to_mbox_cmd(cxlds, mbox_cmd, out_cmd->opcode,
+			     out_cmd->info.size_in, out_cmd->info.size_out,
+			     send_cmd->in.payload);
+
 	return rc;
 }
 
@@ -478,6 +488,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
 	struct device *dev = &cxlmd->dev;
 	struct cxl_send_command send;
 	struct cxl_mem_command c;
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
 	dev_dbg(dev, "Send IOCTL\n");
@@ -485,7 +496,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
 	if (copy_from_user(&send, s, sizeof(send)))
 		return -EFAULT;
 
-	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c);
+	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c, &mbox_cmd);
 	if (rc)
 		return rc;
 
-- 
2.31.1

[PATCH v3 5/9] cxl/mbox: Remove dependency on cxl_mem_command for a debug msg

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

In preparation for removing access to struct cxl_mem_command,
change this debug message to use cxl_mbox_cmd fields instead.
Retrieve the pretty command name from cxl_mbox_cmd using a new
opcode to command name helper.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 205e671307c3..d6d582baa1ee 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -127,6 +127,17 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
 	return NULL;
 }
 
+static const char *cxl_mem_opcode_to_name(u16 opcode)
+{
+	struct cxl_mem_command *c;
+
+	c = cxl_mem_find_command(opcode);
+	if (c)
+		return cxl_command_names[c->info.id].name;
+
+	return NULL;
+}
+
 /**
  * cxl_mbox_send_cmd() - Send a mailbox command to a device.
  * @cxlds: The device data for the operation
@@ -450,9 +461,9 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
 	dev_dbg(dev,
 		"Submitting %s command for user\n"
 		"\topcode: %x\n"
-		"\tsize: %ub\n",
-		cxl_command_names[cmd->info.id].name, mbox_cmd.opcode,
-		cmd->info.size_in);
+		"\tsize: %zx\n",
+		cxl_mem_opcode_to_name(mbox_cmd.opcode),
+		mbox_cmd.opcode, mbox_cmd.size_in);
 
 	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
 	if (rc)
-- 
2.31.1

[PATCH v3 6/9] cxl/mbox: Make handle_mailbox_cmd_from_user() use a mbox param

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Previously, handle_mailbox_cmd_from_user(), constructed the mailbox
command and dispatched it to the hardware. The construction work
has moved to the validation path.

handle_mailbox_cmd_from_user() now expects a fully validated
mbox param. Make it's caller, cxl_send_cmd(), deliver it. Update
the comments and dereferencing of the new mbox parameter.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 45 +++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d6d582baa1ee..0340399c7029 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -422,8 +422,7 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
 /**
  * handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace.
  * @cxlds: The device data for the operation
- * @cmd: The validated command.
- * @in_payload: Pointer to userspace's input payload.
+ * @mbox_cmd: The validated mailbox command.
  * @out_payload: Pointer to userspace's output payload.
  * @size_out: (Input) Max payload size to copy out.
  *            (Output) Payload size hardware generated.
@@ -438,34 +437,27 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
  *  * %-EINTR	- Mailbox acquisition interrupted.
  *  * %-EXXX	- Transaction level failures.
  *
- * Creates the appropriate mailbox command and dispatches it on behalf of a
- * userspace request. The input and output payloads are copied between
- * userspace.
+ * Dispatches a mailbox command on behalf of a userspace request.
+ * The output payload is copied to userspace.
  *
  * See cxl_send_cmd().
  */
 static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
-					const struct cxl_mem_command *cmd,
-					u64 in_payload, u64 out_payload,
-					s32 *size_out, u32 *retval)
+					struct cxl_mbox_cmd *mbox_cmd,
+					u64 out_payload, s32 *size_out,
+					u32 *retval)
 {
 	struct device *dev = cxlds->dev;
-	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
-	rc = cxl_to_mbox_cmd(cxlds, &mbox_cmd, cmd->opcode, cmd->info.size_in,
-			     cmd->info.size_out, in_payload);
-	if (rc)
-		return rc;
-
 	dev_dbg(dev,
 		"Submitting %s command for user\n"
 		"\topcode: %x\n"
 		"\tsize: %zx\n",
-		cxl_mem_opcode_to_name(mbox_cmd.opcode),
-		mbox_cmd.opcode, mbox_cmd.size_in);
+		cxl_mem_opcode_to_name(mbox_cmd->opcode),
+		mbox_cmd->opcode, mbox_cmd->size_in);
 
-	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
+	rc = cxlds->mbox_send(cxlds, mbox_cmd);
 	if (rc)
 		goto out;
 
@@ -474,22 +466,22 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
 	 * to userspace. While the payload may have written more output than
 	 * this it will have to be ignored.
 	 */
-	if (mbox_cmd.size_out) {
-		dev_WARN_ONCE(dev, mbox_cmd.size_out > *size_out,
+	if (mbox_cmd->size_out) {
+		dev_WARN_ONCE(dev, mbox_cmd->size_out > *size_out,
 			      "Invalid return size\n");
 		if (copy_to_user(u64_to_user_ptr(out_payload),
-				 mbox_cmd.payload_out, mbox_cmd.size_out)) {
+				 mbox_cmd->payload_out, mbox_cmd->size_out)) {
 			rc = -EFAULT;
 			goto out;
 		}
 	}
 
-	*size_out = mbox_cmd.size_out;
-	*retval = mbox_cmd.return_code;
+	*size_out = mbox_cmd->size_out;
+	*retval = mbox_cmd->return_code;
 
 out:
-	kvfree(mbox_cmd.payload_in);
-	kvfree(mbox_cmd.payload_out);
+	kvfree(mbox_cmd->payload_in);
+	kvfree(mbox_cmd->payload_out);
 	return rc;
 }
 
@@ -511,9 +503,8 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
 	if (rc)
 		return rc;
 
-	rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload,
-					  send.out.payload, &send.out.size,
-					  &send.retval);
+	rc = handle_mailbox_cmd_from_user(cxlds, &mbox_cmd, send.out.payload,
+					  &send.out.size, &send.retval);
 	if (rc)
 		return rc;
 
-- 
2.31.1

[PATCH v3 7/9] cxl/mbox: Move cxl_mem_command param to a local variable

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

cxl_validate_command_from_user() is now the single point of validation
for mailbox commands coming from user space. Previously, it returned a
a cxl_mem_command, but that was not sufficient when validation of the
actual mailbox command became a requirement. Now, it returns a fully
validated cxl_mbox_cmd.

Remove the extraneous cxl_mem_command parameter. Define and use a
local version only.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 0340399c7029..c323d6792405 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -331,10 +331,9 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
 }
 
 /**
- * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND.
+ * cxl_validate_cmd_from_user() - Construct a valid cxl_mbox_cmd
  * @cxlds: The device data for the operation
  * @send_cmd: &struct cxl_send_command copied in from userspace.
- * @out_cmd: Sanitized and populated &struct cxl_mem_command.
  * @mbox_cmd: Sanitized and populated &struct cxl_mbox_cmd.
  *
  * Return:
@@ -345,16 +344,14 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
  *  * %-EPERM	- Attempted to use a protected command.
  *  * %-EBUSY	- Kernel has claimed exclusive access to this opcode
  *
- * The result of this command is a fully validated command in @out_cmd that is
+ * The result of this command is a fully validated command in @mbox_cmd that is
  * safe to send to the hardware.
- *
- * See handle_mailbox_cmd_from_user()
  */
 static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
 				      const struct cxl_send_command *send_cmd,
-				      struct cxl_mem_command *out_cmd,
 				      struct cxl_mbox_cmd *mbox_cmd)
 {
+	struct cxl_mem_command mem_cmd;
 	int rc;
 
 	if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX)
@@ -370,16 +367,16 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
 
 	/* Sanitize and construct a cxl_mem_command */
 	if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW)
-		rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, out_cmd);
+		rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, &mem_cmd);
 	else
-		rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd);
+		rc = cxl_to_mem_cmd(cxlds, send_cmd, &mem_cmd);
 
 	if (rc)
 		return rc;
 
 	/* Sanitize and construct a cxl_mbox_cmd */
-	rc = cxl_to_mbox_cmd(cxlds, mbox_cmd, out_cmd->opcode,
-			     out_cmd->info.size_in, out_cmd->info.size_out,
+	rc = cxl_to_mbox_cmd(cxlds, mbox_cmd, mem_cmd.opcode,
+			     mem_cmd.info.size_in, mem_cmd.info.size_out,
 			     send_cmd->in.payload);
 
 	return rc;
@@ -490,7 +487,6 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct device *dev = &cxlmd->dev;
 	struct cxl_send_command send;
-	struct cxl_mem_command c;
 	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
@@ -499,7 +495,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
 	if (copy_from_user(&send, s, sizeof(send)))
 		return -EFAULT;
 
-	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c, &mbox_cmd);
+	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &mbox_cmd);
 	if (rc)
 		return rc;
 
-- 
2.31.1

[PATCH v3 8/9] cxl/mbox: Block immediate mode in SET_PARTITION_INFO command

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

User space may send the SET_PARTITION_INFO mailbox command using
the IOCTL interface. Inspect the input payload and fail if the
immediate flag is set.

This is the first instance of the driver inspecting an input payload
from user space. Assume there will be more such cases and implement
with an extensible helper.

In order for the kernel to react to an immediate partition change it
needs to assert that the change will not affect any active decode. At
a minimum this requires validating that the device is using HDM
decoders instead of the CXL DVSEC for decode, and that none of the
active HDM decoders are affected by the partition change. For now,
just fail until that support arrives.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 41 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h    |  7 +++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index c323d6792405..1930e203061d 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -218,6 +218,40 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
 	return true;
 }
 
+/**
+ * cxl_payload_from_user_allowed() - Check contents of in_payload.
+ * @opcode: The mailbox command opcode.
+ * @payload_in: Pointer to the input payload passed in from user space.
+ *
+ * Return:
+ *  * true	- payload_in passes check for @opcode.
+ *  * false	- payload_in contains invalid or unsupported values.
+ *
+ * The driver may inspect payload contents before sending a mailbox
+ * command from user space to the device. The intent is to reject
+ * commands with input payloads that are known to be unsafe. This
+ * check is not intended to replace the users careful selection of
+ * mailbox command parameters and makes no guarantee that the user
+ * command will succeed, nor that it is appropriate.
+ *
+ * The specific checks are determined by the opcode.
+ */
+static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_SET_PARTITION_INFO: {
+		struct cxl_mbox_set_partition_info *pi = payload_in;
+
+		if (pi->flags && CXL_SET_PARTITION_IMMEDIATE_FLAG)
+			return false;
+		break;
+	}
+	default:
+		break;
+	}
+	return true;
+}
+
 static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
 			   struct cxl_mbox_cmd *mbox, u16 opcode,
 			   size_t in_size, size_t out_size, u64 in_payload)
@@ -235,6 +269,13 @@ static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
 	if (!mbox->payload_in)
 		return PTR_ERR(mbox->payload_in);
 
+	if (!cxl_payload_from_user_allowed(opcode, mbox->payload_in)) {
+		dev_dbg(cxlds->dev, "%s: input payload not allowed\n",
+			cxl_mem_opcode_to_name(opcode));
+		kvfree(mbox->payload_in);
+		return -EBUSY;
+	}
+
 size_out:
 	/* Prepare to handle a full payload for variable sized output */
 	if (out_size < 0)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index d5c9a273d07d..dee574527e50 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -264,6 +264,13 @@ struct cxl_mbox_set_lsa {
 	u8 data[];
 } __packed;
 
+struct cxl_mbox_set_partition_info {
+	__le64 volatile_capacity;
+	u8 flags;
+} __packed;
+
+#define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
+
 /**
  * struct cxl_mem_command - Driver representation of a memory device command
  * @info: Command information as it exists for the UAPI
-- 
2.31.1

[PATCH v3 9/9] cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

With SET_PARTITION_INFO on the exclusive_cmds list for the CXL_PMEM
driver, userspace cannot execute a set-partition command without
first unbinding the pmem driver from the device.

When userspace requests a partition change to take effect on the
next reboot this unbind requirement is unnecessarily restrictive.
The driver does not need to enforce an unbind because partitions
will not change until the next reboot. Of course, userspace still
needs to be aware that changing the size of persistent capacity
on the next reboot will result in the loss of data stored. That
can happen regardless of whether it is presently bound at the time
of issuing the set-partition command.

When userspace requests a partition change to take effect immediately,
restrictions are needed. The CXL_MEM driver currently blocks the usage
of immediate mode, making the presence of SET_PARTITION_INFO, in this
exclusive commands list, redundant.

In the future, when the CXL_MEM driver adds support for immediate
changes to device partitions it will ensure that the partition change
will not affect any active decode. That means the work will not fall
right back here, onto the CXL_PMEM driver.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/pmem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 564d125d25ef..35c6f3af18f2 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -344,7 +344,6 @@ static __init int cxl_pmem_init(void)
 {
 	int rc;
 
-	set_bit(CXL_MEM_COMMAND_ID_SET_PARTITION_INFO, exclusive_cmds);
 	set_bit(CXL_MEM_COMMAND_ID_SET_SHUTDOWN_STATE, exclusive_cmds);
 	set_bit(CXL_MEM_COMMAND_ID_SET_LSA, exclusive_cmds);
 
-- 
2.31.1

Re: [PATCH v3 1/9] cxl/mbox: Move cxl_mem_command construction to helper funcs

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:18 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Sanitizing and constructing a cxl_mem_command from a userspace
> command is part of the validation process prior to submitting
> the command to a CXL device. Move this work to helper functions:
> cxl_to_mem_cmd(), cxl_to_mem_cmd_raw().
> 
> This declutters cxl_validate_cmd_from_user() in preparation for
> adding new validation steps.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

A few trivial comments inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/mbox.c | 158 +++++++++++++++++++++-------------------
>  1 file changed, 85 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index be61a0d8016b..6612d73c37a8 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -207,6 +207,84 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
>  	return true;
>  }
>  
> +static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds,
> +			      const struct cxl_send_command *send_cmd,
> +			      struct cxl_mem_command *mem_cmd)
> +{
> +	if (send_cmd->raw.rsvd)
> +		return -EINVAL;

trivial: Blank line here would be good.

> +	/*
> +	 * Unlike supported commands, the output size of RAW commands
> +	 * gets passed along without further checking, so it must be
> +	 * validated here.
> +	 */
> +	if (send_cmd->out.size > cxlds->payload_size)
> +		return -EINVAL;
> +
> +	if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode))
> +		return -EPERM;
> +
> +	*mem_cmd = (struct cxl_mem_command) {
> +		.info = {
> +			.id = CXL_MEM_COMMAND_ID_RAW,
> +			.size_in = send_cmd->in.size,
> +			.size_out = send_cmd->out.size,
> +		},
> +		.opcode = send_cmd->raw.opcode
> +	};
> +
> +	return 0;
> +}
> +
> +static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
> +			  const struct cxl_send_command *send_cmd,
> +			  struct cxl_mem_command *mem_cmd)
> +{
> +	const struct cxl_command_info *info;
> +	struct cxl_mem_command *c;
> +
> +	if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK)
> +		return -EINVAL;
> +
> +	if (send_cmd->rsvd)
> +		return -EINVAL;
> +
> +	if (send_cmd->in.rsvd || send_cmd->out.rsvd)
> +		return -EINVAL;
> +
> +	/* Convert user's command into the internal representation */

Not clear which chunk of code this applies to.  Seems like we
are just getting some addresses here (obviously that applies to original
code as well) Perhaps move down to where you fill in mem_cmd?

> +	c = &cxl_mem_commands[send_cmd->id];
> +	info = &c->info;

I don't mind that much either way, but you could do these at
declaration of the local variables above, before doing the sanity checks.

> +
> +	/* Check that the command is enabled for hardware */
> +	if (!test_bit(info->id, cxlds->enabled_cmds))
> +		return -ENOTTY;
> +
> +	/* Check that the command is not claimed for exclusive kernel use */
> +	if (test_bit(info->id, cxlds->exclusive_cmds))
> +		return -EBUSY;
> +
> +	/* Check the input buffer is the expected size */
> +	if (info->size_in >= 0 && info->size_in != send_cmd->in.size)
> +		return -ENOMEM;
> +
> +	/* Check the output buffer is at least large enough */
> +	if (info->size_out >= 0 && send_cmd->out.size < info->size_out)
> +		return -ENOMEM;
> +
> +	*mem_cmd = (struct cxl_mem_command) {
> +		.info = {
> +			.id = info->id,
> +			.flags = info->flags,
> +			.size_in = send_cmd->in.size,
> +			.size_out = send_cmd->out.size,
> +		},
> +		.opcode = c->opcode
> +	};
> +
> +	return 0;
> +}
> +
>  /**
>   * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND.
>   * @cxlds: The device data for the operation
> @@ -230,8 +308,7 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
>  				      const struct cxl_send_command *send_cmd,
>  				      struct cxl_mem_command *out_cmd)
>  {
> -	const struct cxl_command_info *info;
> -	struct cxl_mem_command *c;
> +	int rc;
>  
>  	if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX)
>  		return -ENOTTY;
> @@ -244,78 +321,13 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
>  	if (send_cmd->in.size > cxlds->payload_size)
>  		return -EINVAL;
>  
> -	/*
> -	 * Checks are bypassed for raw commands but a WARN/taint will occur
> -	 * later in the callchain
> -	 */
> -	if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW) {
> -		const struct cxl_mem_command temp = {
> -			.info = {
> -				.id = CXL_MEM_COMMAND_ID_RAW,
> -				.flags = 0,
> -				.size_in = send_cmd->in.size,
> -				.size_out = send_cmd->out.size,
> -			},
> -			.opcode = send_cmd->raw.opcode
> -		};
> +	/* Sanitize and construct a cxl_mem_command */
> +	if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW)
> +		rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, out_cmd);
> +	else
> +		rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd);
>  
> -		if (send_cmd->raw.rsvd)
> -			return -EINVAL;
> -
> -		/*
> -		 * Unlike supported commands, the output size of RAW commands
> -		 * gets passed along without further checking, so it must be
> -		 * validated here.
> -		 */
> -		if (send_cmd->out.size > cxlds->payload_size)
> -			return -EINVAL;
> -
> -		if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode))
> -			return -EPERM;
> -
> -		memcpy(out_cmd, &temp, sizeof(temp));
> -
> -		return 0;
> -	}
> -
> -	if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK)
> -		return -EINVAL;
> -
> -	if (send_cmd->rsvd)
> -		return -EINVAL;
> -
> -	if (send_cmd->in.rsvd || send_cmd->out.rsvd)
> -		return -EINVAL;
> -
> -	/* Convert user's command into the internal representation */
> -	c = &cxl_mem_commands[send_cmd->id];
> -	info = &c->info;
> -
> -	/* Check that the command is enabled for hardware */
> -	if (!test_bit(info->id, cxlds->enabled_cmds))
> -		return -ENOTTY;
> -
> -	/* Check that the command is not claimed for exclusive kernel use */
> -	if (test_bit(info->id, cxlds->exclusive_cmds))
> -		return -EBUSY;
> -
> -	/* Check the input buffer is the expected size */
> -	if (info->size_in >= 0 && info->size_in != send_cmd->in.size)
> -		return -ENOMEM;
> -
> -	/* Check the output buffer is at least large enough */
> -	if (info->size_out >= 0 && send_cmd->out.size < info->size_out)
> -		return -ENOMEM;
> -
> -	memcpy(out_cmd, c, sizeof(*c));
> -	out_cmd->info.size_in = send_cmd->in.size;
> -	/*
> -	 * XXX: out_cmd->info.size_out will be controlled by the driver, and the
> -	 * specified number of bytes @send_cmd->out.size will be copied back out
> -	 * to userspace.
> -	 */
> -
> -	return 0;
> +	return rc;
I haven't read on yet so I'll assume there is more coming in this function as otherwise
you could just return directly in the two if / else paths.

Thanks,

Jonathan

>  }
>  
>  int cxl_query_cmd(struct cxl_memdev *cxlmd,

Re: [PATCH v3 2/9] cxl/mbox: Move raw command warning to raw command validation

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:19 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> This move serves two purposes: 1) Emit the warning in the raw
> command validation path, and 2) Remove the dependency on the
> struct cxl_mem_command in handle_mailbox_cmd_from_user() in
> preparation for a refactor of that function.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
FWIW as this is obviously fine.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/mbox.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 6612d73c37a8..28131c6f7fcf 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -224,6 +224,8 @@ static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds,
>  	if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode))
>  		return -EPERM;
>  
> +	dev_WARN_ONCE(cxlds->dev, true, "raw command path used\n");
> +
>  	*mem_cmd = (struct cxl_mem_command) {
>  		.info = {
>  			.id = CXL_MEM_COMMAND_ID_RAW,
> @@ -424,9 +426,6 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
>  		cxl_command_names[cmd->info.id].name, mbox_cmd.opcode,
>  		cmd->info.size_in);
>  
> -	dev_WARN_ONCE(dev, cmd->info.id == CXL_MEM_COMMAND_ID_RAW,
> -		      "raw command path used\n");
> -
>  	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
>  	if (rc)
>  		goto out;

Re: [PATCH v3 0/9] Do not allow set-partition immediate mode

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:17 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Blocking immediate mode in set-partition info triggered a
> refactoring of the send path of userspace mailbox commands.
> 
> The v1 to address the issue was a single patch [1] that inserted
> a new immediate mode check in the send path where the payload was
> available for examining. That was not in a validation function.
> 
> The v2 patchset [2] added refactoring of the send path so that
> validation work can all spawn from cxl_validate_cmd_from_user().
> 
> Here, v3 offers a finer level of refactoring:
> 
> Patches 1-7: Refactor existing code so that all validation work
> 	can spawn from cxl_validate_cmd_from_user().
> 
> 	The movement intends to cleanly rip the work of building a
> 	mailbox command away from handle_mailbox_command_from_user()
> 	and give it to cxl_validate_cmd_from_user().

This makes me wonder a bit if
cxl_validate_cmd_from_user() is an appropriate name given
it now does more than validation?

> 
> Patch 8: Blocks the immediate mode of the set-partition command.
> Patch 9: Removes CXL_PMEM exclusive commands restriction.
> 
> [1] https://lore.kernel.org/linux-cxl/20220103202100.784194-1-alison.schofield@intel.com/
> [2] https://lore.kernel.org/linux-cxl/a4daafc4b537a0b1a673c55300da7747784c4573.1645817416.git.alison.schofield@intel.com/
> 
> Changes in v3:
> - Split up the 'Centralize the validation...' patch into 6 pieces.
> Patch: cxl/mbox: Move cxl_mem_command construction to helper funcs
> - Safely initialize the cxl_mem_command structs. (Dan)
> - Remove unneeded memcpy (Dan)
> Patch: cxl/mbox: Block immediate mode in SET_PARTITION_INFO command
> - No Changes
> Patch: cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list
> - No Changes
> 
> Changes in v2:
> - Refactor the send path of userspace mbox cmds. (Dan, Ben)
> - Patch 3 commit log - update the need to block. (Dan)
> - Return -EBUSY (not -EINVAL), when blocking immediate mode. (Ben)
> - Remove unneeded cast of void (payload_in). (dan)
> - s/u64/__le64 in struct cxl_mbox_set_partition_info. (Dan)
> 
> Alison Schofield (9):
>   cxl/mbox: Move cxl_mem_command construction to helper funcs
>   cxl/mbox: Move raw command warning to raw command validation
>   cxl/mbox: Move build of user mailbox cmd to a helper function
>   cxl/mbox: Construct a users cxl_mbox_cmd in the validation path
>   cxl/mbox: Remove dependency on cxl_mem_command for a debug msg
>   cxl/mbox: Make handle_mailbox_cmd_from_user() use a mbox param
>   cxl/mbox: Move cxl_mem_command param to a local variable
>   cxl/mbox: Block immediate mode in SET_PARTITION_INFO command
>   cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list
> 
>  drivers/cxl/core/mbox.c | 311 +++++++++++++++++++++++++---------------
>  drivers/cxl/cxlmem.h    |   7 +
>  drivers/cxl/pmem.c      |   1 -
>  3 files changed, 200 insertions(+), 119 deletions(-)
> 
> 
> base-commit: 9b688fc651b9d2b633e8d959454670aba1c39162

Re: [PATCH v3 3/9] cxl/mbox: Move build of user mailbox cmd to a helper function

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:20 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> In preparation for moving the construction of a mailbox command
> to the validation path, extract the work into a helper function.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Some suggestions for possible improvements to code organization now
possible because everything is in one place.

Otherwise looks good to me.

Jonathan

> ---
>  drivers/cxl/core/mbox.c | 60 +++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 28131c6f7fcf..d4233cfb2f99 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -207,6 +207,38 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
>  	return true;
>  }
>  
> +static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
> +			   struct cxl_mbox_cmd *mbox, u16 opcode,
> +			   size_t in_size, size_t out_size, u64 in_payload)
> +{
> +	*mbox = (struct cxl_mbox_cmd) {
> +		.opcode = opcode,
> +		.size_in = in_size,
> +		.size_out = out_size,

Given we are going to possibly override this later, my inclination would
be to not set it here and instead do

	if (out_size < 0)
		mbox->size_out = cxlds->payload_size
	else
		mbox->size_out = out_size;

or
	mbox->size_out = out_size < 0 ? cxlds->payload_size : out_size;

> +	};
> +
> +	if (!in_size)
> +		goto size_out;
> +
> +	mbox->payload_in = vmemdup_user(u64_to_user_ptr(in_payload), in_size);
> +	if (!mbox->payload_in)
> +		return PTR_ERR(mbox->payload_in);
> +
> +size_out:
Why format this like this?  Using a goto to skip a couple of lines of code
is a bit ugly.

	if (in_size) {
		mbox->payload_in = vmemdup_user(u64_to_user_ptr(in_payload), in_size);
		if (!mbox->payload_in)
			return PTR_ERR(mbox->payload_in);
	}
	etc

> +	/* Prepare to handle a full payload for variable sized output */
> +	if (out_size < 0)
> +		mbox->size_out = cxlds->payload_size;
> +
> +	if (mbox->size_out) {
> +		mbox->payload_out = kvzalloc(mbox->size_out, GFP_KERNEL);
> +		if (!mbox->payload_out) {
> +			kvfree(mbox->payload_in);
> +			return -ENOMEM;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds,
>  			      const struct cxl_send_command *send_cmd,
>  			      struct cxl_mem_command *mem_cmd)
> @@ -397,27 +429,13 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
>  					s32 *size_out, u32 *retval)
>  {
>  	struct device *dev = cxlds->dev;
> -	struct cxl_mbox_cmd mbox_cmd = {
> -		.opcode = cmd->opcode,
> -		.size_in = cmd->info.size_in,
> -		.size_out = cmd->info.size_out,
> -	};
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
> -	if (cmd->info.size_out) {
> -		mbox_cmd.payload_out = kvzalloc(cmd->info.size_out, GFP_KERNEL);
> -		if (!mbox_cmd.payload_out)
> -			return -ENOMEM;
> -	}
> -
> -	if (cmd->info.size_in) {
> -		mbox_cmd.payload_in = vmemdup_user(u64_to_user_ptr(in_payload),
> -						   cmd->info.size_in);
> -		if (IS_ERR(mbox_cmd.payload_in)) {
> -			kvfree(mbox_cmd.payload_out);
> -			return PTR_ERR(mbox_cmd.payload_in);
> -		}
> -	}
> +	rc = cxl_to_mbox_cmd(cxlds, &mbox_cmd, cmd->opcode, cmd->info.size_in,
> +			     cmd->info.size_out, in_payload);
> +	if (rc)
> +		return rc;
>  
>  	dev_dbg(dev,
>  		"Submitting %s command for user\n"
> @@ -471,10 +489,6 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
>  	if (rc)
>  		return rc;
>  
> -	/* Prepare to handle a full payload for variable sized output */
> -	if (c.info.size_out < 0)
> -		c.info.size_out = cxlds->payload_size;
> -
>  	rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload,
>  					  send.out.payload, &send.out.size,
>  					  &send.retval);

Re: [PATCH v3 4/9] cxl/mbox: Construct a users cxl_mbox_cmd in the validation path

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:21 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Since the validation of a mailbox command is done as it is built,

Perhaps reword this.  I agree it's desirable to have validation
and build together but this says 'is done' and it wasn't done until
this patch.

> move that work out of the dispatch path and into the validation
> path.
> 
> This is a step in refactoring the handling of user space mailbox
> commands. The intent is to have all the validation work originate
> in cxl_validate_cmd_from_user().
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Sometimes things can get broken into too many baby steps!

This change looks odd until patch 7 given info is passed twice
in mbox_cmd and out_cmd.  Maybe note in the patch description that
out_cmd will be brought local in a few patches time?
Probably not worth working out how to reorder and squish the patches
to make this easier to review.

Otherwise, one trivial inline.


Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/mbox.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d4233cfb2f99..205e671307c3 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -324,6 +324,7 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
>   * @cxlds: The device data for the operation
>   * @send_cmd: &struct cxl_send_command copied in from userspace.
>   * @out_cmd: Sanitized and populated &struct cxl_mem_command.
> + * @mbox_cmd: Sanitized and populated &struct cxl_mbox_cmd.
>   *
>   * Return:
>   *  * %0	- @out_cmd is ready to send.
> @@ -340,7 +341,8 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
>   */
>  static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
>  				      const struct cxl_send_command *send_cmd,
> -				      struct cxl_mem_command *out_cmd)
> +				      struct cxl_mem_command *out_cmd,
> +				      struct cxl_mbox_cmd *mbox_cmd)
>  {
>  	int rc;
>  
> @@ -361,6 +363,14 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
>  	else
>  		rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd);
>  
> +	if (rc)
> +		return rc;
> +
> +	/* Sanitize and construct a cxl_mbox_cmd */
> +	rc = cxl_to_mbox_cmd(cxlds, mbox_cmd, out_cmd->opcode,
> +			     out_cmd->info.size_in, out_cmd->info.size_out,
> +			     send_cmd->in.payload);
> +
return cxl_to_mbox_cmd()....

>  	return rc;
>  }
>  
> @@ -478,6 +488,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
>  	struct device *dev = &cxlmd->dev;
>  	struct cxl_send_command send;
>  	struct cxl_mem_command c;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
>  	dev_dbg(dev, "Send IOCTL\n");
> @@ -485,7 +496,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
>  	if (copy_from_user(&send, s, sizeof(send)))
>  		return -EFAULT;
>  
> -	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c);
> +	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c, &mbox_cmd);
>  	if (rc)
>  		return rc;
>  

Re: [PATCH v3 5/9] cxl/mbox: Remove dependency on cxl_mem_command for a debug msg

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:22 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> In preparation for removing access to struct cxl_mem_command,
> change this debug message to use cxl_mbox_cmd fields instead.
> Retrieve the pretty command name from cxl_mbox_cmd using a new
> opcode to command name helper.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/mbox.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 205e671307c3..d6d582baa1ee 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -127,6 +127,17 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
>  	return NULL;
>  }
>  
> +static const char *cxl_mem_opcode_to_name(u16 opcode)
> +{
> +	struct cxl_mem_command *c;
> +
> +	c = cxl_mem_find_command(opcode);
> +	if (c)
> +		return cxl_command_names[c->info.id].name;

nitpick, but i'd prefer the error path out of line.
	if (!c)
		return NULL;

	return cxl_command_names[c->info.id].name;

Otherwise
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> +
> +	return NULL;
> +}
> +
>  /**
>   * cxl_mbox_send_cmd() - Send a mailbox command to a device.
>   * @cxlds: The device data for the operation
> @@ -450,9 +461,9 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
>  	dev_dbg(dev,
>  		"Submitting %s command for user\n"
>  		"\topcode: %x\n"
> -		"\tsize: %ub\n",
> -		cxl_command_names[cmd->info.id].name, mbox_cmd.opcode,
> -		cmd->info.size_in);
> +		"\tsize: %zx\n",
> +		cxl_mem_opcode_to_name(mbox_cmd.opcode),
> +		mbox_cmd.opcode, mbox_cmd.size_in);
>  
>  	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
>  	if (rc)

Re: [PATCH v3 6/9] cxl/mbox: Make handle_mailbox_cmd_from_user() use a mbox param

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:23 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Previously, handle_mailbox_cmd_from_user(), constructed the mailbox
> command and dispatched it to the hardware. The construction work
> has moved to the validation path.
> 
> handle_mailbox_cmd_from_user() now expects a fully validated
> mbox param. Make it's caller, cxl_send_cmd(), deliver it. Update
> the comments and dereferencing of the new mbox parameter.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

One suggestion below.

> ---
>  drivers/cxl/core/mbox.c | 45 +++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d6d582baa1ee..0340399c7029 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -422,8 +422,7 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
>  /**
>   * handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace.
>   * @cxlds: The device data for the operation
> - * @cmd: The validated command.
> - * @in_payload: Pointer to userspace's input payload.
> + * @mbox_cmd: The validated mailbox command.
>   * @out_payload: Pointer to userspace's output payload.
>   * @size_out: (Input) Max payload size to copy out.
>   *            (Output) Payload size hardware generated.
> @@ -438,34 +437,27 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
>   *  * %-EINTR	- Mailbox acquisition interrupted.
>   *  * %-EXXX	- Transaction level failures.
>   *
> - * Creates the appropriate mailbox command and dispatches it on behalf of a
> - * userspace request. The input and output payloads are copied between
> - * userspace.
> + * Dispatches a mailbox command on behalf of a userspace request.
> + * The output payload is copied to userspace.
>   *
>   * See cxl_send_cmd().
>   */
>  static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> -					const struct cxl_mem_command *cmd,
> -					u64 in_payload, u64 out_payload,
> -					s32 *size_out, u32 *retval)
> +					struct cxl_mbox_cmd *mbox_cmd,
> +					u64 out_payload, s32 *size_out,
> +					u32 *retval)
>  {
>  	struct device *dev = cxlds->dev;
> -	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
> -	rc = cxl_to_mbox_cmd(cxlds, &mbox_cmd, cmd->opcode, cmd->info.size_in,
> -			     cmd->info.size_out, in_payload);
> -	if (rc)
> -		return rc;
> -
>  	dev_dbg(dev,
>  		"Submitting %s command for user\n"
>  		"\topcode: %x\n"
>  		"\tsize: %zx\n",
> -		cxl_mem_opcode_to_name(mbox_cmd.opcode),
> -		mbox_cmd.opcode, mbox_cmd.size_in);
> +		cxl_mem_opcode_to_name(mbox_cmd->opcode),
> +		mbox_cmd->opcode, mbox_cmd->size_in);
>  
> -	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
> +	rc = cxlds->mbox_send(cxlds, mbox_cmd);
>  	if (rc)
>  		goto out;
>  
> @@ -474,22 +466,22 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
>  	 * to userspace. While the payload may have written more output than
>  	 * this it will have to be ignored.
>  	 */
> -	if (mbox_cmd.size_out) {
> -		dev_WARN_ONCE(dev, mbox_cmd.size_out > *size_out,
> +	if (mbox_cmd->size_out) {
> +		dev_WARN_ONCE(dev, mbox_cmd->size_out > *size_out,
>  			      "Invalid return size\n");
>  		if (copy_to_user(u64_to_user_ptr(out_payload),
> -				 mbox_cmd.payload_out, mbox_cmd.size_out)) {
> +				 mbox_cmd->payload_out, mbox_cmd->size_out)) {
>  			rc = -EFAULT;
>  			goto out;
>  		}
>  	}
>  
> -	*size_out = mbox_cmd.size_out;
> -	*retval = mbox_cmd.return_code;
> +	*size_out = mbox_cmd->size_out;
> +	*retval = mbox_cmd->return_code;
>  
>  out:
> -	kvfree(mbox_cmd.payload_in);
> -	kvfree(mbox_cmd.payload_out);
> +	kvfree(mbox_cmd->payload_in);
> +	kvfree(mbox_cmd->payload_out);

As this function is no longer responsible for allocating these, I'd be inclined
to pull the frees out to the caller.

That will make things less fragile to any additional code that might in future
occur between

cxl_validate_cmd_from_user() and handle_mailbox_cmd_from_user()

>  	return rc;
>  }
>  
> @@ -511,9 +503,8 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
>  	if (rc)
>  		return rc;
>  
> -	rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload,
> -					  send.out.payload, &send.out.size,
> -					  &send.retval);
> +	rc = handle_mailbox_cmd_from_user(cxlds, &mbox_cmd, send.out.payload,
> +					  &send.out.size, &send.retval);
>  	if (rc)
>  		return rc;
>  

Re: [PATCH v3 7/9] cxl/mbox: Move cxl_mem_command param to a local variable

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:24 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> cxl_validate_command_from_user() is now the single point of validation
> for mailbox commands coming from user space. Previously, it returned a
> a cxl_mem_command, but that was not sufficient when validation of the
> actual mailbox command became a requirement. Now, it returns a fully
> validated cxl_mbox_cmd.
> 
> Remove the extraneous cxl_mem_command parameter. Define and use a
> local version only.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/mbox.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 0340399c7029..c323d6792405 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -331,10 +331,9 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
>  }
>  
>  /**
> - * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND.
> + * cxl_validate_cmd_from_user() - Construct a valid cxl_mbox_cmd
>   * @cxlds: The device data for the operation
>   * @send_cmd: &struct cxl_send_command copied in from userspace.
> - * @out_cmd: Sanitized and populated &struct cxl_mem_command.
>   * @mbox_cmd: Sanitized and populated &struct cxl_mbox_cmd.
>   *
>   * Return:
> @@ -345,16 +344,14 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
>   *  * %-EPERM	- Attempted to use a protected command.
>   *  * %-EBUSY	- Kernel has claimed exclusive access to this opcode
>   *
> - * The result of this command is a fully validated command in @out_cmd that is
> + * The result of this command is a fully validated command in @mbox_cmd that is
>   * safe to send to the hardware.
> - *
> - * See handle_mailbox_cmd_from_user()
>   */
>  static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
>  				      const struct cxl_send_command *send_cmd,
> -				      struct cxl_mem_command *out_cmd,
>  				      struct cxl_mbox_cmd *mbox_cmd)
>  {
> +	struct cxl_mem_command mem_cmd;
>  	int rc;
>  
>  	if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX)
> @@ -370,16 +367,16 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
>  
>  	/* Sanitize and construct a cxl_mem_command */
>  	if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW)
> -		rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, out_cmd);
> +		rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, &mem_cmd);
>  	else
> -		rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd);
> +		rc = cxl_to_mem_cmd(cxlds, send_cmd, &mem_cmd);
>  
>  	if (rc)
>  		return rc;
>  
>  	/* Sanitize and construct a cxl_mbox_cmd */
> -	rc = cxl_to_mbox_cmd(cxlds, mbox_cmd, out_cmd->opcode,
> -			     out_cmd->info.size_in, out_cmd->info.size_out,
> +	rc = cxl_to_mbox_cmd(cxlds, mbox_cmd, mem_cmd.opcode,
> +			     mem_cmd.info.size_in, mem_cmd.info.size_out,
>  			     send_cmd->in.payload);
>  
>  	return rc;
> @@ -490,7 +487,6 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct device *dev = &cxlmd->dev;
>  	struct cxl_send_command send;
> -	struct cxl_mem_command c;
>  	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
> @@ -499,7 +495,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
>  	if (copy_from_user(&send, s, sizeof(send)))
>  		return -EFAULT;
>  
> -	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c, &mbox_cmd);
> +	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &mbox_cmd);
>  	if (rc)
>  		return rc;
>  

Re: [PATCH v3 8/9] cxl/mbox: Block immediate mode in SET_PARTITION_INFO command

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:25 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> User space may send the SET_PARTITION_INFO mailbox command using
> the IOCTL interface. Inspect the input payload and fail if the
> immediate flag is set.
> 
> This is the first instance of the driver inspecting an input payload
> from user space. Assume there will be more such cases and implement
> with an extensible helper.
> 
> In order for the kernel to react to an immediate partition change it
> needs to assert that the change will not affect any active decode. At
> a minimum this requires validating that the device is using HDM
> decoders instead of the CXL DVSEC for decode, and that none of the
> active HDM decoders are affected by the partition change. For now,
> just fail until that support arrives.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Follow on comment from one in earlier patch on the use of a goto
to skip the input payload case... Otherwise,

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/mbox.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h    |  7 +++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index c323d6792405..1930e203061d 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -218,6 +218,40 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
>  	return true;
>  }
>  
> +/**
> + * cxl_payload_from_user_allowed() - Check contents of in_payload.
> + * @opcode: The mailbox command opcode.
> + * @payload_in: Pointer to the input payload passed in from user space.
> + *
> + * Return:
> + *  * true	- payload_in passes check for @opcode.
> + *  * false	- payload_in contains invalid or unsupported values.
> + *
> + * The driver may inspect payload contents before sending a mailbox
> + * command from user space to the device. The intent is to reject
> + * commands with input payloads that are known to be unsafe. This
> + * check is not intended to replace the users careful selection of
> + * mailbox command parameters and makes no guarantee that the user
> + * command will succeed, nor that it is appropriate.

Channeling your inner Lawyer :)

> + *
> + * The specific checks are determined by the opcode.
> + */
> +static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
> +{
> +	switch (opcode) {
> +	case CXL_MBOX_OP_SET_PARTITION_INFO: {
> +		struct cxl_mbox_set_partition_info *pi = payload_in;
> +
> +		if (pi->flags && CXL_SET_PARTITION_IMMEDIATE_FLAG)
> +			return false;
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +	return true;
> +}
> +
>  static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
>  			   struct cxl_mbox_cmd *mbox, u16 opcode,
>  			   size_t in_size, size_t out_size, u64 in_payload)
> @@ -235,6 +269,13 @@ static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
>  	if (!mbox->payload_in)
>  		return PTR_ERR(mbox->payload_in);
>  
> +	if (!cxl_payload_from_user_allowed(opcode, mbox->payload_in)) {
> +		dev_dbg(cxlds->dev, "%s: input payload not allowed\n",
> +			cxl_mem_opcode_to_name(opcode));
> +		kvfree(mbox->payload_in);
> +		return -EBUSY;
> +	}

Ah. This is making the block before the label larger. I'm still doubtful that
the code is more readable with the goto though rather than just having
it indented one more tab.

> +
>  size_out:
>  	/* Prepare to handle a full payload for variable sized output */
>  	if (out_size < 0)
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index d5c9a273d07d..dee574527e50 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -264,6 +264,13 @@ struct cxl_mbox_set_lsa {
>  	u8 data[];
>  } __packed;
>  
> +struct cxl_mbox_set_partition_info {
> +	__le64 volatile_capacity;
> +	u8 flags;
> +} __packed;
> +
> +#define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
> +
>  /**
>   * struct cxl_mem_command - Driver representation of a memory device command
>   * @info: Command information as it exists for the UAPI

Re: [PATCH v3 9/9] cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list

[Jonathan Cameron]

On Wed, 23 Mar 2022 18:11:26 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> With SET_PARTITION_INFO on the exclusive_cmds list for the CXL_PMEM
> driver, userspace cannot execute a set-partition command without
> first unbinding the pmem driver from the device.
> 
> When userspace requests a partition change to take effect on the
> next reboot this unbind requirement is unnecessarily restrictive.
> The driver does not need to enforce an unbind because partitions
> will not change until the next reboot. Of course, userspace still
> needs to be aware that changing the size of persistent capacity
> on the next reboot will result in the loss of data stored. That
> can happen regardless of whether it is presently bound at the time
> of issuing the set-partition command.
> 
> When userspace requests a partition change to take effect immediately,
> restrictions are needed. The CXL_MEM driver currently blocks the usage
> of immediate mode, making the presence of SET_PARTITION_INFO, in this
> exclusive commands list, redundant.
> 
> In the future, when the CXL_MEM driver adds support for immediate
> changes to device partitions it will ensure that the partition change
> will not affect any active decode. That means the work will not fall
> right back here, onto the CXL_PMEM driver.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pmem.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 564d125d25ef..35c6f3af18f2 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -344,7 +344,6 @@ static __init int cxl_pmem_init(void)
>  {
>  	int rc;
>  
> -	set_bit(CXL_MEM_COMMAND_ID_SET_PARTITION_INFO, exclusive_cmds);
>  	set_bit(CXL_MEM_COMMAND_ID_SET_SHUTDOWN_STATE, exclusive_cmds);
>  	set_bit(CXL_MEM_COMMAND_ID_SET_LSA, exclusive_cmds);
>  

Re: [PATCH v3 1/9] cxl/mbox: Move cxl_mem_command construction to helper funcs

[Alison Schofield]

On Fri, Mar 25, 2022 at 10:27:24AM +0000, Jonathan Cameron wrote:
> On Wed, 23 Mar 2022 18:11:18 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Sanitizing and constructing a cxl_mem_command from a userspace
> > command is part of the validation process prior to submitting
> > the command to a CXL device. Move this work to helper functions:
> > cxl_to_mem_cmd(), cxl_to_mem_cmd_raw().
> > 
> > This declutters cxl_validate_cmd_from_user() in preparation for
> > adding new validation steps.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> A few trivial comments inline.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Thanks for the review Jonathan -

> > ---

snip
> >  
> > +static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
> > +			  const struct cxl_send_command *send_cmd,
> > +			  struct cxl_mem_command *mem_cmd)
> > +{
> > +	const struct cxl_command_info *info;
> > +	struct cxl_mem_command *c;
> > +
> > +	if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK)
> > +		return -EINVAL;
> > +
> > +	if (send_cmd->rsvd)
> > +		return -EINVAL;
> > +
> > +	if (send_cmd->in.rsvd || send_cmd->out.rsvd)
> > +		return -EINVAL;
> > +
> > +	/* Convert user's command into the internal representation */
> 
> Not clear which chunk of code this applies to.  Seems like we
> are just getting some addresses here (obviously that applies to original
> code as well) Perhaps move down to where you fill in mem_cmd?

The comment above applies to the line that immediately follow.
ie. senc_cmd->id indexes into the command array.

> 
> > +	c = &cxl_mem_commands[send_cmd->id];
> > +	info = &c->info;
> 
> I don't mind that much either way, but you could do these at
> declaration of the local variables above, before doing the sanity checks.
> 

I err'd on moving things as much 'as is' as possible to keep the
patches easier to review. I like your suggestion and will do it.
(Please like it when you see it in the next version ;))

> > +
snip

> > -
> > -	return 0;
> > +	return rc;
> I haven't read on yet so I'll assume there is more coming in this function as otherwise
> you could just return directly in the two if / else paths.

Yeah, you are seeing some foreshadowing, and you've noted similar in
other patches. I'll clean that up.

Thanks,
Alison

> 
> Thanks,
> 
> Jonathan
> 
> >  }
> >  
> >  int cxl_query_cmd(struct cxl_memdev *cxlmd,
> 

Re: [PATCH v3 6/9] cxl/mbox: Make handle_mailbox_cmd_from_user() use a mbox param

[Alison Schofield]

On Fri, Mar 25, 2022 at 11:04:43AM +0000, Jonathan Cameron wrote:
> On Wed, 23 Mar 2022 18:11:23 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Previously, handle_mailbox_cmd_from_user(), constructed the mailbox
> > command and dispatched it to the hardware. The construction work
> > has moved to the validation path.
> > 
> > handle_mailbox_cmd_from_user() now expects a fully validated
> > mbox param. Make it's caller, cxl_send_cmd(), deliver it. Update
> > the comments and dereferencing of the new mbox parameter.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> One suggestion below.
>
snip

> > @@ -474,22 +466,22 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> >  	 * to userspace. While the payload may have written more output than
> >  	 * this it will have to be ignored.
> >  	 */
> > -	if (mbox_cmd.size_out) {
> > -		dev_WARN_ONCE(dev, mbox_cmd.size_out > *size_out,
> > +	if (mbox_cmd->size_out) {
> > +		dev_WARN_ONCE(dev, mbox_cmd->size_out > *size_out,
> >  			      "Invalid return size\n");
> >  		if (copy_to_user(u64_to_user_ptr(out_payload),
> > -				 mbox_cmd.payload_out, mbox_cmd.size_out)) {
> > +				 mbox_cmd->payload_out, mbox_cmd->size_out)) {
> >  			rc = -EFAULT;
> >  			goto out;
> >  		}
> >  	}
> >  
> > -	*size_out = mbox_cmd.size_out;
> > -	*retval = mbox_cmd.return_code;
> > +	*size_out = mbox_cmd->size_out;
> > +	*retval = mbox_cmd->return_code;
> >  
> >  out:
> > -	kvfree(mbox_cmd.payload_in);
> > -	kvfree(mbox_cmd.payload_out);
> > +	kvfree(mbox_cmd->payload_in);
> > +	kvfree(mbox_cmd->payload_out);
> 
> As this function is no longer responsible for allocating these, I'd be inclined
> to pull the frees out to the caller.
> 
> That will make things less fragile to any additional code that might in future
> occur between
> 
> cxl_validate_cmd_from_user() and handle_mailbox_cmd_from_user()
> 
> >  	return rc;
> >  }

Yeah, not so graceful there. I'll pull them out to the caller, but the
caller isn't the place were they were alloc'd. It goes like this:

cxl_send_cmd() {
	copy_from_user()
	cxl_validate_cmd_from_user() - does the allocs now
	handle_mailbox_from_user() - does the frees now
	? Move the frees here ?	
	copy_to_user()
}

I'll move. See what you think in next version.

> >  
> > @@ -511,9 +503,8 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
> >  	if (rc)
> >  		return rc;
> >  
> > -	rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload,
> > -					  send.out.payload, &send.out.size,
> > -					  &send.retval);
> > +	rc = handle_mailbox_cmd_from_user(cxlds, &mbox_cmd, send.out.payload,
> > +					  &send.out.size, &send.retval);
> >  	if (rc)
> >  		return rc;
> >  
> 

Re: [PATCH v3 5/9] cxl/mbox: Remove dependency on cxl_mem_command for a debug msg

[Alison Schofield]

On Fri, Mar 25, 2022 at 10:56:00AM +0000, Jonathan Cameron wrote:
> On Wed, 23 Mar 2022 18:11:22 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > In preparation for removing access to struct cxl_mem_command,
> > change this debug message to use cxl_mbox_cmd fields instead.
> > Retrieve the pretty command name from cxl_mbox_cmd using a new
> > opcode to command name helper.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/mbox.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 205e671307c3..d6d582baa1ee 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -127,6 +127,17 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
> >  	return NULL;
> >  }
> >  
> > +static const char *cxl_mem_opcode_to_name(u16 opcode)
> > +{
> > +	struct cxl_mem_command *c;
> > +
> > +	c = cxl_mem_find_command(opcode);
> > +	if (c)
> > +		return cxl_command_names[c->info.id].name;
> 
> nitpick, but i'd prefer the error path out of line.
> 	if (!c)
> 		return NULL;
> 
> 	return cxl_command_names[c->info.id].name;

Got it, thanks!

> 
> Otherwise
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Re: [PATCH v3 8/9] cxl/mbox: Block immediate mode in SET_PARTITION_INFO command

[Alison Schofield]

On Fri, Mar 25, 2022 at 11:18:16AM +0000, Jonathan Cameron wrote:
> On Wed, 23 Mar 2022 18:11:25 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > User space may send the SET_PARTITION_INFO mailbox command using
> > the IOCTL interface. Inspect the input payload and fail if the
> > immediate flag is set.
> > 
> > This is the first instance of the driver inspecting an input payload
> > from user space. Assume there will be more such cases and implement
> > with an extensible helper.
> > 
> > In order for the kernel to react to an immediate partition change it
> > needs to assert that the change will not affect any active decode. At
> > a minimum this requires validating that the device is using HDM
> > decoders instead of the CXL DVSEC for decode, and that none of the
> > active HDM decoders are affected by the partition change. For now,
> > just fail until that support arrives.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> Follow on comment from one in earlier patch on the use of a goto
> to skip the input payload case... Otherwise,
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
snip
> >  static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
> >  			   struct cxl_mbox_cmd *mbox, u16 opcode,
> >  			   size_t in_size, size_t out_size, u64 in_payload)
> > @@ -235,6 +269,13 @@ static int cxl_to_mbox_cmd(struct cxl_dev_state *cxlds,
> >  	if (!mbox->payload_in)
> >  		return PTR_ERR(mbox->payload_in);
> >  
> > +	if (!cxl_payload_from_user_allowed(opcode, mbox->payload_in)) {
> > +		dev_dbg(cxlds->dev, "%s: input payload not allowed\n",
> > +			cxl_mem_opcode_to_name(opcode));
> > +		kvfree(mbox->payload_in);
> > +		return -EBUSY;
> > +	}
> 
> Ah. This is making the block before the label larger. I'm still doubtful that
> the code is more readable with the goto though rather than just having
> it indented one more tab.
> 
You remembered my goto from a few patches back and still don't
appreciate it ;)
Will redo.
Thanks for the reviews Jonathan!

Re: [PATCH v3 4/9] cxl/mbox: Construct a users cxl_mbox_cmd in the validation path

[Alison Schofield]

On Fri, Mar 25, 2022 at 10:54:13AM +0000, Jonathan Cameron wrote:
> On Wed, 23 Mar 2022 18:11:21 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Since the validation of a mailbox command is done as it is built,
> 
> Perhaps reword this.  I agree it's desirable to have validation
> and build together but this says 'is done' and it wasn't done until
> this patch.
> 
> > move that work out of the dispatch path and into the validation
> > path.
> > 
> > This is a step in refactoring the handling of user space mailbox
> > commands. The intent is to have all the validation work originate
> > in cxl_validate_cmd_from_user().
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> Sometimes things can get broken into too many baby steps!
> 
> This change looks odd until patch 7 given info is passed twice
> in mbox_cmd and out_cmd.  Maybe note in the patch description that
> out_cmd will be brought local in a few patches time?
> Probably not worth working out how to reorder and squish the patches
> to make this easier to review.

I'm reworking w your feedback and will take another pass at this 
piece, and either change it or document it better.

As you've seen, I did a build up of the new way, then a tear down
of the old way. Let me see if there is a graceful way of avoiding
the overlap here.

Thanks for the review.
Alison

> 
> Otherwise, one trivial inline.
> 
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> >  drivers/cxl/core/mbox.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index d4233cfb2f99..205e671307c3 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -324,6 +324,7 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
> >   * @cxlds: The device data for the operation
> >   * @send_cmd: &struct cxl_send_command copied in from userspace.
> >   * @out_cmd: Sanitized and populated &struct cxl_mem_command.
> > + * @mbox_cmd: Sanitized and populated &struct cxl_mbox_cmd.
> >   *
> >   * Return:
> >   *  * %0	- @out_cmd is ready to send.
> > @@ -340,7 +341,8 @@ static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
> >   */
> >  static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
> >  				      const struct cxl_send_command *send_cmd,
> > -				      struct cxl_mem_command *out_cmd)
> > +				      struct cxl_mem_command *out_cmd,
> > +				      struct cxl_mbox_cmd *mbox_cmd)
> >  {
> >  	int rc;
> >  
> > @@ -361,6 +363,14 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
> >  	else
> >  		rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd);
> >  
> > +	if (rc)
> > +		return rc;
> > +
> > +	/* Sanitize and construct a cxl_mbox_cmd */
> > +	rc = cxl_to_mbox_cmd(cxlds, mbox_cmd, out_cmd->opcode,
> > +			     out_cmd->info.size_in, out_cmd->info.size_out,
> > +			     send_cmd->in.payload);
> > +
> return cxl_to_mbox_cmd()....
> 
> >  	return rc;
> >  }
> >  
> > @@ -478,6 +488,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
> >  	struct device *dev = &cxlmd->dev;
> >  	struct cxl_send_command send;
> >  	struct cxl_mem_command c;
> > +	struct cxl_mbox_cmd mbox_cmd;
> >  	int rc;
> >  
> >  	dev_dbg(dev, "Send IOCTL\n");
> > @@ -485,7 +496,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
> >  	if (copy_from_user(&send, s, sizeof(send)))
> >  		return -EFAULT;
> >  
> > -	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c);
> > +	rc = cxl_validate_cmd_from_user(cxlmd->cxlds, &send, &c, &mbox_cmd);
> >  	if (rc)
> >  		return rc;
> >  
> 

Re: [PATCH v3 6/9] cxl/mbox: Make handle_mailbox_cmd_from_user() use a mbox param

[Jonathan Cameron]

On Fri, 25 Mar 2022 17:25:35 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Fri, Mar 25, 2022 at 11:04:43AM +0000, Jonathan Cameron wrote:
> > On Wed, 23 Mar 2022 18:11:23 -0700
> > alison.schofield@intel.com wrote:
> >   
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > Previously, handle_mailbox_cmd_from_user(), constructed the mailbox
> > > command and dispatched it to the hardware. The construction work
> > > has moved to the validation path.
> > > 
> > > handle_mailbox_cmd_from_user() now expects a fully validated
> > > mbox param. Make it's caller, cxl_send_cmd(), deliver it. Update
> > > the comments and dereferencing of the new mbox parameter.
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>  
> > 
> > One suggestion below.
> >  
> snip
> 
> > > @@ -474,22 +466,22 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> > >  	 * to userspace. While the payload may have written more output than
> > >  	 * this it will have to be ignored.
> > >  	 */
> > > -	if (mbox_cmd.size_out) {
> > > -		dev_WARN_ONCE(dev, mbox_cmd.size_out > *size_out,
> > > +	if (mbox_cmd->size_out) {
> > > +		dev_WARN_ONCE(dev, mbox_cmd->size_out > *size_out,
> > >  			      "Invalid return size\n");
> > >  		if (copy_to_user(u64_to_user_ptr(out_payload),
> > > -				 mbox_cmd.payload_out, mbox_cmd.size_out)) {
> > > +				 mbox_cmd->payload_out, mbox_cmd->size_out)) {
> > >  			rc = -EFAULT;
> > >  			goto out;
> > >  		}
> > >  	}
> > >  
> > > -	*size_out = mbox_cmd.size_out;
> > > -	*retval = mbox_cmd.return_code;
> > > +	*size_out = mbox_cmd->size_out;
> > > +	*retval = mbox_cmd->return_code;
> > >  
> > >  out:
> > > -	kvfree(mbox_cmd.payload_in);
> > > -	kvfree(mbox_cmd.payload_out);
> > > +	kvfree(mbox_cmd->payload_in);
> > > +	kvfree(mbox_cmd->payload_out);  
> > 
> > As this function is no longer responsible for allocating these, I'd be inclined
> > to pull the frees out to the caller.
> > 
> > That will make things less fragile to any additional code that might in future
> > occur between
> > 
> > cxl_validate_cmd_from_user() and handle_mailbox_cmd_from_user()
> >   
> > >  	return rc;
> > >  }  
> 
> Yeah, not so graceful there. I'll pull them out to the caller, but the
> caller isn't the place were they were alloc'd. It goes like this:
> 
> cxl_send_cmd() {
> 	copy_from_user()
> 	cxl_validate_cmd_from_user() - does the allocs now
> 	handle_mailbox_from_user() - does the frees now
> 	? Move the frees here ?	

Could wrap them in a function to balance with the
validate, though that would need renaming to make the connection obvious.



> 	copy_to_user()
> }
> 
> I'll move. See what you think in next version.
> 
> > >  
> > > @@ -511,9 +503,8 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
> > >  	if (rc)
> > >  		return rc;
> > >  
> > > -	rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload,
> > > -					  send.out.payload, &send.out.size,
> > > -					  &send.retval);
> > > +	rc = handle_mailbox_cmd_from_user(cxlds, &mbox_cmd, send.out.payload,
> > > +					  &send.out.size, &send.retval);
> > >  	if (rc)
> > >  		return rc;
> > >    
> >   

Re: [PATCH v3 0/9] Do not allow set-partition immediate mode

[Dan Williams]

On Fri, Mar 25, 2022 at 3:34 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 23 Mar 2022 18:11:17 -0700
> alison.schofield@intel.com wrote:
>
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Blocking immediate mode in set-partition info triggered a
> > refactoring of the send path of userspace mailbox commands.
> >
> > The v1 to address the issue was a single patch [1] that inserted
> > a new immediate mode check in the send path where the payload was
> > available for examining. That was not in a validation function.
> >
> > The v2 patchset [2] added refactoring of the send path so that
> > validation work can all spawn from cxl_validate_cmd_from_user().
> >
> > Here, v3 offers a finer level of refactoring:
> >
> > Patches 1-7: Refactor existing code so that all validation work
> >       can spawn from cxl_validate_cmd_from_user().
> >
> >       The movement intends to cleanly rip the work of building a
> >       mailbox command away from handle_mailbox_command_from_user()
> >       and give it to cxl_validate_cmd_from_user().
>
> This makes me wonder a bit if
> cxl_validate_cmd_from_user() is an appropriate name given
> it now does more than validation?

Yeah, perhaps as a follow on, I think there's renaming possible to
make it clear what the difference is between:

cxl_send_cmd
cxl_mbox_send_cmd
cxl_pci_mbox_send
cxl_validate_cmd_from_user
handle_mailbox_cmd_from_user

---

cxl_send_cmd is really just a helper for the ioctl path so perhaps:
s/cxl_query_cmd/cxl_ioctl_query/
s/cxl_send_cmd/cxl_ioctl_send/

cxl_mbox_send_cmd() is the path that kernel-internal users take that
don't need to do any copy from user work to pass buffers to
cxl_pci_mbox_send() (which needs kernel buffers from
memcpy_{to,from}io()), so perhaps:
s/cxl_mbox_send_cmd/cxl_internal_cmd_send/

cxl_pci_mbox_send seems ok if the other function names change.

cxl_validate_cmd_from_user and handle_mailbox_cmd_from_user are really
doing three distinct operations. Check the payload for correctness,
check the command against the submit policy, and construct a kernel
command buffer from the user buffer. So how about:

cxl_ioctl_send_checks() (inspired by submit_bio_checks())
cxl_ioctl_send_to_kbuf()

...whatever we pick I think it's ok if this is done after this
reorganization completes with the current names.

Re: [PATCH v3 0/9] Do not allow set-partition immediate mode

[Jonathan Cameron]

On Tue, 29 Mar 2022 18:24:50 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Fri, Mar 25, 2022 at 3:34 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 23 Mar 2022 18:11:17 -0700
> > alison.schofield@intel.com wrote:
> >  
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > Blocking immediate mode in set-partition info triggered a
> > > refactoring of the send path of userspace mailbox commands.
> > >
> > > The v1 to address the issue was a single patch [1] that inserted
> > > a new immediate mode check in the send path where the payload was
> > > available for examining. That was not in a validation function.
> > >
> > > The v2 patchset [2] added refactoring of the send path so that
> > > validation work can all spawn from cxl_validate_cmd_from_user().
> > >
> > > Here, v3 offers a finer level of refactoring:
> > >
> > > Patches 1-7: Refactor existing code so that all validation work
> > >       can spawn from cxl_validate_cmd_from_user().
> > >
> > >       The movement intends to cleanly rip the work of building a
> > >       mailbox command away from handle_mailbox_command_from_user()
> > >       and give it to cxl_validate_cmd_from_user().  
> >
> > This makes me wonder a bit if
> > cxl_validate_cmd_from_user() is an appropriate name given
> > it now does more than validation?  
> 
> Yeah, perhaps as a follow on, I think there's renaming possible to
> make it clear what the difference is between:
> 
> cxl_send_cmd
> cxl_mbox_send_cmd
> cxl_pci_mbox_send
> cxl_validate_cmd_from_user
> handle_mailbox_cmd_from_user
> 
> ---
> 
> cxl_send_cmd is really just a helper for the ioctl path so perhaps:
> s/cxl_query_cmd/cxl_ioctl_query/
> s/cxl_send_cmd/cxl_ioctl_send/
> 
> cxl_mbox_send_cmd() is the path that kernel-internal users take that
> don't need to do any copy from user work to pass buffers to
> cxl_pci_mbox_send() (which needs kernel buffers from
> memcpy_{to,from}io()), so perhaps:
> s/cxl_mbox_send_cmd/cxl_internal_cmd_send/
> 
> cxl_pci_mbox_send seems ok if the other function names change.
> 
> cxl_validate_cmd_from_user and handle_mailbox_cmd_from_user are really
> doing three distinct operations. Check the payload for correctness,
> check the command against the submit policy, and construct a kernel
> command buffer from the user buffer. So how about:
> 
> cxl_ioctl_send_checks() (inspired by submit_bio_checks())
> cxl_ioctl_send_to_kbuf()
> 
> ...whatever we pick I think it's ok if this is done after this
> reorganization completes with the current names.

Agreed. Keep this series clean, but then circle back to cleanup
the naming is a good approach.

Jonathan