Re: [PATCH] mm: fix race between MADV_FREE reclaim and blkdev direct IO read

["Huang, Ying" <ying.huang@intel.com>]

Yang Shi <shy828301@gmail.com> writes:

> On Tue, Jan 4, 2022 at 4:32 PM Huang, Ying <ying.huang@intel.com> wrote:
>>
>> Yang Shi <shy828301@gmail.com> writes:
>>
>> > On Fri, Dec 10, 2021 at 6:22 PM Mauricio Faria de Oliveira
>> > <mfo@canonical.com> wrote:
>> >>
>> >> Problem:
>> >> =======
>> >>
>> >> Userspace might read the zero-page instead of actual data from a
>> >> direct IO read on a block device if the buffers have been called
>> >> madvise(MADV_FREE) on earlier (this is discussed below) due to a
>> >> race between page reclaim on MADV_FREE and blkdev direct IO read.
>> >>
>> >> Race condition:
>> >> ==============
>> >>
>> >> During page reclaim, the MADV_FREE page check in try_to_unmap_one()
>> >> checks if the page is not dirty, then discards its PTE (vs remap it
>> >> back if the page is dirty).
>> >>
>> >> However, after try_to_unmap_one() returns to shrink_page_list(), it
>> >> might keep the page _anyway_ if page_ref_freeze() fails (it expects
>> >> a single page ref from the isolation).
>> >>
>> >> Well, blkdev_direct_IO() gets references for all pages, and on READ
>> >> operations it sets them dirty later.
>> >>
>> >> So, if MADV_FREE pages (i.e., not dirty) are used as buffers (more
>> >> later) for direct IO read from block devices and page reclaim runs
>> >> during __blkdev_direct_IO[_simple]() AFTER bio_iov_iter_get_pages()
>> >> but BEFORE it sets pages dirty, that situation happens.
>> >>
>> >> The direct IO read eventually completes. Now, when userspace reads
>> >> the buffers, the PTE is no longer there and the page fault handler
>> >> do_anonymous_page() services that with the zero-page, NOT the data!
>> >>
>> >> A synthetic reproducer is provided.
>> >>
>> >> Page faults:
>> >> ===========
>> >>
>> >> The data read from the block device probably won't generate faults
>> >> due to DMA (no MMU) but even in the case it wouldn't use DMA, that
>> >> happens on different virtual addresses (not user-mapped addresses)
>> >> because `struct bio_vec` stores `struct page` to figure addresses
>> >> out (which are different from/unrelated to user-mapped addresses)
>> >> for the data read.
>> >>
>> >> Thus userspace reads (to user-mapped addresses) still fault, then
>> >> do_anonymous_page() gets another `struct page` that would address/
>> >> map to other memory than the `struct page` used by `struct bio_vec`
>> >> for the read (which runs correctly as the page wasn't freed due to
>> >> page_ref_freeze(), and is reclaimed later) -- but even if the page
>> >> addresses matched, that handler maps the zero-page in the PTE, not
>> >> that page's memory (on read faults.)
>> >>
>> >> If page reclaim happens BEFORE bio_iov_iter_get_pages() the issue
>> >> doesn't happen, because that faults-in all pages as writeable, so
>> >> do_anonymous_page() sets up a new page/rmap/PTE, and that is used
>> >> by direct IO. The userspace reads don't fault as the PTE is there
>> >> (thus zero-page is not used.)
>> >>
>> >> Solution:
>> >> ========
>> >>
>> >> One solution is to check for the expected page reference count in
>> >> try_to_unmap_one() too, which should be exactly two: one from the
>> >> isolation (checked by shrink_page_list()), and the other from the
>> >> rmap (dropped by the discard: label). If that doesn't match, then
>> >> remap the PTE back, just like page dirty does.
>> >>
>> >> The new check in try_to_unmap_one() should be safe in races with
>> >> bio_iov_iter_get_pages() in get_user_pages() fast and slow paths,
>> >> as it's done under the PTE lock. The fast path doesn't take that
>> >> lock but it checks the PTE has changed, then drops the reference
>> >> and leaves the page for the slow path (which does take that lock).
>> >>
>> >> - try_to_unmap_one()
>> >>   - page_vma_mapped_walk()
>> >>     - map_pte() # see pte_offset_map_lock():
>> >>         pte_offset_map()
>> >>         spin_lock()
>> >>   - page_ref_count() # new check
>> >>   - page_vma_mapped_walk_done() # see pte_unmap_unlock():
>> >>       pte_unmap()
>> >>       spin_unlock()
>> >>
>> >> - bio_iov_iter_get_pages()
>> >>   - __bio_iov_iter_get_pages()
>> >>     - iov_iter_get_pages()
>> >>       - get_user_pages_fast()
>> >>         - internal_get_user_pages_fast()
>> >>
>> >>           # fast path
>> >>           - lockless_pages_from_mm()
>> >>             - gup_{pgd,p4d,pud,pmd,pte}_range()
>> >>                 ptep = pte_offset_map() # not _lock()
>> >>                 pte = ptep_get_lockless(ptep)
>> >>                 page = pte_page(pte)
>> >>                 try_grab_compound_head(page) # get ref
>> >>                 if (pte_val(pte) != pte_val(*ptep))
>> >>                         put_compound_head(page) # put ref
>> >>                         # leave page for slow path
>> >>           # slow path
>> >>           - __gup_longterm_unlocked()
>> >>             - get_user_pages_unlocked()
>> >>               - __get_user_pages_locked()
>> >>                 - __get_user_pages()
>> >>                   - follow_{page,p4d,pud,pmd}_mask()
>> >>                     - follow_page_pte()
>> >>                         ptep = pte_offset_map_lock()
>> >>                         pte = *ptep
>> >>                         page = vm_normal_page(pte)
>> >>                         try_grab_page(page) # get ref
>> >>                         pte_unmap_unlock()
>> >>
>> >> Regarding transparent hugepages, that number shouldn't change, as
>> >> MADV_FREE (aka lazyfree) pages are PageAnon() && !PageSwapBacked()
>> >> (madvise_free_pte_range() -> mark_page_lazyfree() -> lru_lazyfree_fn())
>> >> thus should reach shrink_page_list() -> split_huge_page_to_list()
>> >> before try_to_unmap[_one](), so it deals with normal pages only.
>> >>
>> >> (And in case unlikely/TTU_SPLIT_HUGE_PMD/split_huge_pmd_address()
>> >> happens, which it should not or be rare, the page refcount is not
>> >> two, as the head page counts tail pages, and tail pages have zero.
>> >> That also prevents checking the head `page` then incorrectly call
>> >> page_remove_rmap(subpage) for a tail page, that isn't even in the
>> >> shrink_page_list()'s page_list (an effect of split huge pmd/pmvw),
>> >> as it might happen today in this unlikely scenario.)
>> >>
>> >> MADV_FREE'd buffers:
>> >> ===================
>> >>
>> >> So, back to the "if MADV_FREE pages are used as buffers" note.
>> >> The case is arguable, and subject to multiple interpretations.
>> >>
>> >> The madvise(2) manual page on the MADV_FREE advice value says:
>> >> - 'After a successful MADV_FREE ... data will be lost when
>> >>    the kernel frees the pages.'
>> >> - 'the free operation will be canceled if the caller writes
>> >>    into the page' / 'subsequent writes ... will succeed and
>> >>    then [the] kernel cannot free those dirtied pages'
>> >> - 'If there is no subsequent write, the kernel can free the
>> >>    pages at any time.'
>> >>
>> >> Thoughts, questions, considerations...
>> >> - Since the kernel didn't actually free the page (page_ref_freeze()
>> >>   failed), should the data not have been lost? (on userspace read.)
>> >> - Should writes performed by the direct IO read be able to cancel
>> >>   the free operation?
>> >>   - Should the direct IO read be considered as 'the caller' too,
>> >>     as it's been requested by 'the caller'?
>> >>   - Should the bio technique to dirty pages on return to userspace
>> >>     (bio_check_pages_dirty() is called/used by __blkdev_direct_IO())
>> >>     be considered in another/special way here?
>> >> - Should an upcoming write from a previously requested direct IO
>> >>   read be considered as a subsequent write, so the kernel should
>> >>   not free the pages? (as it's known at the time of page reclaim.)
>> >>
>> >> Technically, the last point would seem a reasonable consideration
>> >> and balance, as the madvise(2) manual page apparently (and fairly)
>> >> seem to assume that 'writes' are memory access from the userspace
>> >> process (not explicitly considering writes from the kernel or its
>> >> corner cases; again, fairly).. plus the kernel fix implementation
>> >> for the corner case of the largely 'non-atomic write' encompassed
>> >> by a direct IO read operation, is relatively simple; and it helps.
>> >>
>> >> Reproducer:
>> >> ==========
>> >>
>> >> @ test.c (simplified, but works)
>> >>
>> >>         #define _GNU_SOURCE
>> >>         #include <fcntl.h>
>> >>         #include <stdio.h>
>> >>         #include <unistd.h>
>> >>         #include <sys/mman.h>
>> >>
>> >>         int main() {
>> >>                 int fd, i;
>> >>                 char *buf;
>> >>
>> >>                 fd = open(DEV, O_RDONLY | O_DIRECT);
>> >>
>> >>                 buf = mmap(NULL, BUF_SIZE, PROT_READ | PROT_WRITE,
>> >>                            MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> >>
>> >>                 for (i = 0; i < BUF_SIZE; i += PAGE_SIZE)
>> >>                         buf[i] = 1; // init to non-zero
>> >>
>> >>                 madvise(buf, BUF_SIZE, MADV_FREE);
>> >
>> > IIUC, you are expecting to get the old data after MADV_FREE? TBH, you
>> > should not expect so at all after MADV_FREE since those pages may get
>> > freed at any time.
>> >
>>
>> Per my understanding, if direct IO reading is done after MADV_FREE, I
>> think we want to get the new data instead of old data.
>
> Here "old data" means the data written by "buf[i] = 1;" before
> MADV_FREE in that test code.

OK.  I found that the expected data isn't "1", but "0x79", which is read
from disk image after MADV_FREE.  Re-paste as below,

	# mv test good
	# ./good
	0x7f1509206000: 0x79
	0x7f1509207000: 0x79
	0x7f1509208000: 0x79
	0x7f1509209000: 0x79

Best Regards,
Huang, Ying