On Fri, Nov 15, 2024 at 1:14 PM Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx> wrote: > > Hi Chengming, > > > -----Original Message----- > > From: Chengming Zhou <chengming.zhou@xxxxxxxxx> > > Sent: Wednesday, November 13, 2024 11:24 PM > > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx>; Johannes Weiner > > <hannes@xxxxxxxxxxx> > > Cc: Nhat Pham <nphamcs@xxxxxxxxx>; Yosry Ahmed > > <yosryahmed@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux- > > mm@xxxxxxxxx; usamaarif642@xxxxxxxxx; ryan.roberts@xxxxxxx; Huang, > > Ying <ying.huang@xxxxxxxxx>; 21cnbao@xxxxxxxxx; akpm@linux- > > foundation.org; Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh > > <vinodh.gopal@xxxxxxxxx> > > 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@xxxxxxxxxxx> > > >> Sent: Wednesday, November 13, 2024 9:12 PM > > >> To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx> > > >> Cc: Nhat Pham <nphamcs@xxxxxxxxx>; Yosry Ahmed > > >> <yosryahmed@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux- > > >> mm@xxxxxxxxx; chengming.zhou@xxxxxxxxx; usamaarif642@xxxxxxxxx; > > >> ryan.roberts@xxxxxxx; Huang, Ying <ying.huang@xxxxxxxxx>; > > >> 21cnbao@xxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx; Feghali, Wajdi K > > >> <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh <vinodh.gopal@xxxxxxxxx> > > >> 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. > > Based on Johannes' earlier comments, I don't think it makes sense for > me to submit a v2. > > Thanks, > Kanchana > > > > > Thanks.
