> -----Original Message----- > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > Sent: Friday, November 15, 2024 1:49 PM > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx> > Cc: Chengming Zhou <chengming.zhou@xxxxxxxxx>; Johannes Weiner > <hannes@xxxxxxxxxxx>; Nhat Pham <nphamcs@xxxxxxxxx>; linux- > kernel@xxxxxxxxxxxxxxx; linux-mm@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 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. 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.
