On 6/11/25 15:51, Zhiguo Niu wrote: > Zhiguo Niu <niuzhigu...@gmail.com> 于2025年6月11日周三 14:52写道: >> >> Chao Yu <c...@kernel.org> 于2025年6月11日周三 14:47写道: >>> >>> On 6/11/25 14:41, Zhiguo Niu wrote: >>>> Chao Yu <c...@kernel.org> 于2025年6月11日周三 14:07写道: >>>>> >>>>> On 6/11/25 00:08, Jaegeuk Kim wrote: >>>>>> Hi Zhiguo, >>>>>> >>>>>> This patch causes CPU hang when running fsstress on >>>>>> compressed/non-compressed >>>>>> files. Please check. >>>>> >>>>> Oh, seems it may cause below deadlock: >>>>> >>>>> CPU0 >>>>> process A >>>>> - spin_lock(i_lock) >>>>> software IRQ >>>>> - end_io >>>>> - igrab >>>>> - spin_lock(i_lock) >>>>> >>>>> Thanks, >>>> Hi Chao, >>>> Thanks for pointing this out. >>>> I have tested this patch locally about some basic cases before submission. >>>> So it seems that should use the following method to solve this problem? >>>> " store i_compress_algorithm/sbi in dic to avoid inode access?" >>> >>> Zhiguo, >>> >>> Yeah, I guess so. >> Hi Chao, >> OK, I will prepare it . >> Thanks a lot. >>> >>> Thanks, >>> >>>> thanks! >>>> >>>> >>>>> >>>>>> >>>>>> On 06/05, Zhiguo Niu wrote: >>>>>>> The decompress_io_ctx may be released asynchronously after >>>>>>> I/O completion. If this file is deleted immediately after read, >>>>>>> and the kworker of processing post_read_wq has not been executed yet >>>>>>> due to high workloads, It is possible that the inode(f2fs_inode_info) >>>>>>> is evicted and freed before it is used f2fs_free_dic. >>>>>>> >>>>>>> The UAF case as below: >>>>>>> Thread A Thread B >>>>>>> - f2fs_decompress_end_io >>>>>>> - f2fs_put_dic >>>>>>> - queue_work >>>>>>> add free_dic work to post_read_wq >>>>>>> - do_unlink >>>>>>> - iput >>>>>>> - evict >>>>>>> - call_rcu >>>>>>> This file is deleted after read. >>>>>>> >>>>>>> Thread C kworker to process >>>>>>> post_read_wq >>>>>>> - rcu_do_batch >>>>>>> - f2fs_free_inode >>>>>>> - kmem_cache_free >>>>>>> inode is freed by rcu >>>>>>> - process_scheduled_works >>>>>>> - f2fs_late_free_dic >>>>>>> - f2fs_free_dic >>>>>>> - >>>>>>> f2fs_release_decomp_mem >>>>>>> read >>>>>>> (dic->inode)->i_compress_algorithm >>>>>>> >>>>>>> This patch use igrab before f2fs_free_dic and iput after free the dic >>>>>>> when dic free >>>>>>> action is done by kworker. >>>>>>> >>>>>>> Cc: Daeho Jeong <daehoje...@google.com> >>>>>>> Fixes: bff139b49d9f ("f2fs: handle decompress only post processing in >>>>>>> softirq") >>>>>>> Signed-off-by: Zhiguo Niu <zhiguo....@unisoc.com> >>>>>>> Signed-off-by: Baocong Liu <baocong....@unisoc.com> >>>>>>> --- >>>>>>> v3: use igrab to replace __iget >>>>>>> v2: use __iget/iput function >>>>>>> --- >>>>>>> fs/f2fs/compress.c | 14 +++++++++----- >>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c >>>>>>> index b3c1df9..729ad16 100644 >>>>>>> --- a/fs/f2fs/compress.c >>>>>>> +++ b/fs/f2fs/compress.c >>>>>>> @@ -1687,7 +1687,7 @@ static void f2fs_release_decomp_mem(struct >>>>>>> decompress_io_ctx *dic, >>>>>>> } >>>>>>> >>>>>>> static void f2fs_free_dic(struct decompress_io_ctx *dic, >>>>>>> - bool bypass_destroy_callback); >>>>>>> + bool bypass_destroy_callback, bool late_free); >>>>>>> >>>>>>> struct decompress_io_ctx *f2fs_alloc_dic(struct compress_ctx *cc) >>>>>>> { >>>>>>> @@ -1743,12 +1743,12 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct >>>>>>> compress_ctx *cc) >>>>>>> return dic; >>>>>>> >>>>>>> out_free: >>>>>>> - f2fs_free_dic(dic, true); >>>>>>> + f2fs_free_dic(dic, true, false); >>>>>>> return ERR_PTR(ret); >>>>>>> } >>>>>>> >>>>>>> static void f2fs_free_dic(struct decompress_io_ctx *dic, >>>>>>> - bool bypass_destroy_callback) >>>>>>> + bool bypass_destroy_callback, bool late_free) >>>>>>> { >>>>>>> int i; >>>>>>> >>>>>>> @@ -1775,6 +1775,8 @@ static void f2fs_free_dic(struct >>>>>>> decompress_io_ctx *dic, >>>>>>> } >>>>>>> >>>>>>> page_array_free(dic->inode, dic->rpages, dic->nr_rpages); >>>>>>> + if (late_free) >>>>>>> + iput(dic->inode); >>>>>>> kmem_cache_free(dic_entry_slab, dic); >>>>>>> } >>>>>>> >>>>>>> @@ -1783,16 +1785,18 @@ static void f2fs_late_free_dic(struct >>>>>>> work_struct *work) >>>>>>> struct decompress_io_ctx *dic = >>>>>>> container_of(work, struct decompress_io_ctx, free_work); >>>>>>> >>>>>>> - f2fs_free_dic(dic, false); >>>>>>> + f2fs_free_dic(dic, false, true); >>>>>>> } >>>>>>> >>>>>>> static void f2fs_put_dic(struct decompress_io_ctx *dic, bool in_task) >>>>>>> { >>>>>>> if (refcount_dec_and_test(&dic->refcnt)) { >>>>>>> if (in_task) { >>>>>>> - f2fs_free_dic(dic, false); >>>>>>> + f2fs_free_dic(dic, false, false); >>>>>>> } else { >>>>>>> INIT_WORK(&dic->free_work, f2fs_late_free_dic); >>>>>>> + /* use igrab to avoid inode is evicted >>>>>>> simultaneously */ >>>>>>> + f2fs_bug_on(F2FS_I_SB(dic->inode), >>>>>>> !igrab(dic->inode)); >>>>>>> queue_work(F2FS_I_SB(dic->inode)->post_read_wq, >>>>>>> &dic->free_work); >>>>>>> } >>>>>>> -- >>>>>>> 1.9.1 >>>>> >>> > > Hi Chao, > > The patch is about as follows, because dic->sbi is used directly in > f2fs_free_dic: page_array_free(dic->sbi, dic->tpages, dic->cluster_size); > so there are two points I want to confirm: > 1. As a corresponding, the first parameter (inode) of page_array_alloc > is need to modify to sbi or not ? > 2. As a corresponding, do we need to add the sbi field in compress_ctx > so that page_array_alloc/free called > in compress flow can use sbi directly?
Zhiguo, both 1) and 2) changes look fine to me. Thanks, > Thanks! > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > index b3c1df9..897d8ae 100644 > --- a/fs/f2fs/compress.c > +++ b/fs/f2fs/compress.c > @@ -34,9 +34,8 @@ static void *page_array_alloc(struct inode *inode, int nr) > return f2fs_kzalloc(sbi, size, GFP_NOFS); > } > > -static void page_array_free(struct inode *inode, void *pages, int nr) > +static void page_array_free(struct f2fs_sb_info *sbi, void *pages, int nr) > { > - struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > unsigned int size = sizeof(struct page *) * nr; > > if (!pages) > @@ -155,7 +154,7 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc) > > void f2fs_destroy_compress_ctx(struct compress_ctx *cc, bool reuse) > { > - page_array_free(cc->inode, cc->rpages, cc->cluster_size); > + page_array_free(F2FS_I_SB(cc->inode), cc->rpages, cc->cluster_size); > cc->rpages = NULL; > cc->nr_rpages = 0; > cc->nr_cpages = 0; > @@ -716,7 +715,7 @@ static int f2fs_compress_pages(struct compress_ctx *cc) > if (cc->cpages[i]) > f2fs_compress_free_page(cc->cpages[i]); > } > - page_array_free(cc->inode, cc->cpages, cc->nr_cpages); > + page_array_free(F2FS_I_SB(cc->inode), cc->cpages, cc->nr_cpages); > cc->cpages = NULL; > destroy_compress_ctx: > if (cops->destroy_compress_ctx) > @@ -734,7 +733,7 @@ static void f2fs_release_decomp_mem(struct > decompress_io_ctx *dic, > > void f2fs_decompress_cluster(struct decompress_io_ctx *dic, bool in_task) > { > - struct f2fs_sb_info *sbi = F2FS_I_SB(dic->inode); > + struct f2fs_sb_info *sbi = dic->sbi; > struct f2fs_inode_info *fi = F2FS_I(dic->inode); > const struct f2fs_compress_ops *cops = > f2fs_cops[fi->i_compress_algorithm]; > @@ -1442,13 +1441,13 @@ static int f2fs_write_compressed_pages(struct > compress_ctx *cc, > spin_unlock(&fi->i_size_lock); > > f2fs_put_rpages(cc); > - page_array_free(cc->inode, cc->cpages, cc->nr_cpages); > + page_array_free(F2FS_I_SB(cc->inode), cc->cpages, cc->nr_cpages); > cc->cpages = NULL; > f2fs_destroy_compress_ctx(cc, false); > return 0; > > out_destroy_crypt: > - page_array_free(cc->inode, cic->rpages, cc->cluster_size); > + page_array_free(F2FS_I_SB(cc->inode), cic->rpages, cc->cluster_size); > > for (--i; i >= 0; i--) { > if (!cc->cpages[i]) > @@ -1469,7 +1468,7 @@ static int f2fs_write_compressed_pages(struct > compress_ctx *cc, > f2fs_compress_free_page(cc->cpages[i]); > cc->cpages[i] = NULL; > } > - page_array_free(cc->inode, cc->cpages, cc->nr_cpages); > + page_array_free(F2FS_I_SB(cc->inode), cc->cpages, cc->nr_cpages); > cc->cpages = NULL; > return -EAGAIN; > } > @@ -1499,7 +1498,7 @@ void f2fs_compress_write_end_io(struct bio *bio, > struct page *page) > end_page_writeback(cic->rpages[i]); > } > > - page_array_free(cic->inode, cic->rpages, cic->nr_rpages); > + page_array_free(F2FS_I_SB(cic->inode), cic->rpages, cic->nr_rpages); > kmem_cache_free(cic_entry_slab, cic); > } > > @@ -1637,7 +1636,7 @@ static int f2fs_prepare_decomp_mem(struct > decompress_io_ctx *dic, > f2fs_cops[F2FS_I(dic->inode)->i_compress_algorithm]; > int i; > > - if (!allow_memalloc_for_decomp(F2FS_I_SB(dic->inode), pre_alloc)) > + if (!allow_memalloc_for_decomp(dic->sbi, pre_alloc)) > return 0; > > dic->tpages = page_array_alloc(dic->inode, dic->cluster_size); > @@ -1670,10 +1669,9 @@ static int f2fs_prepare_decomp_mem(struct > decompress_io_ctx *dic, > static void f2fs_release_decomp_mem(struct decompress_io_ctx *dic, > bool bypass_destroy_callback, bool pre_alloc) > { > - const struct f2fs_compress_ops *cops = > - f2fs_cops[F2FS_I(dic->inode)->i_compress_algorithm]; > + const struct f2fs_compress_ops *cops = > f2fs_cops[dic->compress_algorithm]; > > - if (!allow_memalloc_for_decomp(F2FS_I_SB(dic->inode), pre_alloc)) > + if (!allow_memalloc_for_decomp(dic->sbi, pre_alloc)) > return; > > if (!bypass_destroy_callback && cops->destroy_decompress_ctx) > @@ -1708,6 +1706,8 @@ struct decompress_io_ctx *f2fs_alloc_dic(struct > compress_ctx *cc) > > dic->magic = F2FS_COMPRESSED_PAGE_MAGIC; > dic->inode = cc->inode; > + dic->sbi = sbi; > + dic->compress_algorithm = F2FS_I(cc->inode)->i_compress_algorithm; > atomic_set(&dic->remaining_pages, cc->nr_cpages); > dic->cluster_idx = cc->cluster_idx; > dic->cluster_size = cc->cluster_size; > @@ -1762,7 +1762,7 @@ static void f2fs_free_dic(struct decompress_io_ctx *dic, > continue; > f2fs_compress_free_page(dic->tpages[i]); > } > - page_array_free(dic->inode, dic->tpages, dic->cluster_size); > + page_array_free(dic->sbi, dic->tpages, dic->cluster_size); > } > > if (dic->cpages) { > @@ -1771,10 +1771,10 @@ static void f2fs_free_dic(struct decompress_io_ctx > *dic, > continue; > f2fs_compress_free_page(dic->cpages[i]); > } > - page_array_free(dic->inode, dic->cpages, dic->nr_cpages); > + page_array_free(dic->sbi, dic->cpages, dic->nr_cpages); > } > > - page_array_free(dic->inode, dic->rpages, dic->nr_rpages); > + page_array_free(dic->sbi, dic->rpages, dic->nr_rpages); > kmem_cache_free(dic_entry_slab, dic); > } > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 9333a22b..da2137e 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1536,6 +1536,7 @@ struct compress_io_ctx { > struct decompress_io_ctx { > u32 magic; /* magic number to indicate > page is compressed */ > struct inode *inode; /* inode the context belong to */ > + struct f2fs_sb_info *sbi; /* f2fs_sb_info pointer */ > pgoff_t cluster_idx; /* cluster index number */ > unsigned int cluster_size; /* page count in cluster */ > unsigned int log_cluster_size; /* log of cluster size */ > @@ -1576,6 +1577,7 @@ struct decompress_io_ctx { > > bool failed; /* IO error occurred before > decompression? */ > bool need_verity; /* need fs-verity verification > after decompression? */ > + unsigned char compress_algorithm; /* backup algorithm type */ > void *private; /* payload buffer for > specified decompression algorithm */ > void *private2; /* extra payload buffer */ > struct work_struct verity_work; /* work to verify the > decompressed pages */ _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel