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);
