From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752897AbdBJQSJ convert rfc822-to-8bit (ORCPT <rfc822;w@1wt.eu>);
        Fri, 10 Feb 2017 11:18:09 -0500
Received: from aserp1040.oracle.com ([141.146.126.69]:26664 "EHLO
        aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752533AbdBJQSI (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 10 Feb 2017 11:18:08 -0500
Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
To: Paul Durrant <paul.durrant@citrix.com>, xen-devel@lists.xenproject.org,
        linux-kernel@vger.kernel.org
References: <1486736677-10953-1-git-send-email-paul.durrant@citrix.com>
 <1486736677-10953-3-git-send-email-paul.durrant@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <2937e443-e83f-a8c4-674e-33a49d8d4ba1@oracle.com>
Date: Fri, 10 Feb 2017 11:17:36 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.6.0
MIME-Version: 1.0
In-Reply-To: <1486736677-10953-3-git-send-email-paul.durrant@citrix.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8BIT
X-Source-IP: aserv0022.oracle.com [141.146.126.234]
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 02/10/2017 09:24 AM, Paul Durrant wrote:
> +static long privcmd_ioctl_dm_op(void __user *udata)
> +{
> +	struct privcmd_dm_op kdata;
> +	struct privcmd_dm_op_buf *kbufs;
> +	unsigned int nr_pages = 0;
> +	struct page **pages = NULL;
> +	struct xen_dm_op_buf *xbufs = NULL;
> +	unsigned int i;
> +	long rc;
> +
> +	if (copy_from_user(&kdata, udata, sizeof(kdata)))
> +		return -EFAULT;
> +
> +	if (kdata.num == 0)
> +		return 0;
> +
> +	/*
> +	 * Set a tolerable upper limit on the number of buffers
> +	 * without being overly restrictive, since we can't easily
> +	 * predict what future dm_ops may require.
> +	 */

I think this deserves its own macro since it really has nothing to do
with page size, has it? Especially since you are referencing it again
below too.


> +	if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> +		return -E2BIG;
> +
> +	kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
> +	if (!kbufs)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(kbufs, kdata.ubufs,
> +			   sizeof(*kbufs) * kdata.num)) {
> +		rc = -EFAULT;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < kdata.num; i++) {
> +		if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
> +			       kbufs[i].size)) {
> +			rc = -EFAULT;
> +			goto out;
> +		}
> +
> +		nr_pages += DIV_ROUND_UP(
> +			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> +			PAGE_SIZE);
> +	}
> +
> +	/*
> +	 * Again, set a tolerable upper limit on the number of pages
> +	 * needed to lock all the buffers without being overly
> +	 * restrictive, since we can't easily predict the size of
> +	 * buffers future dm_ops may use.
> +	 */

OTOH, these two cases describe different types of copying (the first one
is for buffer descriptors and the second is for buffers themselves). And
so should they be limited by the same value?

> +	if (nr_pages * sizeof(*pages) > PAGE_SIZE) {
> +		rc = -E2BIG;
> +		goto out;
> +	}
> +
> +	pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> +	if (!pages) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
> +	if (!xbufs) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);


Aren't those buffers already locked (as Andrew mentioned)? They are
mmapped with MAP_LOCKED.

And I also wonder whether we need to take rlimit(RLIMIT_MEMLOCK) into
account.

-boris