Re: [PATCH v1] mm: zswap: Fix a potential memory leak in zswap_decompress().

[Yosry Ahmed <yosryahmed@google.com>]

On Tue, Nov 12, 2024 at 9:59 PM Sridhar, Kanchana P
<kanchana.p.sridhar@intel.com> wrote:
>
>
> > -----Original Message-----
> > From: Yosry Ahmed <yosryahmed@google.com>
> > Sent: Tuesday, November 12, 2024 9:35 PM
> > To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> > hannes@cmpxchg.org; nphamcs@gmail.com; chengming.zhou@linux.dev;
> > usamaarif642@gmail.com; ryan.roberts@arm.com; Huang, Ying
> > <ying.huang@intel.com>; 21cnbao@gmail.com; akpm@linux-foundation.org;
> > Feghali, Wajdi K <wajdi.k.feghali@intel.com>; Gopal, Vinodh
> > <vinodh.gopal@intel.com>
> > Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in
> > zswap_decompress().
> >
> > On Tue, Nov 12, 2024 at 9:24 PM Kanchana P Sridhar
> > <kanchana.p.sridhar@intel.com> wrote:
> > >
> > > This is a hotfix for a potential zpool memory leak that could result in
> > > the existing zswap_decompress():
> > >
> > >         mutex_unlock(&acomp_ctx->mutex);
> > >
> > >         if (src != acomp_ctx->buffer)
> > >                 zpool_unmap_handle(zpool, entry->handle);
> > >
> > > Releasing the lock before the conditional does not protect the integrity of
> > > "src", which is set earlier under the acomp_ctx mutex lock. This poses a
> > > risk for the conditional behaving as intended, and consequently not
> > > unmapping the zpool handle, which could cause a zswap zpool memory
> > leak.
> > >
> > > This patch moves the mutex_unlock() to occur after the conditional and
> > > subsequent zpool_unmap_handle(). This ensures that the value of "src"
> > > obtained earlier, with the mutex locked, does not change.
> >
> > The commit log is too complicated and incorrect. It is talking about
> > the stability of 'src', but that's a local variable on the stack
> > anyway. It doesn't need protection.
> >
> > The problem is 'acomp_ctx->buffer' being reused and changed after the
> > mutex is released. Leading to the check not being reliable. Please
> > simplify this.
>
> Thanks Yosry. That's exactly what I meant, but I think the wording got
> confusing. The problem I was trying to fix is the acomp_ctx->buffer
> value changing after the lock is released. This could happen as a result of any
> other compress or decompress that acquires the lock. I will simplify and
> clarify accordingly.
>
> >
> > >
> > > Even though an actual memory leak was not observed, this fix seems like a
> > > cleaner implementation.
> > >
> > > Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@intel.com>
> > > Fixes: 9c500835f279 ("mm: zswap: fix kernel BUG in sg_init_one")
> > > ---
> > >  mm/zswap.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index f6316b66fb23..58810fa8ff23 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -986,10 +986,11 @@ static void zswap_decompress(struct
> > zswap_entry *entry, struct folio *folio)
> > >         acomp_request_set_params(acomp_ctx->req, &input, &output, entry-
> > >length, PAGE_SIZE);
> > >         BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx-
> > >req), &acomp_ctx->wait));
> > >         BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE);
> > > -       mutex_unlock(&acomp_ctx->mutex);
> > >
> > >         if (src != acomp_ctx->buffer)
> > >                 zpool_unmap_handle(zpool, entry->handle);
> >
> > Actually now that I think more about it, I think this check isn't
> > entirely safe, even under the lock. Is it possible that
> > 'acomp_ctx->buffer' just happens to be equal to 'src' from a previous
> > decompression at the same handle? In this case, we will also
> > mistakenly skip the unmap.
>
> If we move the mutex_unlock to happen after the conditional and unmap,
> shouldn't that be sufficient under all conditions? With the fix, "src" can
> take on only 2 values in this procedure: the mapped handle or
> acomp_ctx->buffer. The only ambiguity would be in the (unlikely?) case
> if the mapped zpool handle happens to be equal to acomp_ctx->buffer.

Yes, that's the scenario I mean.

Specifically, in zswap_decompress(), we do not use 'acomp_ctx->buffer'
so 'src' is equal to the mapped handle. But, 'acomp_ctx->buffer'
happens to be equal to the same handle from a previous operation as we
don't clear it.

In this case, the 'src != acomp_ctx->buffer' check will be false, even
though it should be true. This will result in an extra
zpool_unmap_handle() call. I didn't look closely, but this seems like
it can have a worse effect than leaking memory (e.g. there will be an
extra __kunmap_atomic() call in zsmalloc, and we may end up corrupting
a random handle).

>
> Please let me know if I am missing anything.
>
> >
> > It would be more reliable to set a boolean variable if we copy to
> > acomp_ctx->buffer and do the unmap, and check that flag here to check
> > if the unmap was done or not.
>
> Sure, this could be done, but not sure if it is required. Please let me know
> if we still need the boolean variable in addition to moving the mutex_unlock().

If we use a boolean, there is no need to move mutex_unlock(). The
boolean will be a local variable on the stack that doesn't need
protection.

>
> Thanks,
> Kanchana
>
> >
> > > +
> > > +       mutex_unlock(&acomp_ctx->mutex);
> > >  }
> > >
> > >  /*********************************
> > >
> > > base-commit: 0e5bdedb39ded767bff4c6184225578595cee98c
> > > --
> > > 2.27.0
> > >