From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 00E65175A6E for ; Thu, 7 May 2026 09:40:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778146844; cv=none; b=b2Do70TLhCR3xtU8ubQ8nuHWO+D0FPFXJhb627nCb4T5ZFhMAb9zk9mttvT4Q7jk5L6XZWe4J4m0Z7c+jHW01x36k2vlkn7GOz30kFryuf6zGj1BRj3pmQO+ps0U9pxW/X7SB2bhpwLpsd8T5Y7VP4R+p3M60qkRmgMm/9K6wpw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778146844; c=relaxed/simple; bh=64GYkBNAtE+BDFwDFkPtNvIvvoMh/YbkLtBo5cM1LEs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BGZUIPRCj0OfRiF7vpukdDgrKSZO/toA1HWqN69YhZGijHtowryLxPvpHDGZrgU5CRYYkOkuSAbtIudZdolgcgPRzUPyMMJ8Fw47V3S5Ry8cTe5Hk0n2Jz2w7X9zbdhADeHGYQb98RaVXLNBx7oUb1N+eXwtJk3Dussa6C26VcM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org; spf=pass smtp.mailfrom=chromium.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b=OdOBuNMD; arc=none smtp.client-ip=209.85.214.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=chromium.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="OdOBuNMD" Received: by mail-pl1-f174.google.com with SMTP id d9443c01a7336-2b9ec9443c2so10412715ad.1 for ; Thu, 07 May 2026 02:40:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1778146842; x=1778751642; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=6GHLHxbWZMeODD7NZR/2u5e4BofCNB2jQY/FhKfcUTY=; b=OdOBuNMDycO/Uo4HFziT8+lwXxqNPXds1nOdWtJz7oan9GeDH6gtmZhxhe/1/r8iy/ TOBKbyr7v9st6+KoZEBoejG3GHutf4grTBSspUfLQxCaKt50zIdwFqWgICErVpUV74uT KhN/uQW0uOT77wGMABnMsxbV62JfvomsRJyX0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778146842; x=1778751642; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=6GHLHxbWZMeODD7NZR/2u5e4BofCNB2jQY/FhKfcUTY=; b=eo6O7sYhptIAzxNtLfrTtSEQyaXveq3IdAJ4TNZQbM3yRAAJUhfNYwVMUrmfNKNuSX bxYmrJ4Yyb7DMJmWTUPhcLYLM/7A6W3AVDoL6Q/6Xbo02lFuFgUogoaff/A1gPNSRQNC 2jUq2SBOzYfcxH7xxetyBhhBGVN0F462mHtAalhpGi7Vp06tXsfd8+AzZIUOdoqzy7yG wC7m0NHkU/9jcI0PeV1Xlp2TfozVCqcyxdvjk1Ds2ZQuy4ZwGrgGcn9EsPUgixq1MjFH flNgIUGK3nFIU+amf0JZ0vj2g3NvNMnLQa0TIb5mN7ZSqBKqcpJ2rrjsZX+GLvk1UNSI QRgg== X-Forwarded-Encrypted: i=1; AFNElJ8Z3v2cNO/Rxr6tza3jHIyY7AFgagaDrClPls0FZL8aP4pQym4VhNSuFVpHoLL8IU9G16Dk9zpQGdeN2ao=@vger.kernel.org X-Gm-Message-State: AOJu0Yzov2r0AChi/RxVKRPZbyv2ycwel+PIQdZwLtfYdAMgTrahBBOZ i6/or+VcACwDyf6+MeNe8o57gx0C1gfjNLW4evlRfn3s0h03rUIHKOx/E1ozG1xURA== X-Gm-Gg: AeBDieuZ3i53QkBCJ+tGCf6+dhpdw1QuFkadj8zBqRtltQ7ZkGDstF6+O3qMfdD71X9 W8D36fvgLhUZkZdqUrg9dsDwVSDD4Ur16Zx3S35LeGRdA69BCn2WHmHrKNWwiMHXiHYewrY2Cac YRF31ZJuclm8oP+YJY5lg/0KD+U2BK2mM2eHtr44rShit6WkcO7oOFjj8FGkmYSpw6XhxliAnaY GtrYTy/xENdkKVamnm+5znECFoCK9kJRr+M1xfRsAzT2fksv9nTiMf8PJ19nmIiu4iSu9VJdk4I 2KGsJlkWgYVnugkxzg4ZewpHhqTgvsqAS2b1ywJor16HJiroaxph3KRR6QviGii7WSvsXs5+VGX A3fZxlgGv63tr/f/WKX4xpXVTi7wsLwzeRhfp++e8iM9+vH+y/45eLExndTuMff1y7WyDDbr66o gW2qd6CAsER7V81lSf65yqyhS7/+1T6HC97a7QK8RyRIKjzo7Nsx/PjCRidJOq1ew2F1zXhpMdM w== X-Received: by 2002:a17:903:28c:b0:2b4:64cf:e8f8 with SMTP id d9443c01a7336-2babc859054mr19200625ad.2.1778146841053; Thu, 07 May 2026 02:40:41 -0700 (PDT) Received: from google.com ([2a00:79e0:2031:6:7ba2:1dc2:ed3e:1484]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2babaac3576sm20701395ad.4.2026.05.07.02.40.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 May 2026 02:40:40 -0700 (PDT) Date: Thu, 7 May 2026 18:40:37 +0900 From: Sergey Senozhatsky To: Minchan Kim Cc: Richard Chang , Sergey Senozhatsky , Jens Axboe , Andrew Morton , bgeffon@google.com, liumartin@google.com, linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH] zram: fix use-after-free in zram_writeback_endio Message-ID: References: <20260504123230.3833765-1-richardycc@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On (26/05/05 09:37), Minchan Kim wrote: > > @@ -966,9 +966,8 @@ static void zram_writeback_endio(struct bio *bio) > > > > spin_lock_irqsave(&wb_ctl->done_lock, flags); > > list_add(&req->entry, &wb_ctl->done_reqs); > > - spin_unlock_irqrestore(&wb_ctl->done_lock, flags); > > - > > wake_up(&wb_ctl->done_wait); > > + spin_unlock_irqrestore(&wb_ctl->done_lock, flags); > > } > > > > I agree this will fix the issue, but using a lock to extend the lifetime of > an object to avoid a UAF is not a good pattern. Object lifetime shared between > process and interrupt contexts should be managed explicitly using refcount. ->num_inflight is a ref-counter, basically. The problem is that completion is a two-step process, only one part of each is synchronized with the writeback context. I honestly don't want to have two ref-counts: one for requests pending zram completion and one for active endio contexts. Maybe we can repurpose num_inflight instead. > Furthermore, keeping wake_up() outside the critical section minimizes > interrupt-disabled latency So I considered that, but isn't endio already called from IRQ context? Just asking. We wakeup only one waiter (writeback task), so it's not that bad CPU-cycles wise. Do you think it's really a concern? wake_up() under spin-lock solves the problem of a unsynchronized two-stages endio process. > and avoids nesting spinlocks (done_lock -> done_wait.lock), reducing > the risk of future lockdep issues, just in case. I considered lockdep as well but ruled it out as impossible scenario, nesting here is strictly uni-directional, we never call into zram from the scheduler. Just saying. > It definitely will add more overhead for the submission/completion paths to deal > with the refcount, but I think we should go that way at the cost of runtime. Dunno, something like below maybe? --- drivers/block/zram/zram_drv.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index ce2e1c79fc75..27fe50d666d7 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -967,7 +967,7 @@ static int zram_writeback_complete(struct zram *zram, struct zram_wb_req *req) static void zram_writeback_endio(struct bio *bio) { struct zram_wb_req *req = container_of(bio, struct zram_wb_req, bio); - struct zram_wb_ctl *wb_ctl = bio->bi_private; + struct zram_wb_ctl *wb_ctl = READ_ONCE(bio->bi_private); unsigned long flags; spin_lock_irqsave(&wb_ctl->done_lock, flags); @@ -975,6 +975,7 @@ static void zram_writeback_endio(struct bio *bio) spin_unlock_irqrestore(&wb_ctl->done_lock, flags); wake_up(&wb_ctl->done_wait); + atomic_dec(&wb_ctl->num_inflight); } static void zram_submit_wb_request(struct zram *zram, @@ -998,7 +999,7 @@ static int zram_complete_done_reqs(struct zram *zram, unsigned long flags; int ret = 0, err; - while (atomic_read(&wb_ctl->num_inflight) > 0) { + for (;;) { spin_lock_irqsave(&wb_ctl->done_lock, flags); req = list_first_entry_or_null(&wb_ctl->done_reqs, struct zram_wb_req, entry); @@ -1006,7 +1007,6 @@ static int zram_complete_done_reqs(struct zram *zram, list_del(&req->entry); spin_unlock_irqrestore(&wb_ctl->done_lock, flags); - /* ->num_inflight > 0 doesn't mean we have done requests */ if (!req) break; @@ -1014,7 +1014,6 @@ static int zram_complete_done_reqs(struct zram *zram, if (err) ret = err; - atomic_dec(&wb_ctl->num_inflight); release_pp_slot(zram, req->pps); req->pps = NULL; @@ -1129,8 +1128,11 @@ static int zram_writeback_slots(struct zram *zram, if (req) release_wb_req(req); - while (atomic_read(&wb_ctl->num_inflight) > 0) { - wait_event(wb_ctl->done_wait, !list_empty(&wb_ctl->done_reqs)); + while (atomic_read(&wb_ctl->num_inflight) || + !list_empty(&wb_ctl->done_reqs)) { + wait_event_timeout(wb_ctl->done_wait, + !list_empty(&wb_ctl->done_reqs), + HZ); err = zram_complete_done_reqs(zram, wb_ctl); if (err) ret = err; -- 2.54.0.563.g4f69b47b94-goog