Re: [PATCH v4 08/14] mm/gup: grab head page refcount once for group of subpages

On Thu, Sep 30, 2021 at 01:01:14PM +1000, Alistair Popple wrote:

On Thursday, 30 September 2021 5:34:05 AM AEST Jason Gunthorpe wrote:

quoted

On Wed, Sep 29, 2021 at 12:50:15PM +0100, Joao Martins wrote:

quoted

quoted

If the get_dev_pagemap has to remain then it just means we have to
flush before changing pagemap pointers

Right -- I don't think we should need it as that discussion on the other
thread goes.

OTOH, using @pgmap might be useful to unblock gup-fast FOLL_LONGTERM
for certain devmap types[0] (like MEMORY_DEVICE_GENERIC [device-dax]
can support it but not MEMORY_DEVICE_FSDAX [fsdax]).

When looking at Logan's patches I think it is pretty clear to me that
page->pgmap must never be a dangling pointer if the caller has a
legitimate refcount on the page.

For instance the migrate and stuff all blindly calls
is_device_private_page() on the struct page expecting a valid
page->pgmap.

This also looks like it is happening, ie

void __put_page(struct page *page)
{
	if (is_zone_device_page(page)) {
		put_dev_pagemap(page->pgmap);

Is indeed putting the pgmap ref back when the page becomes ungettable.

This properly happens when the page refcount goes to zero and so it
should fully interlock with __page_cache_add_speculative():

	if (unlikely(!page_ref_add_unless(page, count, 0))) {

Thus, in gup.c, if we succeed at try_grab_compound_head() then
page->pgmap is a stable pointer with a valid refcount.

So, all the external pgmap stuff in gup.c is completely pointless.
try_grab_compound_head() provides us with an equivalent protection at
lower cost. Remember gup.c doesn't deref the pgmap at all.

Dan/Alistair/Felix do you see any hole in that argument??

As background note that device pages are currently considered free when
refcount == 1 but the pgmap reference is dropped when the refcount transitions
1->0. The final pgmap reference is typically dropped when a driver calls
memunmap_pages() and put_page() drops the last page reference:

void memunmap_pages(struct dev_pagemap *pgmap)
{
        unsigned long pfn;
        int i;

        dev_pagemap_kill(pgmap);
        for (i = 0; i < pgmap->nr_range; i++)
                for_each_device_pfn(pfn, pgmap, i)
                        put_page(pfn_to_page(pfn));
        dev_pagemap_cleanup(pgmap);

If there are still pgmap references dev_pagemap_cleanup(pgmap) will block until
the final reference is dropped. So I think your argument holds at least for
DEVICE_PRIVATE and DEVICE_GENERIC. DEVICE_FS_DAX defines it's own pagemap
cleanup but I can't see why the same argument wouldn't hold there - if a page
has a valid refcount it must have a reference on the pagemap too.

To close this circle - the issue is use after free on the struct page
* entry while it has a zero ref.

memunmap_pages() does wait for the refcount to go to zero, but it then
goes on to free the memory under the struct pages.

However there are possibly still untracked references to this memory
in the page tables.

This is the bug Dan has been working on - to shootdown page table
mappings before getting to memunmap_pages()

Getting the page map ref will make the use-after-free never crash,
just be a silent security problem. :(

Jason