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

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

Yu Zhao <yuzhao@google.com> writes:

> On Wed, Jan 05, 2022 at 08:34:40PM -0300, Mauricio Faria de Oliveira wrote:
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 163ac4e6bcee..8671de473c25 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -1570,7 +1570,20 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>  
>>  			/* MADV_FREE page check */
>>  			if (!PageSwapBacked(page)) {
>> -				if (!PageDirty(page)) {
>> +				int ref_count = page_ref_count(page);
>> +				int map_count = page_mapcount(page);
>> +
>> +				/*
>> +				 * The only page refs must be from the isolation
>> +				 * (checked by the caller shrink_page_list() too)
>> +				 * and one or more rmap's (dropped by discard:).
>> +				 *
>> +				 * Check the reference count before dirty flag
>> +				 * with memory barrier; see __remove_mapping().
>> +				 */
>> +				smp_rmb();
>> +				if ((ref_count - 1 == map_count) &&
>> +				    !PageDirty(page)) {
>>  					/* Invalidate as we cleared the pte */
>>  					mmu_notifier_invalidate_range(mm,
>>  						address, address + PAGE_SIZE);
>
> Out of curiosity, how does it work with COW in terms of reordering?
> Specifically, it seems to me get_page() and page_dup_rmap() in
> copy_present_pte() can happen in any order, and if page_dup_rmap()
> is seen first, and direct io is holding a refcnt, this check can still
> pass?

I think that you are correct.

After more thoughts, it appears very tricky to compare page count and
map count.  Even if we have added smp_rmb() between page_ref_count() and
page_mapcount(), an interrupt may happen between them.  During the
interrupt, the page count and map count may be changed, for example,
unmapped, or do_swap_page().

Best Regards,
Huang, Ying