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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 15 Jun 2022 12:03:41 +0800 From: yebin <yebin10@...wei.com> To: Eric Whitney <enwlinux@...il.com> CC: <tytso@....edu>, <adilger.kernel@...ger.ca>, <linux-ext4@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <jack@...e.cz> Subject: Re: [PATCH -next] ext4: Fix warning in ext4_da_release_space On 2022/6/15 8:53, Eric Whitney wrote: > * yebin <yebin10@...wei.com>: >> ping... >> >> On 2022/5/20 10:55, Ye Bin wrote: >>> We got issue as follows: >>> WARNING: CPU: 2 PID: 1936 at fs/ext4/inode.c:1511 ext4_da_release_space+0x1b9/0x266 >>> Modules linked in: >>> CPU: 2 PID: 1936 Comm: dd Not tainted 5.10.0+ #344 >>> RIP: 0010:ext4_da_release_space+0x1b9/0x266 >>> RSP: 0018:ffff888127307848 EFLAGS: 00010292 >>> RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffffffff843f67cc >>> RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffed1024e60ed9 >>> RBP: ffff888124dc8140 R08: 0000000000000083 R09: ffffed1075da6d23 >>> R10: ffff8883aed36917 R11: ffffed1075da6d22 R12: ffff888124dc83f0 >>> R13: ffff888124dc844c R14: ffff888124dc8168 R15: 000000000000000c >>> FS: 00007f6b7247d740(0000) GS:ffff8883aed00000(0000) knlGS:0000000000000000 >>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>> CR2: 00007ffc1a0b7dd8 CR3: 00000001065ce000 CR4: 00000000000006e0 >>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >>> Call Trace: >>> ext4_es_remove_extent+0x187/0x230 >>> mpage_release_unused_pages+0x3af/0x470 >>> ext4_writepages+0xb9b/0x1160 >>> do_writepages+0xbb/0x1e0 >>> __filemap_fdatawrite_range+0x1b1/0x1f0 >>> file_write_and_wait_range+0x80/0xe0 >>> ext4_sync_file+0x13d/0x800 >>> vfs_fsync_range+0x75/0x140 >>> do_fsync+0x4d/0x90 >>> __x64_sys_fsync+0x1d/0x30 >>> do_syscall_64+0x33/0x40 >>> entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> >>> Above issue may happens as follows: >>> process1 process2 >>> ext4_da_write_begin >>> ext4_da_reserve_space >>> ext4_es_insert_delayed_block[1/1] >>> ext4_da_write_begin >>> ext4_es_insert_delayed_block[0/1] >>> ext4_writepages >>> ****Delayed block allocation failed**** >>> mpage_release_unused_pages >>> ext4_es_remove_extent[1/1] >>> ext4_da_release_space [reserved 0] >>> >>> ext4_da_write_begin >>> ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk) >>> ->As there exist [0, 1] extent, so will return true >>> ext4_writepages >>> ****Delayed block allocation failed**** >>> mpage_release_unused_pages >>> ext4_es_remove_extent[0/1] >>> ext4_da_release_space [reserved 1] >>> ei->i_reserved_data_blocks [1->0] >>> >>> ext4_es_insert_delayed_block[1/1] >>> >>> ext4_writepages >>> ****Delayed block allocation failed**** >>> mpage_release_unused_pages >>> ext4_es_remove_extent[1/1] >>> ext4_da_release_space [reserved 1] >>> ei->i_reserved_data_blocks[0, -1] >>> ->As ei->i_reserved_data_blocks already is zero but to_free is 1, >>> will trigger warning. >>> >>> To solve above issue, introduce i_clu_lock to protect insert delayed >>> block and remove block under cluster delay allocate mode. >>> >>> Signed-off-by: Ye Bin <yebin10@...wei.com> >>> --- >>> fs/ext4/ext4.h | 3 +++ >>> fs/ext4/extents_status.c | 5 +++++ >>> fs/ext4/inode.c | 11 +++++++++-- >>> fs/ext4/super.c | 1 + >>> 4 files changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h >>> index bcd3b9bf8069..47c88ac4d4a8 100644 >>> --- a/fs/ext4/ext4.h >>> +++ b/fs/ext4/ext4.h >>> @@ -1169,6 +1169,9 @@ struct ext4_inode_info { >>> __u32 i_csum_seed; >>> kprojid_t i_projid; >>> + >>> + /* Protect concurrent add cluster delayed block and remove block */ >>> + struct mutex i_clu_lock; >>> }; >>> /* >>> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c >>> index 9a3a8996aacf..dd679014db98 100644 >>> --- a/fs/ext4/extents_status.c >>> +++ b/fs/ext4/extents_status.c >>> @@ -1433,6 +1433,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk, >>> int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk, >>> ext4_lblk_t len) >>> { >>> + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); >>> ext4_lblk_t end; >>> int err = 0; >>> int reserved = 0; >>> @@ -1455,9 +1456,13 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk, >>> * so that we are sure __es_shrink() is done with the inode before it >>> * is reclaimed. >>> */ >>> + if (sbi->s_cluster_ratio != 1) >>> + mutex_lock(&EXT4_I(inode)->i_clu_lock); >>> write_lock(&EXT4_I(inode)->i_es_lock); >>> err = __es_remove_extent(inode, lblk, end, &reserved); >>> write_unlock(&EXT4_I(inode)->i_es_lock); >>> + if (sbi->s_cluster_ratio != 1) >>> + mutex_unlock(&EXT4_I(inode)->i_clu_lock); >>> ext4_es_print_tree(inode); >>> ext4_da_release_space(inode, reserved); >>> return err; >>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >>> index 01c9e4f743ba..1109d77ad60b 100644 >>> --- a/fs/ext4/inode.c >>> +++ b/fs/ext4/inode.c >>> @@ -1649,17 +1649,22 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) >>> goto errout; >>> reserved = true; >>> } else { /* bigalloc */ >>> + mutex_lock(&EXT4_I(inode)->i_clu_lock); >>> if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) { >>> if (!ext4_es_scan_clu(inode, >>> &ext4_es_is_mapped, lblk)) { >>> ret = ext4_clu_mapped(inode, >>> EXT4_B2C(sbi, lblk)); >>> - if (ret < 0) >>> + if (ret < 0) { >>> + mutex_unlock(&EXT4_I(inode)->i_clu_lock); >>> goto errout; >>> + } >>> if (ret == 0) { >>> ret = ext4_da_reserve_space(inode); >>> - if (ret != 0) /* ENOSPC */ >>> + if (ret != 0) { /* ENOSPC */ >>> + mutex_unlock(&EXT4_I(inode)->i_clu_lock); >>> goto errout; >>> + } >>> reserved = true; >>> } else { >>> allocated = true; >>> @@ -1671,6 +1676,8 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) >>> } >>> ret = ext4_es_insert_delayed_block(inode, lblk, allocated); >>> + if (sbi->s_cluster_ratio != 1) >>> + mutex_unlock(&EXT4_I(inode)->i_clu_lock); >>> if (ret && reserved) >>> ext4_da_release_space(inode, 1); >>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >>> index c5021ca0a28a..aa6f2a68bf41 100644 >>> --- a/fs/ext4/super.c >>> +++ b/fs/ext4/super.c >>> @@ -1347,6 +1347,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb) >>> INIT_WORK(&ei->i_rsv_conversion_work, ext4_end_io_rsv_work); >>> ext4_fc_init_inode(&ei->vfs_inode); >>> mutex_init(&ei->i_fc_lock); >>> + mutex_init(&ei->i_clu_lock); >>> return &ei->vfs_inode; >>> } > As you have reported, it looks like there is potential for a race between > mpage_release_unused_pages and ext4_insert_delayed_block when running on a > bigalloc file system in rare circumstances. There should have been locking > in mpage_release_unused_pages to avoid this. The locking was missed when new > code was added to ext4_insert_delayed_block to handle bigalloc delayed block > accounting. > > I have a solution that addresses that omission and is simpler than your > proposed fix. It does not require adding a new lock. I'm putting that > patch through the usual testing cycles, plus new stress testing to verify that > no lock dependency problems exist. It looks good so far, and I expect to post > it tomorrow. If you would be willing to test that patch, we would welcome > it. As you have not chosen to explain how you've triggered the race, you > are in a better position to determine whether the patch will successfully > address it. I've provided a way to reproduce. > It's important to understand that the delayed block allocation failures > you have noted in your call graphs are far more important events than the > block accounting errors that occur due to the race. The delayed block > allocation failures represent data loss for the user, and they should really > never happen. That's why we'd like to understand how you triggered the race, > whether by fuzzing or some other method. Depending on what was done, there > may be a need to harden the code. > > Eric > . >
Powered by blists - more mailing lists