linux-kernel - Re: [PATCH] mm/gup.c: Simplify and fix check_and_migrate_movable

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 02 Aug 2022 22:52:29 +1000
From:   Alistair Popple <apopple@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     akpm@...ux-foundation.org, minchan@...nel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, jhubbard@...dia.com,
        pasha.tatashin@...een.com
Subject: Re: [PATCH] mm/gup.c: Simplify and fix
 check_and_migrate_movable_pages() return codes


Jason Gunthorpe <jgg@...dia.com> writes:

> On Mon, Aug 01, 2022 at 12:18:53PM +1000, Alistair Popple wrote:
>
>> > AFAICT there is no reason to 'continue' in most of these paths since
>> > we intend to return to userspace with an error anyhow? Why try to
>> > isolate more pages?
>>
>> The main reason would be if callers want to retry the operation. AFAIK
>> isolate_folio_lru() can have transient failures, so if callers want to
>> retry it makes sense to isolate and migrate as many pages as possible
>> rather than one page at a time as subsequent retries may find different
>> pages that can't be isolated.
>
> Except we don't try to do the migrate, we just isolate and then
> unisolate and return failure.

Unless I'm missing something any pages successfully isolated are still
added to movable_page_list then migrated if we 'continue' (at least in
the original code and this patch version, but not v2). Obviously pages
that couldn't be isolated can't be migrated, but subsequent retries
should only need to deal with those pages as the rest should already be
in the correct zone.

>> Actually I should have called this out more clearly - the previous
>> behaviour on isolation failure was to retry indefinitely which is what
>> lead to looping in the kernel. This patch turns isolation failure into
>> an error and doesn't retry. I wonder though if we need to maintain a
>> retry count similar to what migrate_pages() does if there are unexpected
>> page refs?
>
> This makes more sense, exporting this mess to the caller and hoping
> they retry (they won't) doesn't make sense..

Ok, sounds reasonable. Will post a v3 that does this instead.

> Jason