linux-kernel - Re: [PATCH] mm: gup: fix infinite loop within __get_longterm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e307690d-8d0e-4dbf-b385-fac4eeaecf2f@redhat.com>
Date: Tue, 21 Jan 2025 08:49:04 +0100
From: David Hildenbrand <david@...hat.com>
To: Zhaoyang Huang <huangzhaoyang@...il.com>
Cc: "zhaoyang.huang" <zhaoyang.huang@...soc.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Alistair Popple <apopple@...dia.com>, John Hubbard <jhubbard@...dia.com>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org, steve.kang@...soc.com
Subject: Re: [PATCH] mm: gup: fix infinite loop within __get_longterm_locked

On 21.01.25 02:31, Zhaoyang Huang wrote:
> On Tue, Jan 21, 2025 at 4:14 AM David Hildenbrand <david@...hat.com> wrote:
>>
>> On 20.01.25 10:26, zhaoyang.huang wrote:
>>> From: Zhaoyang Huang <zhaoyang.huang@...soc.com>
>>>
>>> Infinite loop within __get_longterm_locked detected in an unique usage
>>> of pin_user_pages where the VA's pages are all unpinnable(vm_ops->fault
>>> function allocate pages via cma_alloc for hardware purpose and leave them
>>> out of LRU) Fixing this by have 'collected' reflect the actual number> of pages in movable_folio_list.
>>
>> Maybe something like:
>>
>> "
>> We can run into an infinite loop in __get_longterm_locked() when
>> collect_longterm_unpinnable_folios() finds only folios that are isolated
>> from the LRU or were never added to the LRU. This can happen when all
>> folios to be pinned are never added to the LRU, for example when
>> vm_ops->fault allocated pages using cma_alloc() and never added them to
>> the LRU.
>>
>> We incorrectly update the "collected" variable even if nothing was
>> collected. Fix it by incrementing "collected" only when we isolated a
>> folio and added it to the list of folios to migrate.
>> "
>>
>> I assume, long-term these things will not actually be folios, but pages,
>> and we'll have to skip them in different code -- or assume they can be
>> longterm pinned even on CMA because they are allocated by the CMA-owning
>> driver.
> Thanks for the commit message. will update them to v2
>>
>>>
>>> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@...soc.com>
>>> ---
>>>    mm/gup.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/gup.c b/mm/gup.c
>>> index 3b75e631f369..2231ce7221f9 100644
>>> --- a/mm/gup.c
>>> +++ b/mm/gup.c
>>> @@ -2341,8 +2341,6 @@ static unsigned long collect_longterm_unpinnable_folios(
>>>                if (folio_is_longterm_pinnable(folio))
>>>                        continue;
>>>
>>> -             collected++;
>>> -
>>>                if (folio_is_device_coherent(folio))
>>>                        continue;
>>>
>>> @@ -2359,6 +2357,8 @@ static unsigned long collect_longterm_unpinnable_folios(
>>>                if (!folio_isolate_lru(folio))
>>>                        continue;
>>>
>>> +             collected++;
>>> +
>>>                list_add_tail(&folio->lru, movable_folio_list);
>>>                node_stat_mod_folio(folio,
>>>                                    NR_ISOLATED_ANON + folio_is_file_lru(folio),
>>
>> What if folio_isolate_hugetlb() succeeded? The return value can tell us
>> if it actually succeeded.
> How about remove the variable 'collected' and change the criteria to
> if(list_empty(&movable_folio_list))

That works if we know that the input list is empty, which is the case.

So let's turn that function into a void function, ans simply check 
list_empty() in the caller that prepares the empty list.

-- 
Cheers,

David / dhildenb