[Devel] [PATCH rh7] ploop: avoid costly user-controllable kmalloc() calls

[Andrey Ryabinin]

Certain ploop's ioctls copy large arrays from user space into
kernel memory allocated via kmalloc() call.
User-controllable kmalloc() is bad as it could taint the kernel
(if size > MAX_ORDER). Also allocation of large physically contiguous
memory is slow and not reliable in low-memory situations.

So instead of preallocating memory and coping big chunk of user memory
into the kernel, we copy only one element that we need at the moment.
Such approach is much more reliable, may save us several MBs of kernel
memory, and might be even faster.

https://jira.sw.ru/browse/PSBM-47303

Signed-off-by: Andrey Ryabinin <aryabinin at virtuozzo.com>
---
 drivers/block/ploop/dev.c | 73 +++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 41 deletions(-)

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index f3bb092..c4f81df 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -4167,7 +4167,7 @@ static int ploop_freeblks_ioc(struct ploop_device *plo, unsigned long arg)
 {
 	struct ploop_delta *delta;
 	struct ploop_freeblks_ctl ctl;
-	struct ploop_freeblks_ctl_extent *extents;
+	struct ploop_freeblks_ctl_extent __user *extents;
 	struct ploop_freeblks_desc *fbd;
 	int i;
 	int rc = 0;
@@ -4184,34 +4184,35 @@ static int ploop_freeblks_ioc(struct ploop_device *plo, unsigned long arg)
 	if (copy_from_user(&ctl, (void*)arg, sizeof(ctl)))
 		return -EFAULT;
 
-	extents = kzalloc(sizeof(*extents) * ctl.n_extents, GFP_KERNEL);
-	if (!extents)
-		return -ENOMEM;
-
 	delta = ploop_top_delta(plo);
 	if (delta->level != ctl.level) {
 		rc = -EINVAL;
-		goto free_extents;
-	}
-
-	if (copy_from_user(extents, (u8*)arg + sizeof(ctl),
-			   sizeof(*extents) * ctl.n_extents)) {
-		rc = -EINVAL;
-		goto free_extents;
+		goto exit;
 	}
 
 	fbd = ploop_fb_init(plo);
 	if (!fbd) {
 		rc = -ENOMEM;
-		goto free_extents;
+		goto exit;
 	}
 
+	extents = (void __user *)(arg + sizeof(ctl));
+
 	for (i = 0; i < ctl.n_extents; i++) {
-		rc = ploop_fb_add_free_extent(fbd, extents[i].clu,
-					      extents[i].iblk, extents[i].len);
+		struct ploop_freeblks_ctl_extent extent;
+
+		if (copy_from_user(&extent, &extents[i],
+					sizeof(extent))) {
+			rc = -EFAULT;
+			ploop_fb_fini(fbd, rc);
+			goto exit;
+		}
+
+		rc = ploop_fb_add_free_extent(fbd, extent.clu,
+					extent.iblk, extent.len);
 		if (rc) {
 			ploop_fb_fini(fbd, rc);
-			goto free_extents;
+			goto exit;
 		}
 	}
 
@@ -4232,8 +4233,7 @@ static int ploop_freeblks_ioc(struct ploop_device *plo, unsigned long arg)
 	}
 
 	ploop_relax(plo);
-free_extents:
-	kfree(extents);
+exit:
 	return rc;
 }
 
@@ -4324,13 +4324,8 @@ static void ploop_relocblks_process(struct ploop_device *plo)
 	spin_unlock_irq(&plo->lock);
 }
 
-static int release_fbd(struct ploop_device *plo,
-		       struct ploop_relocblks_ctl_extent *e,
-		       int err)
+static int release_fbd(struct ploop_device *plo, int err)
 {
-	if (e)
-		kfree(e);
-
 	clear_bit(PLOOP_S_DISCARD, &plo->state);
 
 	ploop_quiesce(plo);
@@ -4378,7 +4373,6 @@ static int ploop_relocblks_ioc(struct ploop_device *plo, unsigned long arg)
 {
 	struct ploop_delta *delta = ploop_top_delta(plo);
 	struct ploop_relocblks_ctl ctl;
-	struct ploop_relocblks_ctl_extent *extents = NULL;
 	struct ploop_freeblks_desc *fbd = plo->fbd;
 	int i;
 	int err = 0;
@@ -4406,26 +4400,23 @@ static int ploop_relocblks_ioc(struct ploop_device *plo, unsigned long arg)
 		goto already;
 
 	if (ctl.n_extents) {
-		extents = kzalloc(sizeof(*extents) * ctl.n_extents,
-				GFP_KERNEL);
-		if (!extents)
-			return release_fbd(plo, extents, -ENOMEM);
+		struct ploop_relocblks_ctl_extent __user *extents;
 
-		if (copy_from_user(extents, (u8*)arg + sizeof(ctl),
-					sizeof(*extents) * ctl.n_extents))
-			return release_fbd(plo, extents, -EINVAL);
+		extents = (void __user *)(arg + sizeof(ctl));
 
 		for (i = 0; i < ctl.n_extents; i++) {
-			err = ploop_fb_add_reloc_extent(fbd, extents[i].clu,
-					extents[i].iblk,
-					extents[i].len,
-					extents[i].free);
+			struct ploop_relocblks_ctl_extent extent;
+
+			if (copy_from_user(&extent, &extents[i],
+						sizeof(extent)))
+				return release_fbd(plo, -EFAULT);
+
+			/* this extent is also present in freemap */
+			err = ploop_fb_add_reloc_extent(fbd, extent.clu,
+					extent.iblk, extent.len, extent.free);
 			if (err)
-				return release_fbd(plo, extents, err);
+				return release_fbd(plo, err);
 		}
-
-		kfree(extents);
-		extents = NULL;
 	}
 
 	ploop_quiesce(plo);
@@ -4472,7 +4463,7 @@ already:
 	}
 
 	if (ploop_fb_get_n_relocated(fbd) != ploop_fb_get_n_relocating(fbd))
-		return release_fbd(plo, extents, -EIO);
+		return release_fbd(plo, -EIO);
 
 	/* time to truncate */
 	ploop_quiesce(plo);
-- 
2.7.3