Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 15 Jun 2022 20:51:39 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Ye Bin <yebin10@huawei.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        jack@suse.cz
Subject: Re: [PATCH -next] ext4: fix bug_on in ext4_iomap_begin as race
 between bmap and write
Message-ID: <20220615152139.vp64tnv46enwnfcs@riteshh-domain>
References: <20220615135850.1961759-1-yebin10@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220615135850.1961759-1-yebin10@huawei.com>
Precedence: bulk

On 22/06/15 09:58PM, Ye Bin wrote:
> We got issue as follows:
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 9310 at fs/ext4/inode.c:3441 ext4_iomap_begin+0x182/0x5d0
> RIP: 0010:ext4_iomap_begin+0x182/0x5d0
> RSP: 0018:ffff88812460fa08 EFLAGS: 00010293
> RAX: ffff88811f168000 RBX: 0000000000000000 RCX: ffffffff97793c12
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
> RBP: ffff88812c669160 R08: ffff88811f168000 R09: ffffed10258cd20f
> R10: ffff88812c669077 R11: ffffed10258cd20e R12: 0000000000000001
> R13: 00000000000000a4 R14: 000000000000000c R15: ffff88812c6691ee
> FS:  00007fd0d6ff3740(0000) GS:ffff8883af180000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fd0d6dda290 CR3: 0000000104a62000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  iomap_apply+0x119/0x570
>  iomap_bmap+0x124/0x150
>  ext4_bmap+0x14f/0x250
>  bmap+0x55/0x80
>  do_vfs_ioctl+0x952/0xbd0
>  __x64_sys_ioctl+0xc6/0x170
>  do_syscall_64+0x33/0x40
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Above issue may happen as follows:
>           bmap                    write
> bmap
>   ext4_bmap
>     iomap_bmap
>       ext4_iomap_begin
>                             ext4_file_write_iter
> 			      ext4_buffered_write_iter
> 			        generic_perform_write
> 				  ext4_da_write_begin
> 				    ext4_da_write_inline_data_begin
> 				      ext4_prepare_inline_data
> 				        ext4_create_inline_data
> 					  ext4_set_inode_flag(inode,
> 						EXT4_INODE_INLINE_DATA);
>       if (WARN_ON_ONCE(ext4_has_inline_data(inode))) ->trigger bug_on
>
> To solved above issue hold inode lock in ext4_bamp.
											^^^ ext4_bmap()

I checked the paths where bmap() kernel api can be called i.e. from jbd2/fc and
generic_swapfile_activate() (apart from ioctl())
For jbd2, it will be called with j_inode within bmap(), hence taking a inode lock
of the inode passed within ext4_bmap() (j_inode in this case) should be safe here.
Same goes with swapfile path as well.

However I feel maybe we should hold inode_lock_shared() since there is no
block/extent map layout changes that can happen via ext4_bmap().
Hence read lock is what IMO should be used here.

-ritesh


>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  fs/ext4/inode.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 53877ffe3c41..f4a95c80f644 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3142,13 +3142,15 @@ static sector_t ext4_bmap(struct address_space *mapping, sector_t block)
>  {
>  	struct inode *inode = mapping->host;
>  	journal_t *journal;
> +	sector_t ret = 0;
>  	int err;
>
> +	inode_lock(inode);
>  	/*
>  	 * We can get here for an inline file via the FIBMAP ioctl
>  	 */
>  	if (ext4_has_inline_data(inode))
> -		return 0;
> +		goto out;
>
>  	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
>  			test_opt(inode->i_sb, DELALLOC)) {
> @@ -3187,10 +3189,14 @@ static sector_t ext4_bmap(struct address_space *mapping, sector_t block)
>  		jbd2_journal_unlock_updates(journal);
>
>  		if (err)
> -			return 0;
> +			goto out;
>  	}
>
> -	return iomap_bmap(mapping, block, &ext4_iomap_ops);
> +	ret = iomap_bmap(mapping, block, &ext4_iomap_ops);
> +
> +out:
> +	inode_unlock(inode);
> +	return ret;
>  }
>
>  static int ext4_read_folio(struct file *file, struct folio *folio)
> --
> 2.31.1
>