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: Friday, November 15, 2024 1:49 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> Cc: Chengming Zhou <chengming.zhou@linux.dev>; Johannes Weiner
> <hannes@cmpxchg.org>; Nhat Pham <nphamcs@gmail.com>; linux-
> kernel@vger.kernel.org; linux-mm@kvack.org; 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 Fri, Nov 15, 2024 at 1:14 PM Sridhar, Kanchana P
> <kanchana.p.sridhar@intel.com> wrote:
> >
> > Hi Chengming,
> >
> > > -----Original Message-----
> > > From: Chengming Zhou <chengming.zhou@linux.dev>
> > > Sent: Wednesday, November 13, 2024 11:24 PM
> > > To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>; Johannes
> Weiner
> > > <hannes@cmpxchg.org>
> > > Cc: Nhat Pham <nphamcs@gmail.com>; Yosry Ahmed
> > > <yosryahmed@google.com>; linux-kernel@vger.kernel.org; linux-
> > > mm@kvack.org; 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().
> > >
> > > Hello,
> > >
> > > On 2024/11/14 14:37, Sridhar, Kanchana P wrote:
> > > >
> > > >> -----Original Message-----
> > > >> From: Johannes Weiner <hannes@cmpxchg.org>
> > > >> Sent: Wednesday, November 13, 2024 9:12 PM
> > > >> To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> > > >> Cc: Nhat Pham <nphamcs@gmail.com>; Yosry Ahmed
> > > >> <yosryahmed@google.com>; linux-kernel@vger.kernel.org; linux-
> > > >> mm@kvack.org; 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 Thu, Nov 14, 2024 at 01:56:16AM +0000, Sridhar, Kanchana P
> wrote:
> > > >>> So my question was, can we prevent the migration to a different cpu
> > > >>> by relinquishing the mutex lock after this conditional
> > > >>
> > > >> Holding the mutex doesn't prevent preemption/migration.
> > > >
> > > > Sure, however, is this also applicable to holding the mutex of a per-cpu
> > > > structure obtained via raw_cpu_ptr()?
> > >
> > > Yes, unless you use migration_disable() or cpus_read_lock() to protect
> > > this section.
> >
> > Ok.
> >
> > >
> > > >
> > > > Would holding the mutex prevent the acomp_ctx of the cpu prior to
> > > > the migration (in the UAF scenario you described) from being deleted?
> > >
> > > No, cpu offline can kick in anytime to free the acomp_ctx->buffer.
> > >
> > > >
> > > > If holding the per-cpu acomp_ctx's mutex isn't sufficient to prevent the
> > > > UAF, I agree, we might need a way to prevent the acomp_ctx from being
> > > > deleted, e.g. with refcounts as you've suggested, or to not use the
> > >
> > > Right, refcount solution from Johannes is very good IMHO.
> > >
> > > > acomp_ctx at all for the check, instead use a boolean.
> > >
> > > But this is not enough to just avoid using acomp_ctx for the check,
> > > the usage of acomp_ctx inside the mutex is also UAF, since cpu offline
> > > can kick in anytime to free the acomp_ctx->buffer.
> >
> > I see. How would the refcounts work? Would this add latency to zswap
> > ops? In low memory situations, could the cpu offlining code over-ride
> > the refcounts?
> 
> I think what Johannes meant is that the zswap compress/decompress
> paths grab a ref on the acomp_ctx before using it, and the CPU
> offlining code only drops the initial ref, and does not free the
> buffer directly. The buffer is only freed when the ref drops to zero.
> 
> I am not familiar with CPU hotplug, would it be simpler if we have a
> wrapper like get_acomp_ctx() that disables migration or calls
> cpus_read_lock() before grabbing the per-CPU acomp_ctx? A similar
> wrapper, put_acompt_ctx() will be used after we are done using the
> acomp_ctx.

Would it be sufficient to add a check for mutex_is_locked() in
zswap_cpu_comp_dead() and if this returns true, to exit without deleting
the acomp? If this is an acceptable solution, it would also require us
to move the mutex_unlock() to occur after the "if (src != acomp_ctx->buffer)"
in zswap_decompress(). This would ensure all existing zswap code that's
within the mutex_lock()-mutex_unlock() will work correctly without
worrying about the acomp_ctx being deleted by cpu offlining.

Not sure if this would be a comprehensive solution, or if it would have
unintended consequences to the cpu offlining code. Would appreciate
comments.

Thanks,
Kanchana

> 
> >
> > Based on Johannes' earlier comments, I don't think it makes sense for
> > me to submit a v2.
> >
> > Thanks,
> > Kanchana
> >
> > >
> > > Thanks.