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... I think Johannes' proposal is the safest :)
