From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751744AbdBMJED convert rfc822-to-8bit (ORCPT ); Mon, 13 Feb 2017 04:04:03 -0500 Received: from smtp.eu.citrix.com ([185.25.65.24]:60936 "EHLO SMTP.EU.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751482AbdBMJEC (ORCPT ); Mon, 13 Feb 2017 04:04:02 -0500 X-IronPort-AV: E=Sophos;i="5.35,156,1484006400"; d="scan'208";a="40642721" From: Paul Durrant To: "'Boris Ostrovsky'" , "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 Thread-Topic: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP Thread-Index: AQHSg6lryuDqq2MGNUCsZBKa112ujKFiWksAgAATDNCAAAVWAIAENdXg Date: Mon, 13 Feb 2017 09:03:59 +0000 Message-ID: <797fa92d018947299249bc92fa8511b9@AMSPEX02CL03.citrite.net> 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> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com] > Sent: 10 February 2017 17:45 > 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 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)? Given that the values are arbitrary, I think it may be better to make them module params. They can then at least be tweaked if privcmd becomes a problem with later dm_ops. Paul > > -boris