Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 May 2022 14:45:30 -0700
From:   Sultan Alsawaf <sultan@kerneltoast.com>
To:     Minchan Kim <minchan@kernel.org>
Cc:     stable@vger.kernel.org, Nitin Gupta <ngupta@vflare.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] zsmalloc: Fix races between asynchronous zspage free and
 page migration
Message-ID: <Ynwuepzrr3krjLG0@sultan-box.localdomain>
References: <20220509024703.243847-1-sultan@kerneltoast.com>
 <Ynv53fkx8cG0ixaE@google.com>
 <YnwTfBLn+6vYSe59@sultan-box.localdomain>
 <Ynwlh3RT0PAYpWpD@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ynwlh3RT0PAYpWpD@google.com>
Precedence: bulk

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