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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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;
>