linux-kernel - Re: [PATCH 01/75] mm/gup: Increment the page refcount before the pincount

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: <Yf2akQ8vRj21eDUC@casper.infradead.org>
Date:   Fri, 4 Feb 2022 21:28:49 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/75] mm/gup: Increment the page refcount before the
 pincount

On Fri, Feb 04, 2022 at 01:13:29PM -0800, John Hubbard wrote:
> On 2/4/22 11:57, Matthew Wilcox (Oracle) wrote:
> > We should always increase the refcount before doing anything else to
> > the page so that other page users see the elevated refcount first.
> 
> Absolutely agree in principle. Is there anything else to say, though,
> such as why this matters here? Or is the change just being done for
> "best practices"? (Which is still a very solid reason, of course.)

I'm not sure if any software examines both refcount and pincount to
determine whether it knows what all of the refcounts to a page mean,
but a human might.  If someone else calls dump_page() after we update
pincount and before we update refcount, we might make a bad decision
while debugging.  When the updates are the other way around, a spurious
extra refcount is to be expected and should not confuse anyone more than
they already are.

> > Signed-off-by: Matthew Wilcox (Oracle) <willy@...radead.org>
> > ---
> >   mm/gup.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index a9d4d724aef7..08020987dfc0 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -220,18 +220,18 @@ bool __must_check try_grab_page(struct page *page, unsigned int flags)
> >   		if (WARN_ON_ONCE(page_ref_count(page) <= 0))
> >   			return false;
> > -		if (hpage_pincount_available(page))
> > -			hpage_pincount_add(page, 1);
> > -		else
> > -			refs = GUP_PIN_COUNTING_BIAS;
> > -
> >   		/*
> >   		 * Similar to try_grab_compound_head(): even if using the
> >   		 * hpage_pincount_add/_sub() routines, be sure to
> >   		 * *also* increment the normal page refcount field at least
> >   		 * once, so that the page really is pinned.
> >   		 */
> > -		page_ref_add(page, refs);
> 
> A fine point: this hunk removes the last use of "refs", which means that
> this patch will lead to an unused variable warning. So I think it would
> be best to remove the "int refs = 1;" line in this patch, rather than
> waiting until patch #10.

Argh!  I noticed that I needed to do that, and then forgot to do it.
Thanks!

> With that change, please feel free to add:
> 
> Reviewed-by: John Hubbard <jhubbard@...dia.com>

Added, and git tree updated.