From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from zeniv.linux.org.uk (zeniv.linux.org.uk [62.89.141.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9FD0215E97 for ; Thu, 9 Jan 2025 07:32:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=62.89.141.173 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736407954; cv=none; b=sxLU7gfYq6Biw1cKKES4ZHObMhZjazazkvubVa12j6Um8PKBdkO6dyXeHV3PdeLJJu40tYW4ZJELIS5rnbSnoryfDvVirO6muXC8WRR9yYxNFVCkmVpCoHuFBApTKJWqDti9sKRlO7Ig3E7UHeAKlt7y+yjM1Rzk5Z23JTiZylo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736407954; c=relaxed/simple; bh=mzsm7XcNJhR/AyBey/Gi9OaSKUB/5GkMvMWhgiwFNpQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=qOJaY/d46/caDnnCTBqjR0OjbUkmEXJKAx4nRMCZD5cfqMs0Scggeu1ZrHpahq4fYC+u/YsTi+3xG+zMV6SXd9S3mJ3a1MuDkwozg7A4QjrgNL51W/ts8MnvK41WOVPZffZrkm/xw4tXfqgzB5XmLmBUaG1DgSYl5rEOLEsgYgw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=zeniv.linux.org.uk; spf=none smtp.mailfrom=ftp.linux.org.uk; dkim=pass (2048-bit key) header.d=linux.org.uk header.i=@linux.org.uk header.b=vQ3dZz9A; arc=none smtp.client-ip=62.89.141.173 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=zeniv.linux.org.uk Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ftp.linux.org.uk Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linux.org.uk header.i=@linux.org.uk header.b="vQ3dZz9A" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=linux.org.uk; s=zeniv-20220401; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=pCVE4L8XnjNOQy1a1OFwBig4c+cM8+qEDxKUxQwTnk0=; b=vQ3dZz9AHaj4JCcdiCJthse6pP bArGE7YWdN08fVbOMR/tt7QW4kaT1uu3niECdxv5u/YopbF/hXMIl+VXHQAHLMqpGiNRSoAgFu6kr O+Cjo0c7OOygnK2aaOZNUN7/7JR2ngeD3bITF9dWtJ4s45225yHLuZG2w1elgzsh+yEKEXAYaGdW5 XjTAMFJjDAZ+HRXdUMWlWYJB2fVDrj5zBZMrgXtzjtnY2FPpPGFQFEpkJvT9Sb7aK+nkxJD/H0YKQ KEFSiBrD1/KoC7n3wuXxZ55PTmDbSQ1TYVhcoKDOTUTxdHbZZgnaZqrzOqPFfLJ9Xyl4jRHmB+i4N CNHZmW8A==; Received: from viro by zeniv.linux.org.uk with local (Exim 4.98 #2 (Red Hat Linux)) id 1tVn1o-0000000H9Tb-1rFp; Thu, 09 Jan 2025 07:32:28 +0000 Date: Thu, 9 Jan 2025 07:32:28 +0000 From: Al Viro To: Lizhi Xu Cc: aivazian.tigran@gmail.com, linux-kernel@vger.kernel.org, syzbot+80e60df48923e1b7691d@syzkaller.appspotmail.com, syzkaller-bugs@googlegroups.com Subject: Re: [PATCH] bfs: put a inode if link count is 0 Message-ID: <20250109073228.GR1977892@ZenIV> References: <20250109062216.GQ1977892@ZenIV> <20250109063939.1937072-1-lizhi.xu@windriver.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: <20250109063939.1937072-1-lizhi.xu@windriver.com> Sender: Al Viro On Thu, Jan 09, 2025 at 02:39:39PM +0800, Lizhi Xu wrote: > On Thu, 9 Jan 2025 06:22:16 +0000, Al Viro wrote: > > > The reproducer performs the rename operation on the file twice in succession > > > and changes the file to the same file name. After the first rename operation, > > > the number of links in the inode is set to 0. In the second execution, the > > > same inode is used, resulting in a 0 value warning for i_nlink. > > > > > > To avoid this issue, put the target inode before exiting the bfs_rename. > > > > This is completely insane - you get an extra drop of in-core inode > > refcount, which *will* end up with dangling pointer and memory corruption. > > Besides, there is a perfectly legitimate case when you open a file and > > rename something on top of it. It MUST remain open and alive until the > > last in-core reference to inode goes away, which must not happen before > > close(). > In the reproducer, changes the file to the same file, the same file name is > "file0", file0 uses mknod to create its inode and sets the i_nlink value to 1. > There is no operation to open file0 in the reproducer. Is this situation also > as you said? I'm not sure I understand your sentence, to be honest. Your patch is 100% wrong - you must *not*, under any circumstances, have ->rename() drop references to in-core struct inode instances. It's *always* wrong; the reference to new_inode in new_dentry->d_inode remains there (as it ought to) and its contribution to new_inode refcount remains unchanged. It has nothing to do with ->i_nlink; you are decrementing ->i_count, which controls the lifetime of in-core struct inode instance. As soon as that reaches zero, struct inode instance will be freed. And destructor of new_dentry *will* call iput() on its ->d_inode, so you'll end up with attempt to decrement refcount of already freed memory object. Again, that has nothing to do with ->i_nlink. I'm not familiar with bfs layout, so I can't tell what's going on with the corrupted image syzbot are messing with just by visual examination. Is there, by any chance, a preexisting file0 in the root of that bfs image? Does mknod() succeed there? Because if it doesn't and what you have is a buggered image with 'file0' being there having zero link count, yes, renaming over it would trigger warnings about detected corrupted filesystem - we are trying to remove a link (on disk) to something that claims (on disk) to have 0 links at the time of that operation; something is clearly wrong and deserves a warning. Again, that iput() in there is basically introducing random memory corruption; it might make the warning go away, but it's not a fix.