linux-kernel - Re: [PATCH 1/2] mm: hugetlb: proc: check for hugetlb shared PMD in /proc/PID/smaps

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: <Y9e56ofZ+E4buuam@dhcp22.suse.cz>
Date:   Mon, 30 Jan 2023 13:36:58 +0100
From:   Michal Hocko <mhocko@...e.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        James Houghton <jthoughton@...gle.com>,
        Peter Xu <peterx@...hat.com>, Yang Shi <shy828301@...il.com>,
        Vishal Moola <vishal.moola@...il.com>,
        Matthew Wilcox <willy@...radead.org>,
        Muchun Song <songmuchun@...edance.com>, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] mm: hugetlb: proc: check for hugetlb shared PMD in
 /proc/PID/smaps

On Fri 27-01-23 17:12:05, Mike Kravetz wrote:
> On 01/27/23 15:04, Andrew Morton wrote:
> > On Fri, 27 Jan 2023 17:23:39 +0100 David Hildenbrand <david@...hat.com> wrote:
> > 
> > > On 26.01.23 23:27, Mike Kravetz wrote:
> > > > A hugetlb page will have a mapcount of 1 if mapped by multiple processes
> > > > via a shared PMD.  This is because only the first process increases the
> > > > map count, and subsequent processes just add the shared PMD page to
> > > > their page table.
> > > > 
> > > > page_mapcount is being used to decide if a hugetlb page is shared or
> > > > private in /proc/PID/smaps.  Pages referenced via a shared PMD were
> > > > incorrectly being counted as private.
> > > > 
> > > > To fix, check for a shared PMD if mapcount is 1.  If a shared PMD is
> > > > found count the hugetlb page as shared.  A new helper to check for a
> > > > shared PMD is added.
> > > > 
> > > ...
> > >
> > > > --- a/fs/proc/task_mmu.c
> > > > +++ b/fs/proc/task_mmu.c
> > > > @@ -749,8 +749,14 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
> > > >   
> > > >   		if (mapcount >= 2)
> > > >   			mss->shared_hugetlb += huge_page_size(hstate_vma(vma));
> > > > -		else
> > > > -			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
> > > > +		else {
> > > 
> > > Better:
> > > 
> > > if (mapcount >= 2 || hugetlb_pmd_shared(pte))
> > > 	mss->shared_hugetlb += huge_page_size(hstate_vma(vma));
> > > else
> > > 	mss->private_hugetlb += huge_page_size(hstate_vma(vma));
> > 
> > Yup.  And that local doesn't add any value?
> > 
> > --- a/fs/proc/task_mmu.c~mm-hugetlb-proc-check-for-hugetlb-shared-pmd-in-proc-pid-smaps-fix
> > +++ a/fs/proc/task_mmu.c
> > @@ -745,18 +745,10 @@ static int smaps_hugetlb_range(pte_t *pt
> >  			page = pfn_swap_entry_to_page(swpent);
> >  	}
> >  	if (page) {
> > -		int mapcount = page_mapcount(page);
> > -
> > -		if (mapcount >= 2)
> > +		if (page_mapcount(page) >= 2 || hugetlb_pmd_shared(pte))
> >  			mss->shared_hugetlb += huge_page_size(hstate_vma(vma));
> > -		else {
> > -			if (hugetlb_pmd_shared(pte))
> > -				mss->shared_hugetlb +=
> > -						huge_page_size(hstate_vma(vma));
> > -			else
> > -				mss->private_hugetlb +=
> > -						huge_page_size(hstate_vma(vma));
> > -		}
> > +		else
> > +			mss->private_hugetlb += huge_page_size(hstate_vma(vma));
> >  	}
> >  	return 0;
> >  }
> 
> Thank you both!  That looks much better.

Yes, this looks simple enough. My only concern would be that this
special casing might be required on other places which is hard to
evaluate. I thought PSS reported by smaps would be broken as well but it
seems pss is not really accounted for hugetlb mappings at all.

Have you tried to look into {in,de}creasing the map count of the page when
the the pte is {un}shared for it?
-- 
Michal Hocko
SUSE Labs