On Tue, Jul 30, 2024 at 04:47:56PM +0800, Baokun Li wrote: > On 2024/7/26 19:45, Ojaswin Mujoo wrote: > > On Wed, Jul 10, 2024 at 12:06:37PM +0800, libaokun@xxxxxxxxxxxxxxx wrote: > > > From: Baokun Li <libaokun1@xxxxxxxxxx> > > > snip > > Hi Baokun, > > > > If i'm not wrong, in this trace, we'll enter ext4_ext_insert_extent() with > > gb_flags having EXT4_GET_BLOCKS_PRE_IO so we won't actually go for a > > merge_up. > > > > That being said, there seems to be a few places where we follow the > > split-insert pattern and it might be possible that one of those call > > sites might not be passing EXT4_GET_BLOCKS_PRE_IO and we'll the double > > free issue you mentioned. I'll check and update if I see anything. > Hi Ojaswin, > > You're right. I am very sorry for the confusion. > > The trace here is wrong, this patch should actually be placed after the two > UAF patches. Here ext4_ext_try_to_merge() is called when trying zeroout in > ext4_split_extent_at(). It is called when trying zeroout with or without > EXT4_GET_BLOCKS_PRE_IO.The correct trace is as follows: > > split2 map split1 > |--------|-------|--------| > > ext4_ext_map_blocks > ext4_ext_handle_unwritten_extents > ext4_split_convert_extents > // path->p_depth == 0 > ext4_split_extent > // 1. do split1 > ext4_split_extent_at > |ext4_ext_insert_extent > | ext4_ext_create_new_leaf > | ext4_ext_grow_indepth > | le16_add_cpu(&neh->eh_depth, 1) > | ext4_find_extent > | // return -ENOMEM > |// get error and try zeroout > |path = ext4_find_extent > | path->p_depth = 1 > |ext4_ext_try_to_merge > | ext4_ext_try_to_merge_up > | path->p_depth = 0 > | brelse(path[1].p_bh) ---> not set to NULL here > |// zeroout success > // 2. update path > ext4_find_extent > // 3. do split2 > ext4_split_extent_at > ext4_ext_insert_extent > ext4_ext_create_new_leaf > ext4_ext_grow_indepth > le16_add_cpu(&neh->eh_depth, 1) > ext4_find_extent > path[0].p_bh = NULL; > path->p_depth = 1 > read_extent_tree_block ---> return err > // path[1].p_bh is still the old value > ext4_free_ext_path > ext4_ext_drop_refs > // path->p_depth == 1 > brelse(path[1].p_bh) ---> brelse a buffer twice > > I'll adjust the order of the patches and correct the trace in the next > version. Hey Baokun, The corrected trace looks good, thanks. Regards, Ojaswin
