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

[Maxim Patlasov]

Acked-by: Maxim Patlasov <mpatlasov at virtuozzo.com>

On 05/19/2016 04:40 AM, Andrey Ryabinin wrote:
> 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);