linux-kernel - Re: [PATCH] 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: Windows password security audit tool. GUI, reports in PDF.

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

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

On Tue, May 03, 2022 at 11:08:25AM -0700, Minchan Kim wrote:
< snip >

Ccing Paul really this time.

Attach original code for Paul.

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..f59bbe3296e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
 #ifdef CONFIG_MIGRATION
 static inline bool is_pinnable_page(struct page *page)
 {
-	return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
-		is_zero_pfn(page_to_pfn(page));
+	int mt = get_pageblock_migratetype(page);
+
+	return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
+		mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
 }
 #else
 static inline bool is_pinnable_page(struct page *page)
--
2.36.0.464.gb9c8b46e94-goog


> > >>>
> > >>> A thing to get some attention is whether we need READ_ONCE or not
> > >>> for the local variable mt.
> > >>>
> > >>
> > >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> > >> there is anything stopping the compiler from re-reading the value. But
> > >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> > >> something in between.
> > > 
> > > How about this?
> > > 
> > >      CPU A                                                      CPU B
> > > 
> > > is_pinnable_page
> > >   ..
> > >   ..                                                set_pageblock_migratetype(MIGRATE_ISOLATE)
> > >   mt == MIGRATE_CMA
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_ISOLATE
> > >   mt == MIGRATE_ISOLATE                             set_pageblock_migratetype(MIGRATE_CMA)
> > >     get_pageblock_miratetype(page)
> > >         returns MIGRATE_CMA
> > >  
> > > So both conditions fails to detect it.
> > 
> > I think you're right. That's nasty.
> 
> Ccing Paul to borrow expertise. :)
> 
> int mt = get_pageblock_migratetype(page);
> 
> if (mt == MIGRATE_CMA)
>   return true;
> 
> if (mt == MIGRATE_ISOLATE)
>   return true;
> 
> I'd like to keep use the local variable mt's value in folloing
> conditions checks instead of refetching the value from
> get_pageblock_migratetype.
> 
> What's the right way to achieve it?
> 
> Thanks in advance!

Paul, could you give any hint?