From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f51.google.com (mail-ot1-f51.google.com [209.85.210.51]) (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 2E5AF31F98C for ; Wed, 20 May 2026 03:06:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779246419; cv=none; b=bumNW7mIFOX8ShMrGG3y6OMaMTfhPlaywM4LGUSqUBIyr+DHRSz+QFE55e79pG9t+VD48c2Nf4jK1Nwqvl5vbu21mvOVmTLjClBM67ZbDktBsOv7bVWHuP85UcJfIcSpjguA/eK3mEtR3q/wGigelfFmAkCFZvAjfXzHA1MNkks= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779246419; c=relaxed/simple; bh=09n8hah+N4ZBTDdK9+URDQ5v1kM6v5jDI7553ZxU6P0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=GKJY4uiuQm/YdcmmtQe4JqvM7zeL936a72DlgMCKSlxVMmAatYqqed05fuZs01wNcDEce5rolTRWWoM5tYmwRiQ7AASmfx7/6O3bTHjndfqvp1sUfFMAv4mt0dTv5hF6BJR8OS/4NaOwe+3SGc/oI2cRKJNBhe21DV/2U3UAHCc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=LAfbCiCm; arc=none smtp.client-ip=209.85.210.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="LAfbCiCm" Received: by mail-ot1-f51.google.com with SMTP id 46e09a7af769-7dd73b7c757so2112852a34.0 for ; Tue, 19 May 2026 20:06:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1779246417; x=1779851217; 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=iCJ1DD7tVweiKbLtyjjzWQ55vZMs9gLi6ogTzUVf/4k=; b=LAfbCiCmrDkF9DdbIlR9GMt9BOaUFVHalVzl7r5Kqmcs7fgL5BCOQTR1/QJSqZQAJS tOuFtYtInOX+ZIO1yox2CPq4xEGbNIi6aUkkhyNw53sj1XivBXc6ab6N3BNCov3/FlDk WQ9tspmBQUd6R/WP+5XOaku/8oebq880s1K9CphpNBe6XROrYEUTWIIgW+kv1H6EpLqA Y0DOR1Lo2eCQiTqnM4DU8T+RPF+PheW7W7QbE5HoM4xsAOs4hkJ3AupOobkkmnCY+NgW 8V2xk02DOiXSl2W0etrxgGLxNnwsW00ytBdWp5+9I5O+Lsjj2ofpfSVgG3FA8DrDdR9n +r3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779246417; x=1779851217; 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=iCJ1DD7tVweiKbLtyjjzWQ55vZMs9gLi6ogTzUVf/4k=; b=jLpvyWCVR7/LanKp6W5qes/h8OC5Q4CYE1n2mrmdoacyjVlO25iWVCQG4lx0yRdtYI XWUspptbAl/jCZl6OupyvHJCTErwFpZSIcNCm7tbeSFV/9y3w0auoX928oTodpaZSh8K ZdZdDOHN+BulYktT0h4aFPdJls0Yckgypgd7Eriqga8RB0ebq9oSdxaEg43PggsiRaAb ZN2N6i9FMzQjPOXk1y8SVR7xcq/FN+j62w+iruzz9f3a37fbbL4E/ztKiEu+WGDG46ns wbuKDtxCMo6Epv33SR5GiZQ+PCyiKYUcZM0EwWXWryi/5vDpOz2WqJsvDfsayYPhYYKP vkCA== X-Forwarded-Encrypted: i=1; AFNElJ/QS79vzXyj5Pp0BDVTG5SVAEz3Y8mU0Z2oDLEOUv4S34rfIqza5oQYPLNzl2tVifIEA5+hdqEreJIZXoQ=@vger.kernel.org X-Gm-Message-State: AOJu0Yz4/k7GSey9w+j8ZRiz1Dk6Z36gshLQJdDNDiAlYP2CwR3SUsB6 oxEYoj6ruCjPuxge0oaSfGc1KRLZzTFatlHFYtB0G+DSiB5bRwgAJup5 X-Gm-Gg: Acq92OHCINGiONrz3YBMF3SOvKsXqR0zjtqNQa3DMhlu8xQMiOUPAGTBUQeeIQBfsy+ E+eZUzOnbq0OWIYPgIUz1vHXnnX7MwcJN1+6kB+43oRRpLElt7usLJ7VS6oepu9btvTfsXsz4WH 3lre2yfbcWefDRX07lq8McqlUmNowWDBgzkciq6sl31D96R7jEjbvgh3f/c8wXIX3ychsIDykRl j4rFunUSIIG05P4p5/OB0TN+la+dhLIKqXO1gtOnq7QIezBYnCu4Y1Hx+y0x5S49m38R1bb3opY oA07OQdZIyCFVXNYJgtvPDj75ZuTzX7yB0+3I3OEIDxcBHDDq+lT+SMx26P1BCynpZG7ugsDX3P Bje0KxIh8kc8QRbucm3TCEDhOGOsPPuU4GPm6Ef1+paSEZFlFQcxx6aKEYeITsUqp9q3p2I4g8t 7SHwiatDPRydls8xv2oAJ2Ezq3a0gQSKiZPhnfPASepScAjs+ldkpEVx5tQnKXkI+a9GKElwzSi R2sP01Z5xpCW59/snig X-Received: by 2002:a05:6830:82d3:b0:7dc:dd58:50b8 with SMTP id 46e09a7af769-7e4f2b89890mr14360672a34.13.1779246416970; Tue, 19 May 2026 20:06:56 -0700 (PDT) Received: from fedora ([172.245.82.59]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-7e55b7cd6c0sm12351755a34.4.2026.05.19.20.06.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 May 2026 20:06:56 -0700 (PDT) Date: Wed, 20 May 2026 11:06:51 +0800 From: Ming Lei To: Tetsuo Handa Cc: Bart Van Assche , Jens Axboe , Christoph Hellwig , Damien Le Moal , linux-block , LKML , Andrew Morton Subject: Re: [PATCH v2] loop: Fix NULL pointer dereference by synchronizing lo_release and loop_queue_rq Message-ID: References: <69e2ca14.a00a0220.1bd0ca.0031.GAE@google.com> <9b2032d6-3f36-4d2b-8128-985c08a4fa37@I-love.SAKURA.ne.jp> <20260518174013.4b72dd50a5bcb89daaed1f62@linux-foundation.org> 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 Tue, May 19, 2026 at 06:27:11PM +0900, Tetsuo Handa wrote: > On 2026/05/19 9:40, Andrew Morton wrote: > > AI review asked a couple of questions: > > https://sashiko.dev/#/patchset/9b2032d6-3f36-4d2b-8128-985c08a4fa37@I-love.SAKURA.ne.jp > > To: gemini/gemini-3.1-pro-preview > > Thank you for your valuable feedback. Your point about asynchronous I/O completing after drain_workqueue() > and potentially causing a UAF at file_inode() from kiocb_end_write() from lo_rw_aio_do_completion() is correct. > The drain_workqueue() alone does not wait for in-flight AIOs that have already returned -EIOCBQUEUED. However, > I'm not convinced that use of blk_mq_freeze_queue() inside __loop_clr_fd() where disk->open_mutex was already > held by bdev_release() is absolutely deadlock-free. > > 1. VFS and Block Layer Lock Contention: > __loop_clr_fd() is exclusively invoked from the lo_release() path during the final close of the device. > At this stage, the block layer is holding disk->open_mutex. If we call blk_mq_freeze_queue() here, it will > synchronously wait for all in-flight AIOs to complete. However, the completion paths of those in-flight AIOs > (or subsequent metadata processing in the underlying filesystem) may attempt to acquire resources or execute > code paths that depend on the very same device state or open/close status. This creates a circular dependency, > leading to an unrecoverable hang. > > 2. Memory Reclaim Deadlock: > blk_mq_freeze_queue() blocks until the queue's usage counter drops to zero. If an in-flight AIO requires memory > allocation for metadata updates upon completion, and the system is under heavy memory pressure, it can trigger > direct memory reclaim. If the reclaim path attempts to sync other buffers or interact with the frozen loop > device/queue, a circular deadlock occurs. > > Therefore, I would like to choose SRCU-based synchronization instead of blk_mq_freeze_queue(). > > * Locking: We call srcu_read_lock(&loop_io_srcu) only for asynchronous paths (cmd->use_aio) immediately > before submitting the I/O to the underlying filesystem in lo_rw_aio(). > > * Unlocking: The reader lock is released via srcu_read_unlock() at the very end of the AIO completion handler > (lo_rw_aio_do_completion()). > > * Synchronization: We place synchronize_srcu(&loop_io_srcu) immediately after drain_workqueue() in __loop_clr_fd(). > > I think that this guarantees that __loop_clr_fd() safely blocks until all pending AIO callbacks are 100% completed, > fully eliminating the UAF risk and ensuring the safety of the subsequent mapping_set_gfp_mask() and fput(), while > remaining entirely deadlock-free. > > What do you think about this approach? > > drivers/block/loop.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 0000913f7efc..7c3961f3cbc9 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -80,6 +80,7 @@ struct loop_cmd { > struct list_head list_entry; > bool use_aio; /* use AIO interface to handle I/O */ > atomic_t ref; /* only for aio */ > + int srcu_idx; > long ret; > struct kiocb iocb; > struct bio_vec *bvec; > @@ -93,6 +94,7 @@ struct loop_cmd { > static DEFINE_IDR(loop_index_idr); > static DEFINE_MUTEX(loop_ctl_mutex); > static DEFINE_MUTEX(loop_validate_mutex); > +DEFINE_SRCU(loop_io_srcu); > > /** > * loop_global_lock_killable() - take locks for safe loop_validate_file() test > @@ -327,6 +329,8 @@ static void lo_rw_aio_do_completion(struct loop_cmd *cmd) > kiocb_end_write(&cmd->iocb); > if (likely(!blk_should_fake_timeout(rq->q))) > blk_mq_complete_request(rq); > + if (cmd->use_aio) > + srcu_read_unlock(&loop_io_srcu, cmd->srcu_idx); > } > > static void lo_rw_aio_complete(struct kiocb *iocb, long ret) > @@ -392,6 +396,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, > if (cmd->use_aio) { > cmd->iocb.ki_complete = lo_rw_aio_complete; > cmd->iocb.ki_flags = IOCB_DIRECT; > + cmd->srcu_idx = srcu_read_lock(&loop_io_srcu); > } else { > cmd->iocb.ki_complete = NULL; > cmd->iocb.ki_flags = 0; > @@ -1118,6 +1123,22 @@ static void __loop_clr_fd(struct loop_device *lo) > struct file *filp; > gfp_t gfp = lo->old_gfp_mask; > > + /* > + * Now that loop_queue_rq() sees lo->lo_state != Lo_bound, > + * wait for already started loop_queue_rq() to complete. > + */ > + synchronize_rcu(); > + /* > + * Now that no more works are scheduled by loop_queue_rq(), > + * wait for already scheduled works to complete. > + */ > + drain_workqueue(lo->workqueue); > + /* > + * Now that no more AIO requests are scheduled by lo_rw_aio(), > + * wait for already started AIO to complete. > + */ > + synchronize_srcu(&loop_io_srcu); The IO after close(loop) should be from writeback. rcu/sruc isn't necessary, please see the patch posted in another thread: https://lore.kernel.org/linux-block/agxJdUf1b0JSDAux@fedora/ in which the check on lo->lo_state is moved to loop_handle_cmd(), meantime drain_workqueue() is added for draining in-flight workers. Thanks, Ming