linux-kernel - Re: [PATCH v2] mm/folio: Avoid special handling for order value 0 in folio_set

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:   Mon, 15 May 2023 10:45:13 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Tarun Sahu <tsahu@...ux.ibm.com>, linux-mm@...ck.org,
        akpm@...ux-foundation.org, muchun.song@...ux.dev,
        aneesh.kumar@...ux.ibm.com, sidhartha.kumar@...cle.com,
        gerald.schaefer@...ux.ibm.com, linux-kernel@...r.kernel.org,
        jaypatel@...ux.ibm.com
Subject: Re: [PATCH v2] mm/folio: Avoid special handling for order value 0 in
 folio_set_order

On 05/15/23 18:16, Matthew Wilcox wrote:
> On Mon, May 15, 2023 at 10:38:09PM +0530, Tarun Sahu wrote:
> > @@ -1951,9 +1950,6 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> >  	struct page *p;
> >  
> >  	__folio_clear_reserved(folio);
> > -	__folio_set_head(folio);
> > -	/* we rely on prep_new_hugetlb_folio to set the destructor */
> > -	folio_set_order(folio, order);
> >  	for (i = 0; i < nr_pages; i++) {
> >  		p = folio_page(folio, i);
> >  
> > @@ -1999,6 +1995,9 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
> >  		if (i != 0)
> >  			set_compound_head(p, &folio->page);
> >  	}
> > +	__folio_set_head(folio);
> > +	/* we rely on prep_new_hugetlb_folio to set the destructor */
> > +	folio_set_order(folio, order);
> 
> This makes me nervous, as I said before.  This means that
> compound_head(tail) can temporarily point to a page which is not marked
> as a head page.  That's different from prep_compound_page().  You need to
> come up with some good argumentation for why this is safe, and no amount
> of testing you do can replace it -- any race in this area will be subtle.

I added comments supporting this approach in the first version of the patch.
My argument was that this is actually safer than the existing code.  That is
because we freeze the page (ref count 0) before setting compound_head(tail).
So, nobody should be taking any speculative refs on those tail pages.

In the existing code, we set the compound page order in the head before
freezing the head or any tail pages.  Therefore, speculative refs can be
taken on any of the pages while in this state.

If we want prep_compound_gigantic_folio to work like prep_compound_page
we would need to take two passes through the pages.  In the first pass,
freeze all the pages and in the second set up the compound page.
-- 
Mike Kravetz