re: ext4: convert file system to meta_bg if needed during resizing

re: ext4: convert file system to meta_bg if needed during resizing

[Dan Carpenter]

Hello Theodore Ts'o,

The patch 1c6bd7173d66: "ext4: convert file system to meta_bg if 
needed during resizing" from Sep 13, 2012, leads to the following 
warning:
fs/ext4/resize.c:1829 ext4_convert_meta_bg()
	 error: potential NULL dereference 'ei'.

  1770  static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
  1771  {
  1772          handle_t *handle;
  1773          struct ext4_sb_info *sbi = EXT4_SB(sb);
  1774          struct ext4_super_block *es = sbi->s_es;
  1775          struct ext4_inode_info *ei = 0;
                                        ^^^^^^
Sparse is going to complain.  Not sure why Fengguang hasn't emailed you.

  1776          ext4_fsblk_t nr;
  1777          int i, ret, err = 0;
  1778          int credits = 1;
  1779  
  1780          ext4_msg(sb, KERN_INFO, "Converting file system to meta_bg");
  1781          if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RESIZE_INODE)) {
  1782                  if (es->s_reserved_gdt_blocks) {
  1783                          ext4_error(sb, "Unexpected non-zero "
  1784                                     "s_reserved_gdt_blocks");
  1785                          return -EPERM;
  1786                  }
  1787                  if (!inode) {
  1788                          ext4_error(sb, "Unexpected NULL resize_inode");
  1789                          return -EPERM;
  1790                  }
  1791                  ei = EXT4_I(inode);
                        ^^^^^^^^^^^^^^^^^^
We only set "ei" if EXT4_FEATURE_COMPAT_RESIZE_INODE.

  1792  
  1793                  /* Do a quick sanity check of the resize inode */
  1794                  if (inode->i_blocks != 1 << (inode->i_blkbits - 9))
  1795                          goto invalid_resize_inode;
  1796                  for (i = 0; i < EXT4_N_BLOCKS; i++) {
  1797                          if (i == EXT4_DIND_BLOCK) {
  1798                                  if (ei->i_data[i])
  1799                                          continue;
  1800                                  else
  1801                                          goto invalid_resize_inode;
  1802                          }
  1803                          if (ei->i_data[i])
  1804                                  goto invalid_resize_inode;
  1805                  }
  1806                  credits += 3;   /* block bitmap, bg descriptor, resize inode */
  1807          }
  1808  
  1809          handle = ext4_journal_start_sb(sb, credits);
  1810          if (IS_ERR(handle))
  1811                  return PTR_ERR(handle);
  1812  
  1813          err = ext4_journal_get_write_access(handle, sbi->s_sbh);
  1814          if (err)
  1815                  goto errout;
  1816  
  1817          EXT4_CLEAR_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RESIZE_INODE);
  1818          EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_META_BG);
  1819          sbi->s_es->s_first_meta_bg =
  1820                  cpu_to_le32(num_desc_blocks(sb, sbi->s_groups_count));
  1821  
  1822          err = ext4_handle_dirty_super(handle, sb);
  1823          if (err) {
  1824                  ext4_std_error(sb, err);
  1825                  goto errout;
  1826          }
  1827  
  1828          if (inode) {
  1829                  nr = le32_to_cpu(ei->i_data[EXT4_DIND_BLOCK]);
                                         ^^^^^^^^^^
Null deref.  Perhaps inode implies EXT4_FEATURE_COMPAT_RESIZE_INODE?

  1830                  ext4_free_blocks(handle, inode, NULL, nr, 1,
  1831                                   EXT4_FREE_BLOCKS_METADATA |
  1832                                   EXT4_FREE_BLOCKS_FORGET);
  1833                  ei->i_data[EXT4_DIND_BLOCK] = 0;
  1834                  inode->i_blocks = 0;
  1835  
  1836                  err = ext4_mark_inode_dirty(handle, inode);
  1837                  if (err)
  1838                          ext4_std_error(sb, err);
  1839          }

regards,
dan carpenter

Re: ext4: convert file system to meta_bg if needed during resizing

[Fengguang Wu]

On Tue, Sep 18, 2012 at 02:46:35PM +0300, Dan Carpenter wrote:
> Hello Theodore Ts'o,
> 
> The patch 1c6bd7173d66: "ext4: convert file system to meta_bg if 
> needed during resizing" from Sep 13, 2012, leads to the following 
> warning:
> fs/ext4/resize.c:1829 ext4_convert_meta_bg()
> 	 error: potential NULL dereference 'ei'.
> 
>   1770  static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
>   1771  {
>   1772          handle_t *handle;
>   1773          struct ext4_sb_info *sbi = EXT4_SB(sb);
>   1774          struct ext4_super_block *es = sbi->s_es;
>   1775          struct ext4_inode_info *ei = 0;
>                                         ^^^^^^
> Sparse is going to complain.  Not sure why Fengguang hasn't emailed you.

Sorry I'm doing lots of changes these days and they apparently break
many things.. I'd expect the build test system to gradually restore to
normal in the coming days.

Thanks,
Fengguang

Re: ext4: convert file system to meta_bg if needed during resizing

[Theodore Ts'o]

On Tue, Sep 18, 2012 at 02:46:35PM +0300, Dan Carpenter wrote:
>   1775          struct ext4_inode_info *ei = 0;
>                                         ^^^^^^
> Sparse is going to complain.  Not sure why Fengguang hasn't emailed you.

>   1791                  ei = EXT4_I(inode);
>                         ^^^^^^^^^^^^^^^^^^
> We only set "ei" if EXT4_FEATURE_COMPAT_RESIZE_INODE.
> ...

>   1829                  nr = le32_to_cpu(ei->i_data[EXT4_DIND_BLOCK]);
>                                          ^^^^^^^^^^
> Null deref.  Perhaps inode implies EXT4_FEATURE_COMPAT_RESIZE_INODE?

Inode does imply EXT4_FEATURE_COMPAT_RESIZE_INODE, but I should make
the code cleaner.

Thanks for pointing this out.

							- Ted

[PATCH] ext4: fix online resizing when the # of block groups is constant

[Theodore Ts'o]

Commit 1c6bd7173d66b3 introduced a regression where an online resize
operation which did not change the number of block groups would fail,
i.e:

	mke2fs -t /dev/vdc 60000
	mount /dev/vdc
	resize2fs /dev/vdc 60001

This was due to a bug in the logic regarding when to try converting
the filesystem to use meta_bg.

Also fix up a number of other minor issues with the online resizing
code: (a) Fix a sparse warning; (b) only check to make sure the device
is large enough once, instead of multiple times through the resize
loop.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/resize.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 3c9367b..ee985ca 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1772,23 +1772,18 @@ static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
 	handle_t *handle;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_super_block *es = sbi->s_es;
-	struct ext4_inode_info *ei = 0;
+	struct ext4_inode_info *ei = EXT4_I(inode);
 	ext4_fsblk_t nr;
 	int i, ret, err = 0;
 	int credits = 1;
 
 	ext4_msg(sb, KERN_INFO, "Converting file system to meta_bg");
-	if (EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_RESIZE_INODE)) {
+	if (inode) {
 		if (es->s_reserved_gdt_blocks) {
 			ext4_error(sb, "Unexpected non-zero "
 				   "s_reserved_gdt_blocks");
 			return -EPERM;
 		}
-		if (!inode) {
-			ext4_error(sb, "Unexpected NULL resize_inode");
-			return -EPERM;
-		}
-		ei = EXT4_I(inode);
 
 		/* Do a quick sanity check of the resize inode */
 		if (inode->i_blocks != 1 << (inode->i_blkbits - 9))
@@ -1873,12 +1868,19 @@ int ext4_resize_fs(struct super_block *sb, ext4_fsblk_t n_blocks_count)
 	int err = 0, flexbg_size = 1 << sbi->s_log_groups_per_flex;
 	int meta_bg;
 
+	/* See if the device is actually as big as what was requested */
+	bh = sb_bread(sb, n_blocks_count - 1);
+	if (!bh) {
+		ext4_warning(sb, "can't read last block, resize aborted");
+		return -ENOSPC;
+	}
+	brelse(bh);
+
 retry:
 	o_blocks_count = ext4_blocks_count(es);
 
-	if (test_opt(sb, DEBUG))
-		ext4_msg(sb, KERN_DEBUG, "resizing filesystem from %llu "
-		       "to %llu blocks", o_blocks_count, n_blocks_count);
+	ext4_msg(sb, KERN_INFO, "resizing filesystem from %llu "
+		 "to %llu blocks", o_blocks_count, n_blocks_count);
 
 	if (n_blocks_count < o_blocks_count) {
 		/* On-line shrinking not supported */
@@ -1922,7 +1924,7 @@ retry:
 		}
 	}
 
-	if ((!resize_inode && !meta_bg) || n_group == o_group) {
+	if ((!resize_inode && !meta_bg) || n_blocks_count == o_blocks_count) {
 		err = ext4_convert_meta_bg(sb, resize_inode);
 		if (err)
 			goto out;
@@ -1937,14 +1939,6 @@ retry:
 		}
 	}
 
-	/* See if the device is actually as big as what was requested */
-	bh = sb_bread(sb, n_blocks_count - 1);
-	if (!bh) {
-		ext4_warning(sb, "can't read last block, resize aborted");
-		return -ENOSPC;
-	}
-	brelse(bh);

re: ext4: convert file system to meta_bg if needed during resizing

[Colin Ian King]

Hi,

static analysis with Coverity has found an issue in function
ext4_convert_meta_bg() with the following commit

commit 1c6bd7173d66b3dfdefcedb38cabc1fb03997509
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu Sep 13 10:19:24 2012 -0400

    ext4: convert file system to meta_bg if needed during resizing

The analysis is as follows:

1898
1899 errout:
1900        ret = ext4_journal_stop(handle);
1901        if (!err)

Unused value (UNUSED_VALUE)assigned_value: Assigning value from ret to
err here, but that stored value is not used.

1902                err = ret;
1903        return ret;

I'm not entirely sure why !err is being checked and then err is being
re-assigned and then is not used.

Colin

Re: ext4: convert file system to meta_bg if needed during resizing

[Theodore Y. Ts'o]

On Sat, Feb 29, 2020 at 12:39:34AM +0000, Colin Ian King wrote:
> Hi,
> 
> static analysis with Coverity has found an issue in function
> ext4_convert_meta_bg() with the following commit
> 
> commit 1c6bd7173d66b3dfdefcedb38cabc1fb03997509
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Thu Sep 13 10:19:24 2012 -0400
> 
>     ext4: convert file system to meta_bg if needed during resizing
> 
> The analysis is as follows:
> 
> 1898
> 1899 errout:
> 1900        ret = ext4_journal_stop(handle);
> 1901        if (!err)
> 
> Unused value (UNUSED_VALUE)assigned_value: Assigning value from ret to
> err here, but that stored value is not used.
> 
> 1902                err = ret;
> 1903        return ret;

Line 1903 should be "return err".

Want to send a patch, or shall I just commit the fix?

     	       	      	       	      - Ted

Re: ext4: convert file system to meta_bg if needed during resizing

[Colin Ian King]

On 29/02/2020 17:42, Theodore Y. Ts'o wrote:
> On Sat, Feb 29, 2020 at 12:39:34AM +0000, Colin Ian King wrote:
>> Hi,
>>
>> static analysis with Coverity has found an issue in function
>> ext4_convert_meta_bg() with the following commit
>>
>> commit 1c6bd7173d66b3dfdefcedb38cabc1fb03997509
>> Author: Theodore Ts'o <tytso@mit.edu>
>> Date:   Thu Sep 13 10:19:24 2012 -0400
>>
>>     ext4: convert file system to meta_bg if needed during resizing
>>
>> The analysis is as follows:
>>
>> 1898
>> 1899 errout:
>> 1900        ret = ext4_journal_stop(handle);
>> 1901        if (!err)
>>
>> Unused value (UNUSED_VALUE)assigned_value: Assigning value from ret to
>> err here, but that stored value is not used.
>>
>> 1902                err = ret;
>> 1903        return ret;
> 
> Line 1903 should be "return err".
> 
> Want to send a patch, or shall I just commit the fix?
> 
>      	       	      	       	      - Ted
> 
I don't mind if you commit the fix, I wasn't sure what the appropriate
fix was, so I kudos to you Ted for that.

Colin