Re: [PATCH v2 02/15] IB/pvrdma: Add device command support

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

On Fri, Jul 29, 2016 at 09:31:16PM +0000, Adit Ranadive wrote:
> On Fri, 29 Jul 2016 12:37:29 +0000, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote:
> > > On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia-QHcLZuEGTsthl2p70BpVqQ@public.gmane.orgm> wrote:
> > > > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote:
> > > > > This patch enables posting Verb requests and receiving responses to/from
> > > > > the backend PVRDMA emulation layer.
> > > > >
> > >
> > > ...
> > >
> > > > > +int
> > > > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req,
> > > > > +		bool expect_resp, union pvrdma_cmd_resp *resp)
> > > > > +{
> > > > > +	int err;
> > > > > +
> > > > > +	dev_dbg(&dev->pdev->dev, "post request to device\n");
> > > > > +
> > > > > +	/* Serializiation */
> > > > > +	down(&dev->cmd_sema);
> > > > > +
> > > > > +	spin_lock(&dev->cmd_lock);
> > > > > +	memcpy(dev->cmd_slot, req, sizeof(*req));
> > > >
> > > > Just to protect against memory corruption suggesting to do:
> > > > 	memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req)));
> > > >
> > >
> > > Done.
> > 
> > Hi Adit,
> > I didn't check how "dev->cmd_slot" was declared and if it is equal to
> > req in size. Additionally I didn't check if memcpy has any size
> > limitations, but wanted to warn you that implementation of this
> > suggestion as is will leave your system in much worse situation of
> > partially copied data.
> > 
> > if you really want to check sizes, please do it with BUILD_ON compilation
> > macro. I have serious doubts if you need it.
> > 
> > Thanks
> 
> Hi Leon,
> 
> Thanks for taking a look!
> I think this is okay to do was that we allocate a page for cmd_slot anyways
> and the size of req will always be smaller than PAGE_SIZE. If the request is
> malformed, our virtual hardware should throw an error for it.
> I dont think we would run into partially copied data.
> 
> Having said that, I dont see a necessary advantage of using min here so am
> willing to drop it if you feel strongly about it.
> 
> Can you clarify what you mean by memcpy size limitations?

I didn't have time to look after reasons for Yuval's comment about
min(..), so I assumed that one of the possibilities will be limitation
of memcpy which I'm not aware.

> 
> Thanks,
> Adit

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]