> -----Original Message----- > From: Nhat Pham <nphamcs@xxxxxxxxx> > Sent: Wednesday, November 13, 2024 4:29 PM > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx> > Cc: Johannes Weiner <hannes@xxxxxxxxxxx>; 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 Wed, Nov 13, 2024 at 2:13 PM Sridhar, Kanchana P > <kanchana.p.sridhar@xxxxxxxxx> wrote: > > > > > > > > Thanks Johannes, for these insights. I was thinking of the following > > in zswap_decompress() as creating a non-preemptible context because > > of the call to raw_cpu_ptr() at the start; with this context extending > > until the mutex_unlock(): > > > > acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > > mutex_lock(&acomp_ctx->mutex); > > > > [...] > > > > mutex_unlock(&acomp_ctx->mutex); > > > > if (src != acomp_ctx->buffer) > > zpool_unmap_handle(zpool, entry->handle); > > > > Based on this understanding, I was a bit worried about the > > "acomp_ctx->buffer" in the conditional that gates the > > zpool_unmap_handle() not being the same acomp_ctx as the one > > at the beginning. I may have been confusing myself, since the acomp_ctx > > is not re-evaluated before the conditional, just reused from the > > start. My apologies to you and Yosry! > > > > > > > > That being said, I do think there is a UAF bug in CPU hotplugging. > > > > > > There is an acomp_ctx for each cpu, but note that this is best effort > > > parallelism, not a guarantee that we always have the context of the > > > local CPU. Look closely: we pick the "local" CPU with preemption > > > enabled, then contend for the mutex. This may well put us to sleep and > > > get us migrated, so we could be using the context of a CPU we are no > > > longer running on. This is fine because we hold the mutex - if that > > > other CPU tries to use the acomp_ctx, it'll wait for us. > > > > > > However, if we get migrated and vacate the CPU whose context we have > > > locked, the CPU might get offlined and zswap_cpu_comp_dead() can free > > > the context underneath us. I think we need to refcount the acomp_ctx. > > > > I see. Wouldn't it then seem to make the code more fail-safe to not allow > > the migration to happen until after the check for (src != acomp_ctx->buffer), > by > > moving the mutex_unlock() after this check? Or, use a boolean to determine > > if the unmap_handle needs to be done as Yosry suggested? > > Hmm does it make it safe? It is mutex_lock() that puts the task to > sleep, after which it can get migrated to a different CPU. Moving > mutex_unlock() to below or not doesn't really matter, no? Or am I > missing something here... Thanks for the comments, Nhat. I guess my last comment in response to what Johannes mentioned about the UAF situation, is the one that's still an open from my perspective. If the process can get migrated after the mutex unlock, and if the acomp_ctx obtained earlier gets deleted (as Johannes was mentioning), then we could be accessing an invalid pointer in the "if (src != acomp_ctx->buffer)". So my question was, can we prevent the migration to a different cpu by relinquishing the mutex lock after this conditional; or, make the conditional be determined by a boolean that gets set earlier to decide whether or not to unmap the zpool handle (as against using the acomp_ctx to decide this). Thanks, Kanchana > > I think Johannes' proposal is the safest :)
