linux-kernel - Re: [PATCH] zsmalloc: Fix races between asynchronous zspage free and page migration

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ynwuepzrr3krjLG0@sultan-box.localdomain>
Date:   Wed, 11 May 2022 14:45:30 -0700
From:   Sultan Alsawaf <sultan@...neltoast.com>
To:     Minchan Kim <minchan@...nel.org>
Cc:     stable@...r.kernel.org, Nitin Gupta <ngupta@...are.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] zsmalloc: Fix races between asynchronous zspage free and
 page migration

On Wed, May 11, 2022 at 02:07:19PM -0700, Minchan Kim wrote:
> Then, how about this?

Your proposal is completely wrong still. My original patch is fine; we can stick
with that.

> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 9152fbde33b5..2f205c18aee4 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1716,12 +1716,31 @@ static enum fullness_group putback_zspage(struct size_class *class,
>   * To prevent zspage destroy during migration, zspage freeing should
>   * hold locks of all pages in the zspage.
>   */
> -static void lock_zspage(struct zspage *zspage)
> +static void lock_zspage(struct zs_pool *pool, struct zspage *zspage)
>  {
> -       struct page *page = get_first_page(zspage);
> -
> +       struct page *page;
> +       int nr_locked;
> +       struct page *locked_pages[ZS_MAX_PAGES_PER_ZSPAGE];
> +       struct address_space *mapping;
> +retry:
> +       nr_locked = 0;
> +       memset(locked_pages, 0, sizeof(struct page) * ARRAY_SIZE(locked_pages));

This memset() zeroes out memory past the end of the array because it is an array
of pointers, not an array of page structs; the sizeof() is incorrect.

> +       page = get_first_page(zspage);

You can't use get_first_page() outside of the migrate lock.

>         do {
>                 lock_page(page);

You can't lock a page that you don't own.

> +               locked_pages[nr_locked++] = page;
> +               /*
> +                * subpage in the zspage could be migrated under us so
> +                * verify it. Once it happens, retry the lock sequence.
> +                */
> +               mapping = page_mapping(page)

This doesn't compile.

> +               if (mapping != pool->inode->i_mapping ||
> +                   page_private(page) != (unsigned long)zspage) {
> +                       do {
> +                               unlock_page(locked_pages[--nr_locked]);
> +                       } while (nr_locked > 0)

This doesn't compile.

> +                       goto retry;
> +               }

There's no need to unlock all of the pages that were locked so far because once
a page is locked, it cannot be migrated.

>         } while ((page = get_next_page(page)) != NULL);
>  }

You can't use get_next_page() outside of the migrate lock.

> 
> @@ -1987,7 +2006,7 @@ static void async_free_zspage(struct work_struct *work)
> 
>         list_for_each_entry_safe(zspage, tmp, &free_pages, list) {
>                 list_del(&zspage->list);
> -               lock_zspage(zspage);
> +               lock_zspage(pool, zspage);
> 
>                 get_zspage_mapping(zspage, &class_idx, &fullness);
>                 VM_BUG_ON(fullness != ZS_EMPTY);

Sultan