* [PATCH v2 0/4] Do not allow set-partition immediate mode
@ 2022-02-25 20:30 alison.schofield
2022-02-25 20:30 ` [PATCH v2 1/4] cxl/mbox: Move cxl_mem_command construction to helper funcs alison.schofield
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: alison.schofield @ 2022-02-25 20:30 UTC (permalink / raw)
To: Ben Widawsky, Dan Williams, Ira Weiny, Vishal Verma
Cc: Alison Schofield, linux-cxl
From: Alison Schofield <alison.schofield@intel.com>
The v1 to address this 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 the 'validation' function.
Reviewer feedback led to refactoring the send path of userspace mailbox
commands so that validation work can all spawn from one place:
cxl_validate_cmd_from_user().
Hence this set:
Patches 1,2: Refactor existing code.
Patch 3: Blocks the immediate mode of the set-partition command.
Original v1 patch updated per reviewer feedback.
Patch 4: Removes CXL_PMEM exclusive commands restriction.
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)
[1] https://lore.kernel.org/linux-cxl/20220103202100.784194-1-alison.schofield@intel.com/
Alison Schofield (4):
cxl/mbox: Move cxl_mem_command construction to helper funcs
cxl/mbox: Centralize the validation of user commands
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 | 308 +++++++++++++++++++++++-----------------
drivers/cxl/cxlmem.h | 7 +
drivers/cxl/pmem.c | 1 -
3 files changed, 184 insertions(+), 132 deletions(-)
base-commit: 3e9e1e72c0c24fcbeb8c96f4e886be138f61496f
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/4] cxl/mbox: Move cxl_mem_command construction to helper funcs
2022-02-25 20:30 [PATCH v2 0/4] Do not allow set-partition immediate mode alison.schofield
@ 2022-02-25 20:30 ` alison.schofield
2022-02-25 21:31 ` Dan Williams
2022-02-25 20:30 ` [PATCH v2 2/4] cxl/mbox: Centralize the validation of user commands alison.schofield
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: alison.schofield @ 2022-02-25 20:30 UTC (permalink / raw)
To: Ben Widawsky, Dan Williams, Ira Weiny, Vishal Verma
Cc: Alison Schofield, linux-cxl
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 | 143 ++++++++++++++++++++++------------------
1 file changed, 79 insertions(+), 64 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index be61a0d8016b..06fbe6d079ba 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -207,6 +207,75 @@ 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;
+
+ dev_WARN_ONCE(cxlds->dev, true, "raw command path used\n");
+
+ mem_cmd->info.id = CXL_MEM_COMMAND_ID_RAW;
+ mem_cmd->info.flags = 0;
+ mem_cmd->info.size_in = send_cmd->in.size;
+ mem_cmd->info.size_out = send_cmd->out.size;
+ mem_cmd->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;
+
+ memcpy(mem_cmd, c, sizeof(*c));
+ mem_cmd->info.size_in = send_cmd->in.size;
+ return 0;
+}
+
/**
* cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND.
* @cxlds: The device data for the operation
@@ -230,8 +299,8 @@ 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;
+ struct cxl_mem_command mem_cmd;
+ int rc;
if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX)
return -ENOTTY;
@@ -244,70 +313,16 @@ 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, &mem_cmd);
+ else
+ rc = cxl_to_mem_cmd(cxlds, send_cmd, &mem_cmd);
- if (send_cmd->raw.rsvd)
- return -EINVAL;
+ if (rc)
+ return rc;
- /*
- * 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));
+ memcpy(out_cmd, &mem_cmd, sizeof(mem_cmd));
out_cmd->info.size_in = send_cmd->in.size;
/*
* XXX: out_cmd->info.size_out will be controlled by the driver, and the
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/4] cxl/mbox: Centralize the validation of user commands
2022-02-25 20:30 [PATCH v2 0/4] Do not allow set-partition immediate mode alison.schofield
2022-02-25 20:30 ` [PATCH v2 1/4] cxl/mbox: Move cxl_mem_command construction to helper funcs alison.schofield
@ 2022-02-25 20:30 ` alison.schofield
2022-02-25 20:31 ` [PATCH v2 3/4] cxl/mbox: Block immediate mode in SET_PARTITION_INFO command alison.schofield
2022-02-25 20:31 ` [PATCH v2 4/4] cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list alison.schofield
3 siblings, 0 replies; 6+ messages in thread
From: alison.schofield @ 2022-02-25 20:30 UTC (permalink / raw)
To: Ben Widawsky, Dan Williams, Ira Weiny, Vishal Verma
Cc: Alison Schofield, linux-cxl
From: Alison Schofield <alison.schofield@intel.com>
The validation of a user command is primarily, but not exclusively,
performed in cxl_validate_cmd_from_user(). Other functions in the
send path perform checks as the command is prepared for submission
to the device.
Centralize the command validation work in cxl_validate_cmd_from_user().
Make it return a valid cxl_mbox_cmd that is subsequently consumed by
handle_mailbox_cmd_from_user().
This reorganization is in preparation for performing additional
validation on user commands.
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
drivers/cxl/core/mbox.c | 127 ++++++++++++++++++----------------------
1 file changed, 58 insertions(+), 69 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 06fbe6d079ba..e0140864a9fd 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -277,10 +277,11 @@ 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 from
+ * the users cxl_send_command.
* @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.
@@ -290,14 +291,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 mailbox command 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;
@@ -322,13 +323,34 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
if (rc)
return rc;
- memcpy(out_cmd, &mem_cmd, sizeof(mem_cmd));
- 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.
- */
+ /* Construct the cxl_mbox_cmd */
+ memset(mbox_cmd, 0, sizeof(*mbox_cmd));
+ mbox_cmd->opcode = mem_cmd.opcode;
+ mbox_cmd->size_in = mem_cmd.info.size_in;
+
+ if (!mbox_cmd->size_in)
+ goto size_out;
+
+ mbox_cmd->payload_in = vmemdup_user(u64_to_user_ptr(send_cmd->in.payload),
+ mbox_cmd->size_in);
+ if (IS_ERR(mbox_cmd->payload_in))
+ return PTR_ERR(mbox_cmd->payload_in);
+
+size_out:
+ /* Prepare to handle a full payload for variable sized output */
+ if (mem_cmd.info.size_out < 0)
+ mbox_cmd->size_out = cxlds->payload_size;
+ else
+ mbox_cmd->size_out = mem_cmd.info.size_out;
+
+ if (mbox_cmd->size_out) {
+ mbox_cmd->payload_out = kvzalloc(mbox_cmd->size_out,
+ GFP_KERNEL);
+ if (!mbox_cmd->payload_out) {
+ kvfree(mbox_cmd->payload_in);
+ return -ENOMEM;
+ }
+ }
return 0;
}
@@ -370,67 +392,33 @@ 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 ready to send.
* @out_payload: Pointer to userspace's output payload.
- * @size_out: (Input) Max payload size to copy out.
- * (Output) Payload size hardware generated.
+ * @size_out: (Output) Payload size hardware generated.
* @retval: Hardware generated return code from the operation.
*
* Return:
* * %0 - Mailbox transaction succeeded. This implies the mailbox
* protocol completed successfully not that the operation itself
* was successful.
- * * %-ENOMEM - Couldn't allocate a bounce buffer.
* * %-EFAULT - Something happened with copy_to/from_user.
* * %-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 the 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 = {
- .opcode = cmd->opcode,
- .size_in = cmd->info.size_in,
- .size_out = cmd->info.size_out,
- };
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);
- }
- }
-
- 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);
-
- dev_WARN_ONCE(dev, cmd->info.id == CXL_MEM_COMMAND_ID_RAW,
- "raw command path used\n");
-
- rc = cxlds->mbox_send(cxlds, &mbox_cmd);
+ rc = cxlds->mbox_send(cxlds, mbox_cmd);
if (rc)
goto out;
@@ -439,22 +427,21 @@ 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;
}
@@ -463,7 +450,7 @@ 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;
dev_dbg(dev, "Send IOCTL\n");
@@ -471,17 +458,19 @@ 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, &mbox_cmd);
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;
+ dev_dbg(dev,
+ "Submitting %s command for user\n"
+ "\topcode: %x\n"
+ "\tsize: %zx\n",
+ cxl_command_names[send.id].name,
+ mbox_cmd.opcode, mbox_cmd.size_in);
- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/4] cxl/mbox: Block immediate mode in SET_PARTITION_INFO command
2022-02-25 20:30 [PATCH v2 0/4] Do not allow set-partition immediate mode alison.schofield
2022-02-25 20:30 ` [PATCH v2 1/4] cxl/mbox: Move cxl_mem_command construction to helper funcs alison.schofield
2022-02-25 20:30 ` [PATCH v2 2/4] cxl/mbox: Centralize the validation of user commands alison.schofield
@ 2022-02-25 20:31 ` alison.schofield
2022-02-25 20:31 ` [PATCH v2 4/4] cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list alison.schofield
3 siblings, 0 replies; 6+ messages in thread
From: alison.schofield @ 2022-02-25 20:31 UTC (permalink / raw)
To: Ben Widawsky, Dan Williams, Ira Weiny, Vishal Verma
Cc: Alison Schofield, linux-cxl
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 | 42 +++++++++++++++++++++++++++++++++++++++++
drivers/cxl/cxlmem.h | 7 +++++++
2 files changed, 49 insertions(+)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index e0140864a9fd..b49341d7b126 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -207,6 +207,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_mem_cmd_raw(struct cxl_dev_state *cxlds,
const struct cxl_send_command *send_cmd,
struct cxl_mem_command *mem_cmd)
@@ -336,6 +370,14 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds,
if (IS_ERR(mbox_cmd->payload_in))
return PTR_ERR(mbox_cmd->payload_in);
+ if (!cxl_payload_from_user_allowed(mbox_cmd->opcode,
+ mbox_cmd->payload_in)) {
+ dev_dbg(cxlds->dev, "%s: input payload not allowed\n",
+ cxl_command_names[mem_cmd.info.id].name);
+ kvfree(mbox_cmd->payload_in);
+ return -EBUSY;
+ }
+
size_out:
/* Prepare to handle a full payload for variable sized output */
if (mem_cmd.info.size_out < 0)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index d5c9a273d07d..db3c20e29def 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 {
+ u64 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 4/4] cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list
2022-02-25 20:30 [PATCH v2 0/4] Do not allow set-partition immediate mode alison.schofield
` (2 preceding siblings ...)
2022-02-25 20:31 ` [PATCH v2 3/4] cxl/mbox: Block immediate mode in SET_PARTITION_INFO command alison.schofield
@ 2022-02-25 20:31 ` alison.schofield
3 siblings, 0 replies; 6+ messages in thread
From: alison.schofield @ 2022-02-25 20:31 UTC (permalink / raw)
To: Ben Widawsky, Dan Williams, Ira Weiny, Vishal Verma
Cc: Alison Schofield, linux-cxl
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 quiescing of the device before
setting up the 'next' partitions. 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/unbound 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 on 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 fabdb0c6dbf2..73a2868b5f95 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/4] cxl/mbox: Move cxl_mem_command construction to helper funcs
2022-02-25 20:30 ` [PATCH v2 1/4] cxl/mbox: Move cxl_mem_command construction to helper funcs alison.schofield
@ 2022-02-25 21:31 ` Dan Williams
0 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2022-02-25 21:31 UTC (permalink / raw)
To: Schofield, Alison; +Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl
On Fri, Feb 25, 2022 at 12:27 PM <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>
> ---
> drivers/cxl/core/mbox.c | 143 ++++++++++++++++++++++------------------
> 1 file changed, 79 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index be61a0d8016b..06fbe6d079ba 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -207,6 +207,75 @@ 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;
> +
> + dev_WARN_ONCE(cxlds->dev, true, "raw command path used\n");
> +
> + mem_cmd->info.id = CXL_MEM_COMMAND_ID_RAW;
> + mem_cmd->info.flags = 0;
> + mem_cmd->info.size_in = send_cmd->in.size;
> + mem_cmd->info.size_out = send_cmd->out.size;
> + mem_cmd->opcode = send_cmd->raw.opcode;
One subtle change from the previous version to this one is that it no
longer gets automatic zero initialization by the compiler. This leaves
a subtle timebomb if 'struct cxl_mem_command' is ever updated. So I
would write this as:
*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
};
...where you can skip setting flags to 0 since that is implied by the
syntax and there is no need to worry about if all the unwritten fields
are zeroed.
> +
> + 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;
> +
> + memcpy(mem_cmd, c, sizeof(*c));
> + mem_cmd->info.size_in = send_cmd->in.size;
Perhaps for symmetry this can become:
*mem_cmd = (struct cxl_mem_command) {
.info = {
.id = info->id,
.flags = info->flags,
.size_in = send_cmd->in.size,
.size_out = info->size_out,
},
.opcode = c->opcode
};
...if I got that right...
> + return 0;
> +}
> +
> /**
> * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND.
> * @cxlds: The device data for the operation
> @@ -230,8 +299,8 @@ 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;
> + struct cxl_mem_command mem_cmd;
> + int rc;
>
> if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX)
> return -ENOTTY;
> @@ -244,70 +313,16 @@ 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, &mem_cmd);
> + else
> + rc = cxl_to_mem_cmd(cxlds, send_cmd, &mem_cmd);
>
> - if (send_cmd->raw.rsvd)
> - return -EINVAL;
> + if (rc)
> + return rc;
>
> - /*
> - * 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));
> + memcpy(out_cmd, &mem_cmd, sizeof(mem_cmd));
Given the above helpers are already copying into a pointer, why not
pass them out_cmd directly instead of the extra mem_cmd hop?
> out_cmd->info.size_in = send_cmd->in.size;
This could also be deleted since both helpers above already set
size_in = send_cmd->in.size.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-25 21:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-25 20:30 [PATCH v2 0/4] Do not allow set-partition immediate mode alison.schofield
2022-02-25 20:30 ` [PATCH v2 1/4] cxl/mbox: Move cxl_mem_command construction to helper funcs alison.schofield
2022-02-25 21:31 ` Dan Williams
2022-02-25 20:30 ` [PATCH v2 2/4] cxl/mbox: Centralize the validation of user commands alison.schofield
2022-02-25 20:31 ` [PATCH v2 3/4] cxl/mbox: Block immediate mode in SET_PARTITION_INFO command alison.schofield
2022-02-25 20:31 ` [PATCH v2 4/4] cxl/pmem: Remove CXL SET_PARTITION_INFO from exclusive_cmds list alison.schofield
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).