linux-ext4 - Re: [PATCH v5 2/2] ext4: clear the verified flag of the modified leaf or idx if error

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

Date:   Thu, 16 Feb 2023 15:25:23 +0800
From:   zhanchengbin <zhanchengbin1@...wei.com>
To:     Jan Kara <jack@...e.cz>
CC:     <tytso@....edu>, <jack@...e.com>, <linux-ext4@...r.kernel.org>,
        <yi.zhang@...wei.com>, <linfeilong@...wei.com>,
        <liuzhiqiang26@...wei.com>, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v5 2/2] ext4: clear the verified flag of the modified leaf
 or idx if error

The last patch did not take into account path[0].p_bh == NULL, so I
reworked the code.

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0f95e857089e..05585afae0db 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1750,13 +1750,19 @@ static int ext4_ext_correct_indexes(handle_t 
*handle, struct inode *inode,
                         break;
                 err = ext4_ext_get_access(handle, inode, path + k);
                 if (err)
-                       break;
+                       goto clean;
                 path[k].p_idx->ei_block = border;
                 err = ext4_ext_dirty(handle, inode, path + k);
                 if (err)
-                       break;
+                       goto clean;
         }
+       return 0;

+clean:
+       while (k++ < depth) {
+               /* k here will not be 0, so don't consider the case 
where path[0].p_bh is NULL */
+               clear_buffer_verified(path[k].p_bh);
+       }
         return err;
  }

@@ -2304,6 +2310,7 @@ static int ext4_ext_rm_idx(handle_t *handle, 
struct inode *inode,
  {
         int err;
         ext4_fsblk_t leaf;
+       int b_depth = depth;

         /* free index block */
         depth--;
@@ -2339,11 +2346,18 @@ static int ext4_ext_rm_idx(handle_t *handle, 
struct inode *inode,
                 path--;
                 err = ext4_ext_get_access(handle, inode, path);
                 if (err)
-                       break;
+                       goto clean;
                 path->p_idx->ei_block = (path+1)->p_idx->ei_block;
                 err = ext4_ext_dirty(handle, inode, path);
                 if (err)
-                       break;
+                       goto clean;
+       }
+       return 0;
+
+clean:
+       while (depth++ < b_depth - 1) {
+               /* depth here will not be 0, so don't consider the case 
where path[0].p_bh is NULL */
+               clear_buffer_verified(path[depth].p_bh);
         }
         return err;
  }


On 2023/2/14 20:52, Jan Kara wrote:
> 
> This would be more understandable as:
> 
> 	if (k >= 0)
> 		while (k++ < depth)
> 			...
> 
> Also the loop is IMO wrong because it will run with k == depth as well (due
> to post-increment) and that is not initialized. Furthermore it will run
> also if we exit the previous loop due to:
> 
>                  /* change all left-side indexes */
>                  if (path[k+1].p_idx != EXT_FIRST_INDEX(path[k+1].p_hdr))
>                          break;
> 
> which is unwanted as well. Which suggests that you didn't test your changes
> much (if at all...). So please make sure your changes are tested next time.
> Thank you!
> 
> 								Honza
I only ran xfstest locally. Do you have any better suggestions?

Thanks,
  - bin.
> 
>>   
>>   	return err;
>>   }
>> @@ -2304,6 +2306,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>>   {
>>   	int err;
>>   	ext4_fsblk_t leaf;
>> +	int b_depth = depth;