linux-kernel - Re: [PATCH v2] fs-writeback: writeback_sb_inodes: Recalculate 'wrote' according skipped pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <4df77267-09f0-c4be-116e-86e447cbd292@huawei.com>
Date:   Mon, 18 Apr 2022 21:27:09 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Hillf Danton <hdanton@...a.com>
CC:     <hch@....de>, <torvalds@...ux-foundation.org>,
        <linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <yukuai3@...wei.com>
Subject: Re: [PATCH v2] fs-writeback: writeback_sb_inodes: Recalculate 'wrote'
 according skipped pages

HI Hillf,
> On Mon, 18 Apr 2022 17:28:24 +0800 Zhihao Cheng wrote:
>> Commit 505a666ee3fc ("writeback: plug writeback in wb_writeback() and
>> writeback_inodes_wb()") has us holding a plug during wb_writeback, which
>> may cause a potential ABBA dead lock:
>>
>>      wb_writeback		fat_file_fsync
>> blk_start_plug(&plug)
>> for (;;) {
>>    iter i-1: some reqs have been added into plug->mq_list  // LOCK A
>>    iter i:
>>      progress = __writeback_inodes_wb(wb, work)
>>      . writeback_sb_inodes // fat's bdev
See comments " fat's bdev".
> 
> 	if (inode->i_state & I_SYNC) {
> 		/* Wait for I_SYNC. This function drops i_lock... */
> 		inode_sleep_on_writeback(inode);
> 		/* Inode may be gone, start again */
> 		spin_lock(&wb->list_lock);
> 		continue;
> 	}
> 	inode->i_state |= I_SYNC;
This inode is fat's bdev's inode.
> 
>>      .   __writeback_single_inode
>>      .   . generic_writepages
>>      .   .   __block_write_full_page
>>      .   .   . . 	    __generic_file_fsync
>>      .   .   . . 	      sync_inode_metadata
>>      .   .   . . 	        writeback_single_inode
> 
> 				if (inode->i_state & I_SYNC) {
This inode is fat's inode.
> 					/*
> 					 * Writeback is already running on the inode.  For WB_SYNC_NONE,
> 					 * that's enough and we can just return.  For WB_SYNC_ALL, we
> 					 * must wait for the existing writeback to complete, then do
> 					 * writeback again if there's anything left.
> 					 */
> 					if (wbc->sync_mode != WB_SYNC_ALL)
> 						goto out;
> 					__inode_wait_for_writeback(inode);
> 				}
> 				inode->i_state |= I_SYNC;
> 
>>      .   .   . . 		  __writeback_single_inode
>>      .   .   . . 		    fat_write_inode
>>      .   .   . . 		      __fat_write_inode
>>      .   .   . . 		        sync_dirty_buffer	// fat's bdev
>>      .   .   . . 			  lock_buffer(bh)	// LOCK B
>>      .   .   . . 			    submit_bh
>>      .   .   . . 			      blk_mq_get_tag	// LOCK A
>>      .   .   . trylock_buffer(bh)  // LOCK B
> 
> Given I_SYNC checked on both sides, the chance for ABBA deadlock is zero.
Above two inodes are not same.