On Wed, Jan 26, 2022 at 8:58 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 26.01.22 17:53, Yang Shi wrote: > > On Wed, Jan 26, 2022 at 3:57 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > >> > >> On 26.01.22 12:48, Jann Horn wrote: > >>> On Wed, Jan 26, 2022 at 12:38 PM David Hildenbrand <david@xxxxxxxxxx> wrote: > >>>> On 26.01.22 12:29, Jann Horn wrote: > >>>>> On Wed, Jan 26, 2022 at 11:51 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > >>>>>> On 20.01.22 21:28, Yang Shi wrote: > >>>>>>> The syzbot reported the below BUG: > >>>>>>> > >>>>>>> kernel BUG at include/linux/page-flags.h:785! > >>> [...] > >>>>>>> RIP: 0010:PageDoubleMap include/linux/page-flags.h:785 [inline] > >>>>>>> RIP: 0010:__page_mapcount+0x2d2/0x350 mm/util.c:744 > >>> [...] > >>>>>> Does this point at the bigger issue that reading the mapcount without > >>>>>> having the page locked is completely unstable? > >>>>> > >>>>> (See also https://lore.kernel.org/all/CAG48ez0M=iwJu=Q8yUQHD-+eZDg6ZF8QCF86Sb=CN1petP=Y0Q@xxxxxxxxxxxxxx/ > >>>>> for context.) > >>>> > >>>> Thanks for the pointer. > >>>> > >>>>> > >>>>> I'm not sure what you mean by "unstable". Do you mean "the result is > >>>>> not guaranteed to still be valid when the call returns", "the result > >>>>> might not have ever been valid", or "the call might crash because the > >>>>> page's state as a compound page is unstable"? > >>>> > >>>> A little bit of everything :) > >>> [...] > >>>>> In case you mean "the result might not have ever been valid": > >>>>> Yes, even with this patch applied, in theory concurrent THP splits > >>>>> could cause us to count some page mappings twice. Arguably that's not > >>>>> entirely correct. > >>>> > >>>> Yes, the snapshot is not atomic and, thereby, unreliable. That what I > >>>> mostly meant as "unstable". > >>>> > >>>>> > >>>>> In case you mean "the call might crash because the page's state as a > >>>>> compound page could concurrently change": > >>>> > >>>> I think that's just a side-product of the snapshot not being "correct", > >>>> right? > >>> > >>> I guess you could see it that way? The way I look at it is that > >>> page_mapcount() is designed to return a number that's at least as high > >>> as the number of mappings (rarely higher due to races), and using > >>> page_mapcount() on an unlocked page is legitimate if you're fine with > >>> the rare double-counting of references. In my view, the problem here > >>> is: > >>> > >>> There are different types of references to "struct page" - some of > >>> them allow you to call page_mapcount(), some don't. And in particular, > >>> get_page() doesn't give you a reference that can be used with > >>> page_mapcount(), but locking a (real, non-migration) PTE pointing to > >>> the page does give you such a reference. > >> > >> I assume the point is that as long as the page cannot be unmapped > >> because you block it from getting unmapped (PT lock), the compound page > >> cannot get split. As long as the page cannot get unmapped from that page > >> table you should have at least a mapcount of 1. > > > > If you mean holding ptl could prevent THP from splitting, then it is > > not true since you may be in the middle of THP split just exactly like > > the race condition solved by this patch. > > While you hold the PT lock and discover a mapped page, unmap_page() > cannot continue and unmap the page. That's what I meant "as long as the > page cannot be unmapped". > > What doesn't work is if you hold the PT lock and discover a migration > entry, because then you're already past unmap_page(). That's the issue > you're fixing. Yeah, it means you lose the race :-( > > > > > Just page lock or elevated page refcount could serialize against THP > > split AFAIK. > > > >> > >> But yeah, using the mapcount of a page that is not even mapped > >> (migration entry) is clearly wrong. > >> > >> To summarize: reading the mapcount on an unlocked page will easily > >> return a wrong result and the result should not be relied upon. reading > >> the mapcount of a migration entry is dangerous and certainly wrong. > > > > Depends on your usecase. Some just want to get a snapshot, just like > > smaps, they don't care. > > Right, but as discussed, even the snapshot might be slightly wrong. That > might be just fine for smaps (and I would have enjoyed a comment in the > code stating that :) ). I think that is documented already, see Documentation/filesystems/proc.rst: Note: reading /proc/PID/maps or /proc/PID/smaps is inherently racy (consistent output can be achieved only in the single read call). Of course, if the extra note is preferred in the code, I could try to add some in a separate patch. > > > -- > Thanks, > > David / dhildenb >
