linux-kernel - Re: [PATCH v2 6/9] mm: free user PTE page table pages

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]

Date:   Wed, 1 Sep 2021 13:16:13 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Qi Zheng <zhengqi.arch@...edance.com>, akpm@...ux-foundation.org,
        tglx@...utronix.de, hannes@...xchg.org, mhocko@...nel.org,
        vdavydov.dev@...il.com, kirill.shutemov@...ux.intel.com,
        mika.penttila@...tfour.com, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        songmuchun@...edance.com
Subject: Re: [PATCH v2 6/9] mm: free user PTE page table pages

On Wed, Sep 01, 2021 at 06:13:07PM +0200, David Hildenbrand wrote:
> On 01.09.21 17:32, Jason Gunthorpe wrote:
> > On Wed, Sep 01, 2021 at 03:57:09PM +0200, David Hildenbrand wrote:
> > > On 01.09.21 15:53, Jason Gunthorpe wrote:
> > > > On Thu, Aug 19, 2021 at 11:18:55AM +0800, Qi Zheng wrote:
> > > > 
> > > > > diff --git a/mm/gup.c b/mm/gup.c
> > > > > index 2630ed1bb4f4..30757f3b176c 100644
> > > > > +++ b/mm/gup.c
> > > > > @@ -500,6 +500,9 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
> > > > >    	if (unlikely(pmd_bad(*pmd)))
> > > > >    		return no_page_table(vma, flags);
> > > > > +	if (!pte_try_get(mm, pmd))
> > > > > +		return no_page_table(vma, flags);
> > > > > +
> > > > >    	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
> > > > 
> > > > This is not good on a performance path, the pte_try_get() is
> > > > locking/locking the same lock that pte_offset_map_lock() is getting.
> > > 
> > > Yes, and we really need patch #8, anything else is just confusing reviewers.
> > 
> > It is a bit better with patch 8, but it is still not optimal, we don't
> > need to do the atomic work at all if the entire ptep is accessed while
> > locked. So the above is stil not what I would expect here, even with
> > RCU.
> > 
> > eg I would expect that this kind of change would work first with the
> > existing paired acessors, ie
> > 
> > 	pte = pte_offset_map(pmd, address);
> > 	pte_unmap(pte);
> > 
> > Should handle the refcount under the covers, and same kind of idea for
> > the _locked/_unlocked varient.
> 
> See my other mail.

Do you have a reference?

> > Only places that don't already use that pairing should get modified.
> > 
> > To do this we have to extend the API so that pte_offset_map() can
> > fail, or very cleverly return some kind of global non-present pte page
> > (I wonder if the zero page would work?)
> 
> I explored both ideas (returning NULL, return a specially prepared page) and
> it didn't work in some cases where we unmap+remap etc.

I wouldn't think it works everywhere, bit it works in a lot of places,
and it is a heck of a lot better than what is proposed here. I'd
rather see the places that can use it be moved, and the few places
that can't be opencoded.

Jason