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

On Sun, Aug 14, 2016 at 17:24:46 +0000, Leon Romanovsky [off-list ref] wrote:

On Sun, Aug 14, 2016 at 02:25:36PM +0300, Yuval Shaia wrote:

quoted

On Sun, Jul 31, 2016 at 09:15:47AM +0300, Leon Romanovsky wrote:

quoted

On Fri, Jul 29, 2016 at 09:31:16PM +0000, Adit Ranadive wrote:

quoted

On Fri, 29 Jul 2016 12:37:29 +0000, Leon Romanovsky [off-list ref] wrote:

quoted

On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote:

quoted

On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia [off-list ref] wrote:

quoted

On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote:

quoted

This patch enables posting Verb requests and receiving responses to/from
the backend PVRDMA emulation layer.

...

quoted

quoted

+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.

I understand that it is not such realistic that one of the structures in
union pvrdma_cmd_req will be more then PAGE_SIZE but any other alternative
to protect from such case?

BUILD_BUG_ON(...) is a perfect way to achieve it.

Thanks for your comments, Leon and Yuval! I will add that in v4 of the patch series.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html