Received-SPF: pass (google.com: domain of linux-kernel+bounces-133061-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 5 Apr 2024 15:23:46 +0200
From: Jan Kara <jack@suse.cz>
To: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>, Kemeng Shi <shikemeng@huaweicloud.com>,
	gregkh@linuxfoundation.org, konishi.ryusuke@gmail.com,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nilfs@vger.kernel.org, syzkaller-bugs@googlegroups.com,
	tj@kernel.org, viro@zeniv.linux.org.uk
Subject: Re: [syzbot] [nilfs?] KASAN: slab-out-of-bounds Read in wb_writeback
Message-ID: <20240405132346.bid7gibby3lxxhez@quack3>
References: <000000000000fd0f2a061506cc93@google.com>
 <00000000000003b8c406151e0fd1@google.com>
 <20240403094717.zex45tc2kpkfelny@quack3>
 <20240405-heilbad-eisbrecher-cd0cbc27f36f@brauner>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="q5dy3frwjvcfpelv"
Content-Disposition: inline
In-Reply-To: <20240405-heilbad-eisbrecher-cd0cbc27f36f@brauner>


--q5dy3frwjvcfpelv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri 05-04-24 13:05:59, Christian Brauner wrote:
> On Wed, Apr 03, 2024 at 11:47:17AM +0200, Jan Kara wrote:
> > On Tue 02-04-24 07:38:25, syzbot wrote:
> > > syzbot has found a reproducer for the following issue on:
> > > 
> > > HEAD commit:    c0b832517f62 Add linux-next specific files for 20240402
> > > git tree:       linux-next
> > > console+strace: https://syzkaller.appspot.com/x/log.txt?x=14af7dd9180000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=afcaf46d374cec8c
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=7b219b86935220db6dd8
> > > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1729f003180000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17fa4341180000
> > > 
> > > Downloadable assets:
> > > disk image: https://storage.googleapis.com/syzbot-assets/0d36ec76edc7/disk-c0b83251.raw.xz
> > > vmlinux: https://storage.googleapis.com/syzbot-assets/6f9bb4e37dd0/vmlinux-c0b83251.xz
> > > kernel image: https://storage.googleapis.com/syzbot-assets/2349287b14b7/bzImage-c0b83251.xz
> > > mounted in repro: https://storage.googleapis.com/syzbot-assets/9760c52a227c/mount_0.gz
> > > 
> > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > Reported-by: syzbot+7b219b86935220db6dd8@syzkaller.appspotmail.com
> > > 
> > > ==================================================================
> > > BUG: KASAN: slab-out-of-bounds in __lock_acquire+0x78/0x1fd0 kernel/locking/lockdep.c:5005
> > > Read of size 8 at addr ffff888020485fa8 by task kworker/u8:2/35
> > 
> > Looks like the writeback cleanups are causing some use-after-free issues.
> > The code KASAN is complaining about is:
> > 
> > 		/*
> > 		 * Nothing written. Wait for some inode to
> > 		 * become available for writeback. Otherwise
> > 		 * we'll just busyloop.
> > 		 */
> > 		trace_writeback_wait(wb, work);
> > 		inode = wb_inode(wb->b_more_io.prev);
> > >>>>>		spin_lock(&inode->i_lock); <<<<<<
> > 		spin_unlock(&wb->list_lock);
> > 		/* This function drops i_lock... */
> > 		inode_sleep_on_writeback(inode);
> > 
> > in wb_writeback(). Now looking at the changes indeed the commit
> > 167d6693deb ("fs/writeback: bail out if there is no more inodes for IO and
> > queued once") is buggy because it will result in trying to fetch 'inode'
> > from empty b_more_io list and thus we'll corrupt memory. I think instead of
> > modifying the condition:
> > 
> > 		if (list_empty(&wb->b_more_io)) {
> > 
> > we should do:
> > 
> > -		if (progress) {
> > +		if (progress || !queued) {
> >                         spin_unlock(&wb->list_lock);
> >                         continue;
> >                 }
> > 
> > Kemeng?
> 
> Fwiw, I observed this on xfstest too the last few days and tracked it
> down to this series. Here's the splat I got in case it helps:

OK, since this is apparently causing more issues and Kemeng didn't reply
yet, here's a fix in the form of the patch. It has passed some basic
testing. Feel free to fold it into Kemeng's patch so that we don't keep
linux-next broken longer than necessary. Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--q5dy3frwjvcfpelv
Content-Type: text/x-patch; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-writeback-Fix-memory-corruption-in-writeback-code.patch"

From cede4bc05f7a9a38f21b5943c11592fdb098b4f4 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Fri, 5 Apr 2024 13:57:28 +0200
Subject: [PATCH] writeback: Fix memory corruption in writeback code

Commit 167d6693deb3 ("fs/writeback: bail out if there is no more inodes
for IO and queued once") made the loop in wb_writeback() continue, even
if we didn't have any inodes in b_more_io list when we didn't queue any
inodes into b_io list yet. Conceptually this is fine however the loop in
this case takes the first inode from b_more_io list and waits for
writeback on it to complete. When b_more_io list is empty, this results
in accesses beyond the wb->b_more_io list head corrupting struct
wb_writeback and memory beyond it. Fix the problem by directly
restarting the loop in this case instead of going through waiting on
inode in b_more_io list.

Reported-by: syzbot+7b219b86935220db6dd8@syzkaller.appspotmail.com
Fixes: 167d6693deb3 ("fs/writeback: bail out if there is no more inodes for IO and queued once")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fs-writeback.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index f7ed4192d0f8..92a5b8283528 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2137,7 +2137,7 @@ static long wb_writeback(struct bdi_writeback *wb,
 		 * mean the overall work is done. So we keep looping as long
 		 * as made some progress on cleaning pages or inodes.
 		 */
-		if (progress) {
+		if (progress || !queued) {
 			spin_unlock(&wb->list_lock);
 			continue;
 		}
@@ -2145,7 +2145,7 @@ static long wb_writeback(struct bdi_writeback *wb,
 		/*
 		 * No more inodes for IO, bail
 		 */
-		if (list_empty(&wb->b_more_io) && queued) {
+		if (list_empty(&wb->b_more_io)) {
 			spin_unlock(&wb->list_lock);
 			break;
 		}
-- 
2.35.3


--q5dy3frwjvcfpelv--