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

[Konstantin Khorenko]

The commit is pushed to "branch-rh7-3.10.0-327.18.2.vz7.14.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-327.18.2.vz7.14.5
------>
commit 1f61933478251e603117125316771469f9284788
Author: Andrey Ryabinin <aryabinin at virtuozzo.com>
Date:   Fri May 20 18:27:26 2016 +0400

    ploop: avoid costly user-controllable kmalloc() calls
    
    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>
    Acked-by: Maxim Patlasov <mpatlasov 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 15e34e5..da35c5c 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);