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

[Alison Schofield <alison.schofield@intel.com>]

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