Re: [PATCH STABLE 4.9 1/1] mm, gup: add missing refcount overflow checks on x86 and s390

[Vlastimil Babka <vbabka@suse.cz>]

On 12/3/19 1:22 PM, Ben Hutchings wrote:
>> +		    || !page_cache_get_speculative(head)))
>>  			return 0;
>>  		if (unlikely(pte_val(pte) != pte_val(*ptep))) {
>>  			put_page(head);
> [...]
>> --- a/arch/x86/mm/gup.c
>> +++ b/arch/x86/mm/gup.c
>> @@ -202,10 +202,12 @@ static int __gup_device_huge_pmd(pmd_t pmd, unsigned long addr,
>>  			undo_dev_pagemap(nr, nr_start, pages);
>>  			return 0;
>>  		}
>> +		if (unlikely(!try_get_page(page))) {
>> +			put_dev_pagemap(pgmap);
>> +			return 0;
>> +		}
>>  		SetPageReferenced(page);
>>  		pages[*nr] = page;
>> -		get_page(page);
>> -		put_dev_pagemap(pgmap);
> 
> This leaks a pgmap reference on success!

Good catch, deleted one line too many!

>>  		(*nr)++;
>>  		pfn++;
>>  	} while (addr += PAGE_SIZE, addr != end);
>> @@ -230,6 +232,8 @@ static noinline int gup_huge_pmd(pmd_t pmd, unsigned long addr,
>>  
>>  	refs = 0;
>>  	head = pmd_page(pmd);
>> +	if (WARN_ON_ONCE(page_ref_count(head) <= 0))
> 
> Why <= 0, given we use < 0 elsewhere?

The code uses get_head_page_multiple() which boils down to atomic_add
and not add_unless_zero(), so it assumes a pre-existing pin that must
not go away or it's a bug (one that I've been hunting recently in this
area). The check makes it explicit.

> 
>> +		return 0;
>>  	page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
>>  	do {
>>  		VM_BUG_ON_PAGE(compound_head(page) != head, page);
>> @@ -289,6 +293,8 @@ static noinline int gup_huge_pud(pud_t pud, unsigned long addr,
>>  
>>  	refs = 0;
>>  	head = pud_page(pud);
>> +	if (WARN_ON_ONCE(page_ref_count(head) <= 0))
> 
> Same question here.

Same as above.

> Ben.
> 
>> +		return 0;
>>  	page = head + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
>>  	do {
>>  		VM_BUG_ON_PAGE(compound_head(page) != head, page);