linux-kernel - Re: [PATCH] mm/gup: stop leaking pinned pages in low memory conditions

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: <91dbe5c9-68c3-4e92-a9c3-a3dad77d6dc2@redhat.com>
Date: Fri, 18 Oct 2024 00:03:03 +0200
From: David Hildenbrand <david@...hat.com>
To: John Hubbard <jhubbard@...dia.com>, Alistair Popple <apopple@...dia.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org,
 Shigeru Yoshida <syoshida@...hat.com>, Jason Gunthorpe <jgg@...dia.com>,
 Minchan Kim <minchan@...nel.org>, Pasha Tatashin <pasha.tatashin@...een.com>
Subject: Re: [PATCH] mm/gup: stop leaking pinned pages in low memory
 conditions

On 17.10.24 23:57, John Hubbard wrote:
> On 10/17/24 2:47 PM, David Hildenbrand wrote:
>> On 17.10.24 23:28, Alistair Popple wrote:
>>> David Hildenbrand <david@...hat.com> writes:
>>>> On 16.10.24 22:22, John Hubbard wrote:
> ...
>>>>> +        if (rc != -EAGAIN && rc != 0)
>>>>> +            unpin_user_pages(pages, nr_pinned_pages);
>>>>> +
>>>>>         } while (rc == -EAGAIN);
>>>>
>>>> Wouldn't it be cleaner to simply have here after the loop (possibly
>>>> even after the memalloc_pin_restore())
>>>>
>>>> if (rc)
>>>>      unpin_user_pages(pages, nr_pinned_pages);
>>>>
>>>> But maybe I am missing something.
>>>
>>> I initially thought the same thing but I'm not sure it is
>>> correct. Consider what happens when __get_user_pages_locked() fails
>>> earlier in the loop. In this case it will have bailed out of the loop
>>> with rc <= 0 but we shouldn't call unpin_user_pages().
> 
> doh. yes. Thanks for catching that, Alistair! I actually considered
> it during the first draft, too, but got tunnel vision during the
> review, sigh.
> 
>>
>> Ah, I see what you mean, I primarily only stared at the diff.
>>
>> We should likely avoid using nr_pinned_pages as a temporary variable that
>> can hold an error value.
>>
> 
> OK, I still want to defer all the pretty refactoring ideas into some
> future effort, so for now, let's just leave v1 alone except for fixing
> the typo in the comment, yes?

Fine with me!

Acked-by: David Hildenbrand <david@...hat.com>

-- 
Cheers,

David / dhildenb