Re: [PATCH -next] ext4: Remove redundant null pointer check

[Baokun Li <libaokun1@huawei.com>]

Hi Theodore,

Unfortunately it is possible that the regression was introduced
precisely by the mishandling of conflicts here.

On 2024/9/3 20:50, Theodore Ts'o wrote:
> On Tue, Sep 03, 2024 at 03:52:01PM +0800, Baokun Li wrote:
>> Thanks for the cleanup patch.
>>
>> But the change is already included in the patch:
>>
>>   https://lore.kernel.org/all/20240710040654.1714672-21-libaokun@huaweicloud.com/
> Yeah, I noticed.  I had already applied Zetao's patch when I processed
> yours, so I just ended up manually handling the patch conflict.
>
> (I haven't set out the patch acks yet, because the current state of
> the ext4/dev branch is apparently causing a test regression which I'm
> trying to root cause.  They will be in tomorrow's fs-next and
> linux-next branch, though unless I end up figuring out the problematic
> patch or patch series, and end up dropping them from the ext4 dev
> branch today.  Still, feel free to take a look and let me know if I
> screwed up anything.)
>
> 						- Ted
>

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=92bbb922166cd7a829bf60d168372ffa1c54e81d

The changes after resolving the conflict in ext4_ext_clear_bb() are
as follows:

---------------------------------------------------------------------------

@@ -6128,12 +6122,9 @@ int ext4_ext_clear_bb(struct inode *inode)
         if (IS_ERR(path))
                 return PTR_ERR(path);
         ex = path[path->p_depth].p_ext;
-       if (!ex) {
-               ext4_free_ext_path(path);
-               return 0;
-       }
+       if (!ex)
+               goto out;
         end = le32_to_cpu(ex->ee_block) + ext4_ext_get_actual_len(ex);
-       ext4_free_ext_path(path);

         cur = 0;
         while (cur < end) {
@@ -6146,7 +6137,6 @@ int ext4_ext_clear_bb(struct inode *inode)
                         path = ext4_find_extent(inode, map.m_lblk, 
NULL, 0);
                         if (!IS_ERR(path)) {
                                 for (j = 0; j < path->p_depth; j++) {
-
ext4_mb_mark_bb(inode->i_sb,
path[j].p_block, 1, false);
ext4_fc_record_regions(inode->i_sb, inode->i_ino,
@@ -6161,5 +6151,7 @@ int ext4_ext_clear_bb(struct inode *inode)
                 cur = cur + map.m_len;
         }

+out:
+       ext4_free_ext_path(path);
         return 0;
  }

---------------------------------------------------------------------------

This can cause path leaks and path double free.

----------------------------------------

     path_A = ext4_find_extent
     while (cur < end) {
             path_B = ext4_find_extent(inode, map.m_lblk, NULL, 0);
             // path_A leak
             ext4_free_ext_path(path_B);
     }
out:
     ext4_free_ext_path(path_B); // path_B double free

----------------------------------------

I think it's best to drop Zetao's patch
   b2e662cb86ca ("ext4: remove redundant null pointer check")
and then reapply the conflicting patch
  92bbb922166c ("ext4: make some fast commit functions reuse extents path")

Or apply the following modifications to conflicting patch in the tree:

---------------------------------------------------------------------------

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 789316f22f97..34e25eee6521 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -6132,7 +6132,7 @@ int ext4_ext_clear_bb(struct inode *inode)
                 if (ret < 0)
                         break;
                 if (ret > 0) {
-                       path = ext4_find_extent(inode, map.m_lblk, NULL, 0);
+                       path = ext4_find_extent(inode, map.m_lblk, path, 0);
                         if (!IS_ERR(path)) {
                                 for (j = 0; j < path->p_depth; j++) {
ext4_mb_mark_bb(inode->i_sb,
@@ -6140,7 +6140,8 @@ int ext4_ext_clear_bb(struct inode *inode)
ext4_fc_record_regions(inode->i_sb, inode->i_ino,
                                                         0, 
path[j].p_block, 1, 1);
                                 }
-                               ext4_free_ext_path(path);
+                       } else {
+                               path = NULL;
                         }
                         ext4_mb_mark_bb(inode->i_sb, map.m_pblk, 
map.m_len, false);
                         ext4_fc_record_regions(inode->i_sb, inode->i_ino,

---------------------------------------------------------------------------


Cheers,
Baokun