From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2195BC32750 for ; Fri, 2 Aug 2019 06:10:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EBD562087C for ; Fri, 2 Aug 2019 06:10:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388420AbfHBGKN (ORCPT ); Fri, 2 Aug 2019 02:10:13 -0400 Received: from mx2.suse.de ([195.135.220.15]:33608 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727403AbfHBGKN (ORCPT ); Fri, 2 Aug 2019 02:10:13 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 95267B634; Fri, 2 Aug 2019 06:10:09 +0000 (UTC) Subject: Re: [PATCH 20/34] xen: convert put_page() to put_user_page*() To: John Hubbard , john.hubbard@gmail.com, Andrew Morton Cc: devel@driverdev.osuosl.org, Dave Chinner , Christoph Hellwig , Dan Williams , Ira Weiny , x86@kernel.org, linux-mm@kvack.org, Dave Hansen , amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, devel@lists.orangefs.org, xen-devel@lists.xenproject.org, Boris Ostrovsky , rds-devel@oss.oracle.com, =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Jan Kara , ceph-devel@vger.kernel.org, kvm@vger.kernel.org, linux-block@vger.kernel.org, linux-crypto@vger.kernel.org, linux-fbdev@vger.kernel.org, linux-fsdevel@vger.kernel.org, LKML , linux-media@vger.kernel.org, linux-nfs@vger.kernel.org, linux-rdma@vger.kernel.org, linux-xfs@vger.kernel.org, netdev@vger.kernel.org, sparclinux@vger.kernel.org, Jason Gunthorpe References: <20190802022005.5117-1-jhubbard@nvidia.com> <20190802022005.5117-21-jhubbard@nvidia.com> <4471e9dc-a315-42c1-0c3c-55ba4eeeb106@suse.com> From: Juergen Gross Message-ID: Date: Fri, 2 Aug 2019 08:10:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------8BBD3C4A32BE2A4FA02D8356" Content-Language: de-DE Sender: linux-crypto-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-crypto@vger.kernel.org This is a multi-part message in MIME format. --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 02.08.19 07:48, John Hubbard wrote: > On 8/1/19 9:36 PM, Juergen Gross wrote: >> On 02.08.19 04:19, john.hubbard@gmail.com wrote: >>> From: John Hubbard > ... >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 2f5ce7230a43..29e461dbee2d 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -611,15 +611,10 @@ static int lock_pages( >>>   static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>>   { >>> -    unsigned int i; >>> - >>>       if (!pages) >>>           return; >>> -    for (i = 0; i < nr_pages; i++) { >>> -        if (pages[i]) >>> -            put_page(pages[i]); >>> -    } >>> +    put_user_pages(pages, nr_pages); >> >> You are not handling the case where pages[i] is NULL here. Or am I >> missing a pending patch to put_user_pages() here? >> > > Hi Juergen, > > You are correct--this no longer handles the cases where pages[i] > is NULL. It's intentional, though possibly wrong. :) > > I see that I should have added my standard blurb to this > commit description. I missed this one, but some of the other patches > have it. It makes the following, possibly incorrect claim: > > "This changes the release code slightly, because each page slot in the > page_list[] array is no longer checked for NULL. However, that check > was wrong anyway, because the get_user_pages() pattern of usage here > never allowed for NULL entries within a range of pinned pages." > > The way I've seen these page arrays used with get_user_pages(), > things are either done single page, or with a contiguous range. So > unless I'm missing a case where someone is either > > a) releasing individual pages within a range (and thus likely messing > up their count of pages they have), or > > b) allocating two gup ranges within the same pages[] array, with a > gap between the allocations, > > ...then it should be correct. If so, then I'll add the above blurb > to this patch's commit description. > > If that's not the case (both here, and in 3 or 4 other patches in this > series, then as you said, I should add NULL checks to put_user_pages() > and put_user_pages_dirty_lock(). In this case it is not correct, but can easily be handled. The NULL case can occur only in an error case with the pages array filled partially or not at all. I'd prefer something like the attached patch here. Juergen --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/x-patch; name="gup.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gup.patch" diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 2f5ce7230a43..12bd3154126d 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -582,10 +582,11 @@ static long privcmd_ioctl_mmap_batch( static int lock_pages( struct privcmd_dm_op_buf kbufs[], unsigned int num, - struct page *pages[], unsigned int nr_pages) + struct page *pages[], unsigned int *nr_pages) { - unsigned int i; + unsigned int i, free = *nr_pages; + *nr_pages = 0; for (i = 0; i < num; i++) { unsigned int requested; int pinned; @@ -593,35 +594,22 @@ static int lock_pages( requested = DIV_ROUND_UP( offset_in_page(kbufs[i].uptr) + kbufs[i].size, PAGE_SIZE); - if (requested > nr_pages) + if (requested > free) return -ENOSPC; pinned = get_user_pages_fast( (unsigned long) kbufs[i].uptr, - requested, FOLL_WRITE, pages); + requested, FOLL_WRITE, pages + *nr_pages); if (pinned < 0) return pinned; - nr_pages -= pinned; - pages += pinned; + free -= pinned; + *nr_pages += pinned; } return 0; } -static void unlock_pages(struct page *pages[], unsigned int nr_pages) -{ - unsigned int i; - - if (!pages) - return; - - for (i = 0; i < nr_pages; i++) { - if (pages[i]) - put_page(pages[i]); - } -} - static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) { struct privcmd_data *data = file->private_data; @@ -681,11 +669,12 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL); if (!xbufs) { + nr_pages = 0; rc = -ENOMEM; goto out; } - rc = lock_pages(kbufs, kdata.num, pages, nr_pages); + rc = lock_pages(kbufs, kdata.num, pages, &nr_pages); if (rc) goto out; @@ -699,7 +688,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xen_preemptible_hcall_end(); out: - unlock_pages(pages, nr_pages); + if (pages) + put_user_pages(pages, nr_pages); kfree(xbufs); kfree(pages); kfree(kbufs); --------------8BBD3C4A32BE2A4FA02D8356-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Date: Fri, 02 Aug 2019 06:10:07 +0000 Subject: Re: [PATCH 20/34] xen: convert put_page() to put_user_page*() Message-Id: MIME-Version: 1 Content-Type: multipart/mixed; boundary="------------8BBD3C4A32BE2A4FA02D8356" List-Id: References: <20190802022005.5117-1-jhubbard@nvidia.com> <20190802022005.5117-21-jhubbard@nvidia.com> <4471e9dc-a315-42c1-0c3c-55ba4eeeb106@suse.com> In-Reply-To: To: John Hubbard , john.hubbard@gmail.com, Andrew Morton Cc: linux-fbdev@vger.kernel.org, Jan Kara , kvm@vger.kernel.org, Boris Ostrovsky , Dave Hansen , Dave Chinner , dri-devel@lists.freedesktop.org, linux-mm@kvack.org, sparclinux@vger.kernel.org, Ira Weiny , Dan Williams , devel@driverdev.osuosl.org, rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org, x86@kernel.org, amd-gfx@lists.freedesktop.org, Christoph Hellwig , Jason Gunthorpe , xen-devel@lists.xenproject.org, devel@lists.orangefs.org, linux-media@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-block@vger.kernel.org, =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , linux-rpi-kernel@lists.infradead.org, ceph-devel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-nfs@vger.kerne This is a multi-part message in MIME format. --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit On 02.08.19 07:48, John Hubbard wrote: > On 8/1/19 9:36 PM, Juergen Gross wrote: >> On 02.08.19 04:19, john.hubbard@gmail.com wrote: >>> From: John Hubbard > ... >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 2f5ce7230a43..29e461dbee2d 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -611,15 +611,10 @@ static int lock_pages( >>>   static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>>   { >>> -    unsigned int i; >>> - >>>       if (!pages) >>>           return; >>> -    for (i = 0; i < nr_pages; i++) { >>> -        if (pages[i]) >>> -            put_page(pages[i]); >>> -    } >>> +    put_user_pages(pages, nr_pages); >> >> You are not handling the case where pages[i] is NULL here. Or am I >> missing a pending patch to put_user_pages() here? >> > > Hi Juergen, > > You are correct--this no longer handles the cases where pages[i] > is NULL. It's intentional, though possibly wrong. :) > > I see that I should have added my standard blurb to this > commit description. I missed this one, but some of the other patches > have it. It makes the following, possibly incorrect claim: > > "This changes the release code slightly, because each page slot in the > page_list[] array is no longer checked for NULL. However, that check > was wrong anyway, because the get_user_pages() pattern of usage here > never allowed for NULL entries within a range of pinned pages." > > The way I've seen these page arrays used with get_user_pages(), > things are either done single page, or with a contiguous range. So > unless I'm missing a case where someone is either > > a) releasing individual pages within a range (and thus likely messing > up their count of pages they have), or > > b) allocating two gup ranges within the same pages[] array, with a > gap between the allocations, > > ...then it should be correct. If so, then I'll add the above blurb > to this patch's commit description. > > If that's not the case (both here, and in 3 or 4 other patches in this > series, then as you said, I should add NULL checks to put_user_pages() > and put_user_pages_dirty_lock(). In this case it is not correct, but can easily be handled. The NULL case can occur only in an error case with the pages array filled partially or not at all. I'd prefer something like the attached patch here. Juergen --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/x-patch; name="gup.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gup.patch" diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 2f5ce7230a43..12bd3154126d 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -582,10 +582,11 @@ static long privcmd_ioctl_mmap_batch( static int lock_pages( struct privcmd_dm_op_buf kbufs[], unsigned int num, - struct page *pages[], unsigned int nr_pages) + struct page *pages[], unsigned int *nr_pages) { - unsigned int i; + unsigned int i, free = *nr_pages; + *nr_pages = 0; for (i = 0; i < num; i++) { unsigned int requested; int pinned; @@ -593,35 +594,22 @@ static int lock_pages( requested = DIV_ROUND_UP( offset_in_page(kbufs[i].uptr) + kbufs[i].size, PAGE_SIZE); - if (requested > nr_pages) + if (requested > free) return -ENOSPC; pinned = get_user_pages_fast( (unsigned long) kbufs[i].uptr, - requested, FOLL_WRITE, pages); + requested, FOLL_WRITE, pages + *nr_pages); if (pinned < 0) return pinned; - nr_pages -= pinned; - pages += pinned; + free -= pinned; + *nr_pages += pinned; } return 0; } -static void unlock_pages(struct page *pages[], unsigned int nr_pages) -{ - unsigned int i; - - if (!pages) - return; - - for (i = 0; i < nr_pages; i++) { - if (pages[i]) - put_page(pages[i]); - } -} - static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) { struct privcmd_data *data = file->private_data; @@ -681,11 +669,12 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL); if (!xbufs) { + nr_pages = 0; rc = -ENOMEM; goto out; } - rc = lock_pages(kbufs, kdata.num, pages, nr_pages); + rc = lock_pages(kbufs, kdata.num, pages, &nr_pages); if (rc) goto out; @@ -699,7 +688,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xen_preemptible_hcall_end(); out: - unlock_pages(pages, nr_pages); + if (pages) + put_user_pages(pages, nr_pages); kfree(xbufs); kfree(pages); kfree(kbufs); --------------8BBD3C4A32BE2A4FA02D8356-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH 20/34] xen: convert put_page() to put_user_page*() Date: Fri, 2 Aug 2019 08:10:07 +0200 Message-ID: References: <20190802022005.5117-1-jhubbard@nvidia.com> <20190802022005.5117-21-jhubbard@nvidia.com> <4471e9dc-a315-42c1-0c3c-55ba4eeeb106@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------8BBD3C4A32BE2A4FA02D8356" Return-path: In-Reply-To: Content-Language: de-DE List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: John Hubbard , john.hubbard@gmail.com, Andrew Morton Cc: linux-fbdev@vger.kernel.org, Jan Kara , kvm@vger.kernel.org, Boris Ostrovsky , Dave Hansen , Dave Chinner , dri-devel@lists.freedesktop.org, linux-mm@kvack.org, sparclinux@vger.kernel.org, Ira Weiny , Dan Williams , devel@driverdev.osuosl.org, rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org, x86@kernel.org, amd-gfx@lists.freedesktop.org, Christoph Hellwig , Jason Gunthorpe , xen-devel@lists.xenproject.org, devel@lists.orangefs.org, linux-media@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-block@vger.kernel.org, =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , linux-rpi-kernel@lists.infradead.org, ceph-devel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-nfs@vger.kerne List-Id: ceph-devel.vger.kernel.org This is a multi-part message in MIME format. --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 02.08.19 07:48, John Hubbard wrote: > On 8/1/19 9:36 PM, Juergen Gross wrote: >> On 02.08.19 04:19, john.hubbard@gmail.com wrote: >>> From: John Hubbard > ... >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 2f5ce7230a43..29e461dbee2d 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -611,15 +611,10 @@ static int lock_pages( >>>   static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>>   { >>> -    unsigned int i; >>> - >>>       if (!pages) >>>           return; >>> -    for (i = 0; i < nr_pages; i++) { >>> -        if (pages[i]) >>> -            put_page(pages[i]); >>> -    } >>> +    put_user_pages(pages, nr_pages); >> >> You are not handling the case where pages[i] is NULL here. Or am I >> missing a pending patch to put_user_pages() here? >> > > Hi Juergen, > > You are correct--this no longer handles the cases where pages[i] > is NULL. It's intentional, though possibly wrong. :) > > I see that I should have added my standard blurb to this > commit description. I missed this one, but some of the other patches > have it. It makes the following, possibly incorrect claim: > > "This changes the release code slightly, because each page slot in the > page_list[] array is no longer checked for NULL. However, that check > was wrong anyway, because the get_user_pages() pattern of usage here > never allowed for NULL entries within a range of pinned pages." > > The way I've seen these page arrays used with get_user_pages(), > things are either done single page, or with a contiguous range. So > unless I'm missing a case where someone is either > > a) releasing individual pages within a range (and thus likely messing > up their count of pages they have), or > > b) allocating two gup ranges within the same pages[] array, with a > gap between the allocations, > > ...then it should be correct. If so, then I'll add the above blurb > to this patch's commit description. > > If that's not the case (both here, and in 3 or 4 other patches in this > series, then as you said, I should add NULL checks to put_user_pages() > and put_user_pages_dirty_lock(). In this case it is not correct, but can easily be handled. The NULL case can occur only in an error case with the pages array filled partially or not at all. I'd prefer something like the attached patch here. Juergen --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/x-patch; name="gup.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gup.patch" diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 2f5ce7230a43..12bd3154126d 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -582,10 +582,11 @@ static long privcmd_ioctl_mmap_batch( static int lock_pages( struct privcmd_dm_op_buf kbufs[], unsigned int num, - struct page *pages[], unsigned int nr_pages) + struct page *pages[], unsigned int *nr_pages) { - unsigned int i; + unsigned int i, free = *nr_pages; + *nr_pages = 0; for (i = 0; i < num; i++) { unsigned int requested; int pinned; @@ -593,35 +594,22 @@ static int lock_pages( requested = DIV_ROUND_UP( offset_in_page(kbufs[i].uptr) + kbufs[i].size, PAGE_SIZE); - if (requested > nr_pages) + if (requested > free) return -ENOSPC; pinned = get_user_pages_fast( (unsigned long) kbufs[i].uptr, - requested, FOLL_WRITE, pages); + requested, FOLL_WRITE, pages + *nr_pages); if (pinned < 0) return pinned; - nr_pages -= pinned; - pages += pinned; + free -= pinned; + *nr_pages += pinned; } return 0; } -static void unlock_pages(struct page *pages[], unsigned int nr_pages) -{ - unsigned int i; - - if (!pages) - return; - - for (i = 0; i < nr_pages; i++) { - if (pages[i]) - put_page(pages[i]); - } -} - static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) { struct privcmd_data *data = file->private_data; @@ -681,11 +669,12 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL); if (!xbufs) { + nr_pages = 0; rc = -ENOMEM; goto out; } - rc = lock_pages(kbufs, kdata.num, pages, nr_pages); + rc = lock_pages(kbufs, kdata.num, pages, &nr_pages); if (rc) goto out; @@ -699,7 +688,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xen_preemptible_hcall_end(); out: - unlock_pages(pages, nr_pages); + if (pages) + put_user_pages(pages, nr_pages); kfree(xbufs); kfree(pages); kfree(kbufs); --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4 --------------8BBD3C4A32BE2A4FA02D8356-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: jgross@suse.com (Juergen Gross) Date: Fri, 2 Aug 2019 08:10:07 +0200 Subject: [PATCH 20/34] xen: convert put_page() to put_user_page*() In-Reply-To: References: <20190802022005.5117-1-jhubbard@nvidia.com> <20190802022005.5117-21-jhubbard@nvidia.com> <4471e9dc-a315-42c1-0c3c-55ba4eeeb106@suse.com> Message-ID: List-Id: Linux Driver Project Developer List On 02.08.19 07:48, John Hubbard wrote: > On 8/1/19 9:36 PM, Juergen Gross wrote: >> On 02.08.19 04:19, john.hubbard@gmail.com wrote: >>> From: John Hubbard > ... >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 2f5ce7230a43..29e461dbee2d 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -611,15 +611,10 @@ static int lock_pages( >>> ? static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>> ? { >>> -??? unsigned int i; >>> - >>> ????? if (!pages) >>> ????????? return; >>> -??? for (i = 0; i < nr_pages; i++) { >>> -??????? if (pages[i]) >>> -??????????? put_page(pages[i]); >>> -??? } >>> +??? put_user_pages(pages, nr_pages); >> >> You are not handling the case where pages[i] is NULL here. Or am I >> missing a pending patch to put_user_pages() here? >> > > Hi Juergen, > > You are correct--this no longer handles the cases where pages[i] > is NULL. It's intentional, though possibly wrong. :) > > I see that I should have added my standard blurb to this > commit description. I missed this one, but some of the other patches > have it. It makes the following, possibly incorrect claim: > > "This changes the release code slightly, because each page slot in the > page_list[] array is no longer checked for NULL. However, that check > was wrong anyway, because the get_user_pages() pattern of usage here > never allowed for NULL entries within a range of pinned pages." > > The way I've seen these page arrays used with get_user_pages(), > things are either done single page, or with a contiguous range. So > unless I'm missing a case where someone is either > > a) releasing individual pages within a range (and thus likely messing > up their count of pages they have), or > > b) allocating two gup ranges within the same pages[] array, with a > gap between the allocations, > > ...then it should be correct. If so, then I'll add the above blurb > to this patch's commit description. > > If that's not the case (both here, and in 3 or 4 other patches in this > series, then as you said, I should add NULL checks to put_user_pages() > and put_user_pages_dirty_lock(). In this case it is not correct, but can easily be handled. The NULL case can occur only in an error case with the pages array filled partially or not at all. I'd prefer something like the attached patch here. Juergen -------------- next part -------------- A non-text attachment was scrubbed... Name: gup.patch Type: text/x-patch Size: 2001 bytes Desc: not available URL: From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AE29DC32754 for ; Fri, 2 Aug 2019 06:10:20 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 836D0206A3 for ; Fri, 2 Aug 2019 06:10:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="arikrtqk" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 836D0206A3 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type: In-Reply-To:MIME-Version:Date:Message-ID:From:References:To:Subject:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=j9/6sUrbsj6qQQ/bIXavMOmV22QCImnUUbinj/7ReZ0=; b=arikrtqkUsBZaN4BbVTW70RnK X7mRjm/nymtOWCb/A9DqnFrMCc0PwQzoZCyptbwTrwsyF50zEaJ8UPPd7SF+xhiEhazVJoqs8yomI ssIQ36UT/ezZ3gm7uvEsWSJAlM+izEjWq2TOvB1m8EtyA6b8CwKfI82FUtj7VUdsA2LhNoO/D5j7K WZ6LEdaXnA3BhusKbrwNv1KJUhwjjUYyWTy5jie1LBHP8nMgzMmU42wZRLzKjSp6S2urkmjkdK0oz TEpoic3u0+EEyXIh+nfl5FITrTzfxJ7w3MbXjulIR9ZQFv/69k4dR0WzuzJJEtojw1CmtA+74CQyM SlOS7QdPQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1htQlf-00042q-M2; Fri, 02 Aug 2019 06:10:19 +0000 Received: from mx2.suse.de ([195.135.220.15] helo=mx1.suse.de) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1htQlY-0003tO-3E; Fri, 02 Aug 2019 06:10:13 +0000 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 95267B634; Fri, 2 Aug 2019 06:10:09 +0000 (UTC) Subject: Re: [PATCH 20/34] xen: convert put_page() to put_user_page*() To: John Hubbard , john.hubbard@gmail.com, Andrew Morton References: <20190802022005.5117-1-jhubbard@nvidia.com> <20190802022005.5117-21-jhubbard@nvidia.com> <4471e9dc-a315-42c1-0c3c-55ba4eeeb106@suse.com> From: Juergen Gross Message-ID: Date: Fri, 2 Aug 2019 08:10:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------8BBD3C4A32BE2A4FA02D8356" Content-Language: de-DE X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190801_231012_427167_83506D67 X-CRM114-Status: GOOD ( 27.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, Jan Kara , kvm@vger.kernel.org, Boris Ostrovsky , Dave Hansen , Dave Chinner , dri-devel@lists.freedesktop.org, linux-mm@kvack.org, sparclinux@vger.kernel.org, Ira Weiny , Dan Williams , devel@driverdev.osuosl.org, rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org, x86@kernel.org, amd-gfx@lists.freedesktop.org, Christoph Hellwig , Jason Gunthorpe , xen-devel@lists.xenproject.org, devel@lists.orangefs.org, linux-media@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-block@vger.kernel.org, =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , linux-rpi-kernel@lists.infradead.org, ceph-devel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-nfs@vger.kernel.org, netdev@vger.kernel.org, LKML , linux-xfs@vger.kernel.org, linux-crypto@vger.kernel.org, linux-fsdevel@vger.kernel.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org This is a multi-part message in MIME format. --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 02.08.19 07:48, John Hubbard wrote: > On 8/1/19 9:36 PM, Juergen Gross wrote: >> On 02.08.19 04:19, john.hubbard@gmail.com wrote: >>> From: John Hubbard > ... >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 2f5ce7230a43..29e461dbee2d 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -611,15 +611,10 @@ static int lock_pages( >>>   static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>>   { >>> -    unsigned int i; >>> - >>>       if (!pages) >>>           return; >>> -    for (i = 0; i < nr_pages; i++) { >>> -        if (pages[i]) >>> -            put_page(pages[i]); >>> -    } >>> +    put_user_pages(pages, nr_pages); >> >> You are not handling the case where pages[i] is NULL here. Or am I >> missing a pending patch to put_user_pages() here? >> > > Hi Juergen, > > You are correct--this no longer handles the cases where pages[i] > is NULL. It's intentional, though possibly wrong. :) > > I see that I should have added my standard blurb to this > commit description. I missed this one, but some of the other patches > have it. It makes the following, possibly incorrect claim: > > "This changes the release code slightly, because each page slot in the > page_list[] array is no longer checked for NULL. However, that check > was wrong anyway, because the get_user_pages() pattern of usage here > never allowed for NULL entries within a range of pinned pages." > > The way I've seen these page arrays used with get_user_pages(), > things are either done single page, or with a contiguous range. So > unless I'm missing a case where someone is either > > a) releasing individual pages within a range (and thus likely messing > up their count of pages they have), or > > b) allocating two gup ranges within the same pages[] array, with a > gap between the allocations, > > ...then it should be correct. If so, then I'll add the above blurb > to this patch's commit description. > > If that's not the case (both here, and in 3 or 4 other patches in this > series, then as you said, I should add NULL checks to put_user_pages() > and put_user_pages_dirty_lock(). In this case it is not correct, but can easily be handled. The NULL case can occur only in an error case with the pages array filled partially or not at all. I'd prefer something like the attached patch here. Juergen --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/x-patch; name="gup.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gup.patch" diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 2f5ce7230a43..12bd3154126d 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -582,10 +582,11 @@ static long privcmd_ioctl_mmap_batch( static int lock_pages( struct privcmd_dm_op_buf kbufs[], unsigned int num, - struct page *pages[], unsigned int nr_pages) + struct page *pages[], unsigned int *nr_pages) { - unsigned int i; + unsigned int i, free = *nr_pages; + *nr_pages = 0; for (i = 0; i < num; i++) { unsigned int requested; int pinned; @@ -593,35 +594,22 @@ static int lock_pages( requested = DIV_ROUND_UP( offset_in_page(kbufs[i].uptr) + kbufs[i].size, PAGE_SIZE); - if (requested > nr_pages) + if (requested > free) return -ENOSPC; pinned = get_user_pages_fast( (unsigned long) kbufs[i].uptr, - requested, FOLL_WRITE, pages); + requested, FOLL_WRITE, pages + *nr_pages); if (pinned < 0) return pinned; - nr_pages -= pinned; - pages += pinned; + free -= pinned; + *nr_pages += pinned; } return 0; } -static void unlock_pages(struct page *pages[], unsigned int nr_pages) -{ - unsigned int i; - - if (!pages) - return; - - for (i = 0; i < nr_pages; i++) { - if (pages[i]) - put_page(pages[i]); - } -} - static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) { struct privcmd_data *data = file->private_data; @@ -681,11 +669,12 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL); if (!xbufs) { + nr_pages = 0; rc = -ENOMEM; goto out; } - rc = lock_pages(kbufs, kdata.num, pages, nr_pages); + rc = lock_pages(kbufs, kdata.num, pages, &nr_pages); if (rc) goto out; @@ -699,7 +688,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xen_preemptible_hcall_end(); out: - unlock_pages(pages, nr_pages); + if (pages) + put_user_pages(pages, nr_pages); kfree(xbufs); kfree(pages); kfree(kbufs); --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --------------8BBD3C4A32BE2A4FA02D8356-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D4661C433FF for ; Fri, 2 Aug 2019 06:10:35 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id ACF2D206A3 for ; Fri, 2 Aug 2019 06:10:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ACF2D206A3 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1htQlb-0001jG-0B; Fri, 02 Aug 2019 06:10:15 +0000 Received: from us1-rack-dfw2.inumbo.com ([104.130.134.6]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1htQlZ-0001jB-QB for xen-devel@lists.xenproject.org; Fri, 02 Aug 2019 06:10:13 +0000 X-Inumbo-ID: 2f0f3b6f-b4ec-11e9-8980-bc764e045a96 Received: from mx1.suse.de (unknown [195.135.220.15]) by us1-rack-dfw2.inumbo.com (Halon) with ESMTPS id 2f0f3b6f-b4ec-11e9-8980-bc764e045a96; Fri, 02 Aug 2019 06:10:11 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 95267B634; Fri, 2 Aug 2019 06:10:09 +0000 (UTC) To: John Hubbard , john.hubbard@gmail.com, Andrew Morton References: <20190802022005.5117-1-jhubbard@nvidia.com> <20190802022005.5117-21-jhubbard@nvidia.com> <4471e9dc-a315-42c1-0c3c-55ba4eeeb106@suse.com> From: Juergen Gross Message-ID: Date: Fri, 2 Aug 2019 08:10:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------8BBD3C4A32BE2A4FA02D8356" Content-Language: de-DE Subject: Re: [Xen-devel] [PATCH 20/34] xen: convert put_page() to put_user_page*() X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, Jan Kara , kvm@vger.kernel.org, Boris Ostrovsky , Dave Hansen , Dave Chinner , dri-devel@lists.freedesktop.org, linux-mm@kvack.org, sparclinux@vger.kernel.org, Ira Weiny , Dan Williams , devel@driverdev.osuosl.org, rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org, x86@kernel.org, amd-gfx@lists.freedesktop.org, Christoph Hellwig , Jason Gunthorpe , xen-devel@lists.xenproject.org, devel@lists.orangefs.org, linux-media@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-block@vger.kernel.org, =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , linux-rpi-kernel@lists.infradead.org, ceph-devel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-nfs@vger.kernel.org, netdev@vger.kernel.org, LKML , linux-xfs@vger.kernel.org, linux-crypto@vger.kernel.org, linux-fsdevel@vger.kernel.org Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" This is a multi-part message in MIME format. --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 02.08.19 07:48, John Hubbard wrote: > On 8/1/19 9:36 PM, Juergen Gross wrote: >> On 02.08.19 04:19, john.hubbard@gmail.com wrote: >>> From: John Hubbard > ... >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 2f5ce7230a43..29e461dbee2d 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -611,15 +611,10 @@ static int lock_pages( >>>   static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>>   { >>> -    unsigned int i; >>> - >>>       if (!pages) >>>           return; >>> -    for (i = 0; i < nr_pages; i++) { >>> -        if (pages[i]) >>> -            put_page(pages[i]); >>> -    } >>> +    put_user_pages(pages, nr_pages); >> >> You are not handling the case where pages[i] is NULL here. Or am I >> missing a pending patch to put_user_pages() here? >> > > Hi Juergen, > > You are correct--this no longer handles the cases where pages[i] > is NULL. It's intentional, though possibly wrong. :) > > I see that I should have added my standard blurb to this > commit description. I missed this one, but some of the other patches > have it. It makes the following, possibly incorrect claim: > > "This changes the release code slightly, because each page slot in the > page_list[] array is no longer checked for NULL. However, that check > was wrong anyway, because the get_user_pages() pattern of usage here > never allowed for NULL entries within a range of pinned pages." > > The way I've seen these page arrays used with get_user_pages(), > things are either done single page, or with a contiguous range. So > unless I'm missing a case where someone is either > > a) releasing individual pages within a range (and thus likely messing > up their count of pages they have), or > > b) allocating two gup ranges within the same pages[] array, with a > gap between the allocations, > > ...then it should be correct. If so, then I'll add the above blurb > to this patch's commit description. > > If that's not the case (both here, and in 3 or 4 other patches in this > series, then as you said, I should add NULL checks to put_user_pages() > and put_user_pages_dirty_lock(). In this case it is not correct, but can easily be handled. The NULL case can occur only in an error case with the pages array filled partially or not at all. I'd prefer something like the attached patch here. Juergen --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/x-patch; name="gup.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gup.patch" diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 2f5ce7230a43..12bd3154126d 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -582,10 +582,11 @@ static long privcmd_ioctl_mmap_batch( static int lock_pages( struct privcmd_dm_op_buf kbufs[], unsigned int num, - struct page *pages[], unsigned int nr_pages) + struct page *pages[], unsigned int *nr_pages) { - unsigned int i; + unsigned int i, free = *nr_pages; + *nr_pages = 0; for (i = 0; i < num; i++) { unsigned int requested; int pinned; @@ -593,35 +594,22 @@ static int lock_pages( requested = DIV_ROUND_UP( offset_in_page(kbufs[i].uptr) + kbufs[i].size, PAGE_SIZE); - if (requested > nr_pages) + if (requested > free) return -ENOSPC; pinned = get_user_pages_fast( (unsigned long) kbufs[i].uptr, - requested, FOLL_WRITE, pages); + requested, FOLL_WRITE, pages + *nr_pages); if (pinned < 0) return pinned; - nr_pages -= pinned; - pages += pinned; + free -= pinned; + *nr_pages += pinned; } return 0; } -static void unlock_pages(struct page *pages[], unsigned int nr_pages) -{ - unsigned int i; - - if (!pages) - return; - - for (i = 0; i < nr_pages; i++) { - if (pages[i]) - put_page(pages[i]); - } -} - static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) { struct privcmd_data *data = file->private_data; @@ -681,11 +669,12 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL); if (!xbufs) { + nr_pages = 0; rc = -ENOMEM; goto out; } - rc = lock_pages(kbufs, kdata.num, pages, nr_pages); + rc = lock_pages(kbufs, kdata.num, pages, &nr_pages); if (rc) goto out; @@ -699,7 +688,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) xen_preemptible_hcall_end(); out: - unlock_pages(pages, nr_pages); + if (pages) + put_user_pages(pages, nr_pages); kfree(xbufs); kfree(pages); kfree(kbufs); --------------8BBD3C4A32BE2A4FA02D8356 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVucHJvamVjdC5vcmcKaHR0cHM6Ly9saXN0 cy54ZW5wcm9qZWN0Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL3hlbi1kZXZlbA== --------------8BBD3C4A32BE2A4FA02D8356--