| Message ID | 20220324011126.1144504-7-alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Do not allow set-partition immediate mode | expand |
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; >
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; > > >
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; > > > > >
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;