linux-kernel - RE: [PATCH 20/34] xen: convert put_page() to put_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2807E5FD2F6FDA4886F6618EAC48510E79E66216@CRSMSX101.amr.corp.intel.com>
Date:   Fri, 2 Aug 2019 16:09:44 +0000
From:   "Weiny, Ira" <ira.weiny@...el.com>
To:     Juergen Gross <jgross@...e.com>,
        John Hubbard <jhubbard@...dia.com>,
        "john.hubbard@...il.com" <john.hubbard@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>
CC:     "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        Dave Chinner <david@...morbit.com>,
        Christoph Hellwig <hch@...radead.org>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-rpi-kernel@...ts.infradead.org" 
        <linux-rpi-kernel@...ts.infradead.org>,
        "devel@...ts.orangefs.org" <devel@...ts.orangefs.org>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        "rds-devel@....oracle.com" <rds-devel@....oracle.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Jan Kara <jack@...e.cz>,
        "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sparclinux@...r.kernel.org" <sparclinux@...r.kernel.org>,
        Jason Gunthorpe <jgg@...pe.ca>
Subject: RE: [PATCH 20/34] xen: convert put_page() to put_user_page*()

> 
> 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@...il.com wrote:
> >>> From: John Hubbard <jhubbard@...dia.com>
> > ...
> >>> 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.

I'm not an expert in this code and have not looked at it carefully but that patch does seem to be the better fix than forcing NULL checks on everyone.

Ira