Re: Patch "mm: zswap: fix race between [de]compression and CPU hotunplug" has been added to the 6.12-stable tree

Re: Patch "mm: zswap: fix race between [de]compression and CPU hotunplug" has been added to the 6.12-stable tree

[Yosry Ahmed]

On Mon, Jan 13, 2025 at 6:04 AM Sasha Levin <sashal@kernel.org> wrote:
>
> This is a note to let you know that I've just added the patch titled
>
>     mm: zswap: fix race between [de]compression and CPU hotunplug
>
> to the 6.12-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
>      mm-zswap-fix-race-between-de-compression-and-cpu-hot.patch
> and it can be found in the queue-6.12 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.

Please drop this patch from v6.12 (and all stable trees) as it has a
bug, as I pointed out in [1] (was that the correct place to surface
this?).

Andrew's latest PR contains a revert of this patch (and alternative fix) [2].

Thanks!

[1]https://lore.kernel.org/stable/CAJD7tkaiUA6J1nb0=1ELpUD0OgjoD+tft-iPqbPdyXCpS=2AGQ@mail.gmail.com/
[2]https://lore.kernel.org/lkml/20250113000543.862792e948c2646032a477e0@linux-foundation.org/

>
>
>
> commit c56e79d453ef5b5fc6ded252bc6ba461f12946ba
> Author: Yosry Ahmed <yosryahmed@google.com>
> Date:   Thu Dec 19 21:24:37 2024 +0000
>
>     mm: zswap: fix race between [de]compression and CPU hotunplug
>
>     [ Upstream commit eaebeb93922ca6ab0dd92027b73d0112701706ef ]
>
>     In zswap_compress() and zswap_decompress(), the per-CPU acomp_ctx of the
>     current CPU at the beginning of the operation is retrieved and used
>     throughout.  However, since neither preemption nor migration are disabled,
>     it is possible that the operation continues on a different CPU.
>
>     If the original CPU is hotunplugged while the acomp_ctx is still in use,
>     we run into a UAF bug as the resources attached to the acomp_ctx are freed
>     during hotunplug in zswap_cpu_comp_dead().
>
>     The problem was introduced in commit 1ec3b5fe6eec ("mm/zswap: move to use
>     crypto_acomp API for hardware acceleration") when the switch to the
>     crypto_acomp API was made.  Prior to that, the per-CPU crypto_comp was
>     retrieved using get_cpu_ptr() which disables preemption and makes sure the
>     CPU cannot go away from under us.  Preemption cannot be disabled with the
>     crypto_acomp API as a sleepable context is needed.
>
>     Commit 8ba2f844f050 ("mm/zswap: change per-cpu mutex and buffer to
>     per-acomp_ctx") increased the UAF surface area by making the per-CPU
>     buffers dynamic, adding yet another resource that can be freed from under
>     zswap compression/decompression by CPU hotunplug.
>
>     There are a few ways to fix this:
>     (a) Add a refcount for acomp_ctx.
>     (b) Disable migration while using the per-CPU acomp_ctx.
>     (c) Disable CPU hotunplug while using the per-CPU acomp_ctx by holding
>     the CPUs read lock.
>
>     Implement (c) since it's simpler than (a), and (b) involves using
>     migrate_disable() which is apparently undesired (see huge comment in
>     include/linux/preempt.h).
>
>     Link: https://lkml.kernel.org/r/20241219212437.2714151-1-yosryahmed@google.com
>     Fixes: 1ec3b5fe6eec ("mm/zswap: move to use crypto_acomp API for hardware acceleration")
>     Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
>     Reported-by: Johannes Weiner <hannes@cmpxchg.org>
>     Closes: https://lore.kernel.org/lkml/20241113213007.GB1564047@cmpxchg.org/
>     Reported-by: Sam Sun <samsun1006219@gmail.com>
>     Closes: https://lore.kernel.org/lkml/CAEkJfYMtSdM5HceNsXUDf5haghD5+o2e7Qv4OcuruL4tPg6OaQ@mail.gmail.com/
>     Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>
>     Acked-by: Barry Song <baohua@kernel.org>
>     Reviewed-by: Nhat Pham <nphamcs@gmail.com>
>     Cc: Vitaly Wool <vitalywool@gmail.com>
>     Cc: <stable@vger.kernel.org>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Sasha Levin <sashal@kernel.org>
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 0030ce8fecfc..c86d4bcbb447 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -875,6 +875,18 @@ static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node)
>         return 0;
>  }
>
> +/* Prevent CPU hotplug from freeing up the per-CPU acomp_ctx resources */
> +static struct crypto_acomp_ctx *acomp_ctx_get_cpu(struct crypto_acomp_ctx __percpu *acomp_ctx)
> +{
> +       cpus_read_lock();
> +       return raw_cpu_ptr(acomp_ctx);
> +}
> +
> +static void acomp_ctx_put_cpu(void)
> +{
> +       cpus_read_unlock();
> +}
> +
>  static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
>  {
>         struct crypto_acomp_ctx *acomp_ctx;
> @@ -887,8 +899,7 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
>         gfp_t gfp;
>         u8 *dst;
>
> -       acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> -
> +       acomp_ctx = acomp_ctx_get_cpu(entry->pool->acomp_ctx);
>         mutex_lock(&acomp_ctx->mutex);
>
>         dst = acomp_ctx->buffer;
> @@ -944,6 +955,7 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
>                 zswap_reject_alloc_fail++;
>
>         mutex_unlock(&acomp_ctx->mutex);
> +       acomp_ctx_put_cpu();
>         return comp_ret == 0 && alloc_ret == 0;
>  }
>
> @@ -954,7 +966,7 @@ static void zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>         struct crypto_acomp_ctx *acomp_ctx;
>         u8 *src;
>
> -       acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> +       acomp_ctx = acomp_ctx_get_cpu(entry->pool->acomp_ctx);
>         mutex_lock(&acomp_ctx->mutex);
>
>         src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> @@ -984,6 +996,7 @@ static void zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>
>         if (src != acomp_ctx->buffer)
>                 zpool_unmap_handle(zpool, entry->handle);
> +       acomp_ctx_put_cpu();
>  }
>
>  /*********************************

Re: Patch "mm: zswap: fix race between [de]compression and CPU hotunplug" has been added to the 6.12-stable tree

[Sasha Levin]

On Mon, Jan 13, 2025 at 07:59:47AM -0800, Yosry Ahmed wrote:
>On Mon, Jan 13, 2025 at 6:04 AM Sasha Levin <sashal@kernel.org> wrote:
>>
>> This is a note to let you know that I've just added the patch titled
>>
>>     mm: zswap: fix race between [de]compression and CPU hotunplug
>>
>> to the 6.12-stable tree which can be found at:
>>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>>      mm-zswap-fix-race-between-de-compression-and-cpu-hot.patch
>> and it can be found in the queue-6.12 subdirectory.
>>
>> If you, or anyone else, feels it should not be added to the stable tree,
>> please let <stable@vger.kernel.org> know about it.
>
>Please drop this patch from v6.12 (and all stable trees) as it has a
>bug, as I pointed out in [1] (was that the correct place to surface
>this?).
>
>Andrew's latest PR contains a revert of this patch (and alternative fix) [2].
>
>Thanks!

Will do, thanks!

-- 
Thanks,
Sasha