linux-kernel - Re: [PATCH] f2fs: fix panic in f2fs_put

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <30a5f29b-886f-4ee7-93d1-8c3e1bb8bb49@kernel.org>
Date: Thu, 16 May 2024 17:32:16 +0800
From: Chao Yu <chao@...nel.org>
To: sunshijie <sunshijie@...omi.corp-partner.google.com>, jaegeuk@...nel.org,
 linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Cc: sunshijie <sunshijie@...omi.com>
Subject: Re: [PATCH] f2fs: fix panic in f2fs_put_super

On 2024/5/16 16:55, sunshijie wrote:
> When thread A calls kill_f2fs_super, Thread A first executes the code sbi->node_inode = NULL;
> Then thread A may submit a bio to the function iput(sbi->meta_inode);
> Then thread A enters the process D state,
> Now that the bio submitted by thread A is complete, it calls f2fs_write_end_io and may trigger null-ptr-deref in NODE_MAPPING.

I didn't get it, if there is no cp_err, f2fs_write_checkpoint() in
f2fs_put_super() will flush all dirty pages of node_inode, if there is
cp_err, below flow will keep all dirty pages being truncated, and
there is sanity check on all types of dirty pages.

	/* our cp_error case, we can wait for any writeback page */
	f2fs_flush_merged_writes(sbi);

	f2fs_wait_on_all_pages(sbi, F2FS_WB_CP_DATA);

	if (err || f2fs_cp_error(sbi)) {
		truncate_inode_pages_final(NODE_MAPPING(sbi));
		truncate_inode_pages_final(META_MAPPING(sbi));
	}

	for (i = 0; i < NR_COUNT_TYPE; i++) {
		if (!get_pages(sbi, i))
			continue;
		f2fs_err(sbi, "detect filesystem reference count leak during "
			"umount, type: %d, count: %lld", i, get_pages(sbi, i));
		f2fs_bug_on(sbi, 1);
	}

So, is there any missing case that dirty page of node_inode is missed by
f2fs_put_super()?

Thanks,

> 
> Thread A                                          IRQ context
> - f2fs_put_super
>   - sbi->node_inode = NULL;
>   - iput(sbi->meta_inode);
>    - iput_final
>     - write_inode_now
>      - writeback_single_inode
>       - __writeback_single_inode
>        - filemap_fdatawait
>         - filemap_fdatawait_range
>          - __kcfi_typeid_free_transhuge_page
>           - __filemap_fdatawait_range
>            - wait_on_page_writeback
>             - folio_wait_writeback
>              - folio_wait_bit
>               - folio_wait_bit_common
>                - io_schedule
> 
>                                                    - __handle_irq_event_percpu
>                                                     - ufs_qcom_mcq_esi_handler
>                                                      - ufshcd_mcq_poll_cqe_nolock
>                                                       - ufshcd_compl_one_cqe
>                                                        - scsi_done
>                                                         - scsi_done_internal
>                                                          - blk_mq_complete_request
>                                                           - scsi_complete
>                                                            - scsi_finish_command
>                                                             - scsi_io_completion
>                                                              - scsi_end_request
>                                                               - blk_update_request
>                                                                - bio_endio
>                                                                 - f2fs_write_end_io
>                                                                  - NODE_MAPPING(sbi)
> 
> Signed-off-by: sunshijie <sunshijie@...omi.com>
> ---
>   fs/f2fs/super.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index adffc9b80a9c..aeb085e11f9a 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1641,12 +1641,12 @@ static void f2fs_put_super(struct super_block *sb)
>   
>   	f2fs_destroy_compress_inode(sbi);
>   
> -	iput(sbi->node_inode);
> -	sbi->node_inode = NULL;
> -
>   	iput(sbi->meta_inode);
>   	sbi->meta_inode = NULL;
>   
> +	iput(sbi->node_inode);
> +	sbi->node_inode = NULL;
> +
>   	mutex_unlock(&sbi->umount_mutex);
>   
>   	/*