linux-kernel - Re: [PATCH v4] mm: fix is_pinnable

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 May 2022 13:21:15 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     Jason Gunthorpe <jgg@...pe.ca>, Minchan Kim <minchan@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-mm <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        John Dias <joaodias@...gle.com>,
        David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH v4] mm: fix is_pinnable_page against on cma page

On Tue, May 17, 2022 at 01:12:02PM -0700, John Hubbard wrote:
> On 5/17/22 12:28, Jason Gunthorpe wrote:
> > > If you compare this to the snippet above, you'll see that there is
> > > an extra mov statement, and that one dereferences a pointer from
> > > %rax:
> > > 
> > >      mov    (%rax),%rbx
> > 
> > That is the same move as:
> > 
> >     mov    0x8(%rdx,%rax,8),%rbx
> > 
> > Except that the EA calculation was done in advance and stored in rax.
> > 
> > lea isn't a memory reference, it is just computing the pointer value
> > that 0x8(%rdx,%rax,8) represents. ie the lea computes
> > 
> >    %rax = %rdx + %rax*8 + 6
> > 
> > Which is then fed into the mov. Maybe it is an optimization to allow
> > one pipe to do the shr and an other to the EA - IDK, seems like a
> > random thing for the compiler to do.

Maybe an optimization suppressed due to the volatile nature of the
load?  If so, perhaps it might be considered a compiler bug.  Though
it is quite difficult to get optimization bugs involving volatile
to be taken seriously.

> Apologies for getting that wrong, and thanks for walking me through the
> asm.
> 
> [...]
> > 
> > Paul can correct me, but I understand we do not have a list of allowed
> > operations that are exempted from the READ_ONCE() requirement. ie it
> > is not just conditional branching that requires READ_ONCE().
> > 
> > This is why READ_ONCE() must always be on the memory load, because the
> > point is to sanitize away the uncertainty that comes with an unlocked
> > read of unstable memory contents. READ_ONCE() samples the value in
> > memory, and removes all tearing, multiload, etc "instability" that may
> > effect down stream computations. In this way down stream compulations
> > become reliable.
> > 
> > Jason
> 
> So then:

Works for me!

Acked-by: Paul E. McKenney <paulmck@...nel.org>

							Thanx, Paul

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0e42038382c1..b404f87e2682 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -482,7 +482,12 @@ unsigned long __get_pfnblock_flags_mask(const struct page *page,
>         word_bitidx = bitidx / BITS_PER_LONG;
>         bitidx &= (BITS_PER_LONG-1);
> 
> -       word = bitmap[word_bitidx];
> +       /*
> +        * This races, without locks, with set_pageblock_migratetype(). Ensure
> +        * a consistent (non-tearing) read of the memory array, so that results,
> +        * even though racy, are not corrupted.
> +        */
> +       word = READ_ONCE(bitmap[word_bitidx]);
>         return (word >> bitidx) & mask;
>  }
> 
> 
> thanks,
> -- 
> John Hubbard
> NVIDIA