Re: [PATCH] mm/memory: add refcount for special mapping page in copy_one_pte()

[Michal Hocko <mhocko@kernel.org>]

On Sat 18-04-20 11:12:07, chenqiwu wrote:
> On Fri, Apr 17, 2020 at 05:23:31PM +0200, Michal Hocko wrote:
> > On Fri 17-04-20 22:26:18, chenqiwu wrote:
> > > On Fri, Apr 17, 2020 at 01:43:12PM +0200, Michal Hocko wrote:
> > > > On Fri 17-04-20 18:31:54, qiwuchen55@gmail.com wrote:
> > > > > From: chenqiwu <chenqiwu@xiaomi.com>
> > > > > 
> > > > > If we get a special mapping page like device mapping page or zero page
> > > > > when copy_one_pte, it's necessary add the page refcount count.
> > > > 
> > > > From the changelog it is not clear what is the actual problem and how
> > > > the patch address it. Please be more verbose.
> > > >
> > > I don't find any actual problem, but I think there should be addressed
> > > to update the page refcount for special mappings include devmap and zero
> > > page instead of doing nothing else, since we copy the pte from one task
> > > to the other.
> > 
> > As Matthew pointed out, zero pages are special. Just check how
> > vm_normal_page returns NULL (the same is the case for pte_devmap). This
> > means, among other things that zap_pte_range which is called during
> > munmap will only clear the pte but it doesn't operate on those pages so
> > there is no put_page for your get_page here.
> > 
> > I do realize that this might be a subtle details that might be
> > confusing. On the other hand trying to formulate the specific problem
> > and add an explanation of the fix in the changelog could have revealed
> > this. It is really trivial to generate mappings backed by zero pages and
> > if the reference count was not handled properly then it would blow up
> > pretty quickly.
> >
> I agree. But I can't see where to put the normal page refcount back
> in zap_pte_range(), is there an imbalnace between copy_one_pte() and
> zap_pte_range()?

No, there is no imbalance. The reference counter handling is very well
hidden. Each page gets tracked by __tlb_remove_page and later handled
tlb_flush_mmu. That all is done to optimize tlb flushing which is
necessary when pages are freed.
-- 
Michal Hocko
SUSE Labs