linux-kernel - Re: [PATCH v2 1/4] mm: migrate: Move the page count validation to the proper place

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]

Message-ID: <6f1b616a-d67c-7580-a3b2-fcb621917063@linux.alibaba.com>
Date:   Mon, 16 Aug 2021 18:58:12 +0800
From:   Baolin Wang <baolin.wang@...ux.alibaba.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     akpm@...ux-foundation.org, apopple@...dia.com, shy828301@...il.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] mm: migrate: Move the page count validation to the
 proper place



On 2021/8/15 18:34, Matthew Wilcox wrote:
> On Sun, Aug 15, 2021 at 02:23:03PM +0800, Baolin Wang wrote:
>> We've got the expected count for anonymous page or file page by
>> expected_page_refs() at the beginning of migrate_page_move_mapping(),
>> thus we should move the page count validation a little forward to
>> reduce duplicated code.
>>
>> Moreover the i_pages lock is not used to guarantee the page refcount
>> safety in migrate_page_move_mapping(), so we can move the getting page
>> count out of the i_pages lock. Since now the migration page has
>> established a migration pte under the page lock now, with the page
>> refcount freezing validation, to ensure that the page references
>> meet the migration requirement.
> 
> I remain unconvinced by this.
> 
> Looking at folio_migrate_mapping() a little more deeply, I don't
> understand why we first check folio_ref_count() and then attempt
> to free the refcount.  Why not just try to freeze it directly?
> 
> ie instead of your patch, this:
> 
> +++ b/mm/migrate.c
> @@ -403,13 +403,8 @@ int folio_migrate_mapping(struct address_space *mapping,
>          newzone = folio_zone(newfolio);
> 
>          xas_lock_irq(&xas);
> -       if (folio_ref_count(folio) != expected_count ||
> -           xas_load(&xas) != folio) {
> -               xas_unlock_irq(&xas);
> -               return -EAGAIN;
> -       }
> -
> -       if (!folio_ref_freeze(folio, expected_count)) {
> +       if (xas_load(&xas) != folio ||
> +           !folio_ref_freeze(folio, expected_count)) {
>                  xas_unlock_irq(&xas);
>                  return -EAGAIN;
>          }

I think this is reasonable, like what we've done in __remove_mapping().

> And since we've got the lock on the page, how can somebody else be
> removing it from the page cache?  I think that xas_load() can be
> removed too.  So even more simply,

Good point, this is more simply.

> 
> +++ b/mm/migrate.c
> @@ -403,12 +403,6 @@ int folio_migrate_mapping(struct address_space *mapping,
>          newzone = folio_zone(newfolio);
> 
>          xas_lock_irq(&xas);
> -       if (folio_ref_count(folio) != expected_count ||
> -           xas_load(&xas) != folio) {
> -               xas_unlock_irq(&xas);
> -               return -EAGAIN;
> -       }
> -
>          if (!folio_ref_freeze(folio, expected_count)) {
>                  xas_unlock_irq(&xas);
>                  return -EAGAIN;
> 
> but I'm not really set up to test page migration.  Does your test suite
> test migrating file-backed pages?

Yes, I've tested above changes, and the mapped file pages migration 
works well. So can I resend this patch set with your Suggested-by tag? 
Thanks.