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

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

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