From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752389AbdBJTfn convert rfc822-to-8bit (ORCPT ); Fri, 10 Feb 2017 14:35:43 -0500 Received: from aserp1050.oracle.com ([141.146.126.70]:24344 "EHLO aserp1050.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbdBJTfm (ORCPT ); Fri, 10 Feb 2017 14:35:42 -0500 Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP To: Paul Durrant , "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> <2937e443-e83f-a8c4-674e-33a49d8d4ba1@oracle.com> Cc: Juergen Gross From: Boris Ostrovsky Message-ID: Date: Fri, 10 Feb 2017 12:44:52 -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: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8BIT X-Source-IP: aserp1040.oracle.com [141.146.126.69] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/10/2017 11:28 AM, Paul Durrant wrote: >> -----Original Message----- >> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com] >> Sent: 10 February 2017 16:18 >> To: Paul Durrant ; xen-devel@lists.xenproject.org; >> linux-kernel@vger.kernel.org >> Cc: Juergen Gross >> Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP >> >> 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? >> > I think there needs to be some limit and limiting the allocation to a page was the best I came up with. Can you think of a better one? How about something like (with rather arbitrary values) #define PRIVCMD_DMOP_MAX_NUM_BUFFERS 16 #define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ 4096 and make them part of the interface (i.e. put them into privcmd.h)? -boris