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

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

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