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

["Sridhar, Kanchana P" <kanchana.p.sridhar@intel.com>]

> -----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.

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().

Thanks,
Kanchana

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