diff mbox series

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

Message ID 20220324011126.1144504-5-alison.schofield@intel.com
State Superseded
Headers show
Series Do not allow set-partition immediate mode | expand

Commit Message

Alison Schofield March 24, 2022, 1:11 a.m. UTC 
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(-)

Comments

Jonathan Cameron March 25, 2022, 10:54 a.m. UTC | #1 
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;
>
Alison Schofield March 26, 2022, 12:37 a.m. UTC | #2 
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;
> >  
>
diff mbox series

Patch

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;