From: Zhang Yi <[email protected]>
v2->v3:
- Fix the potential overflow on journal space check in patch 7.
- Rename ext4_get_journal_dev() to ext4_get_journal_blkdev() in patch 11.
v1->v2:
- Fix the changelog in patch 1 and 2.
- Simplify the comments for local functions in patch 6.
- Remove the incorrect zero fast_commit blocks check in patch 7.
- Fix a UAF problem in patch 11.
Hello,
This patch set cleanup the journal load and initialization process
(discussed and suggested by Ted in [1]). Firstly, move reading of the
journal superblock from jbd2_journal_load() and jbd2_journal_wipe()
early to journal_init_common(), and completely drop the kludgy call of
journal_get_superblock() in jbd2_journal_check_used_features(). Then
cleanup the ext4_get_journal() and ext4_get_dev_journal(), making their
initialization process and error handling process more clear, and return
proper errno if some bad happens. Finally rename those two functions to
jbd2_open_{dev,inode}_journal. This patch set has passed
'kvm-xfstests -g auto'.
[1] https://lore.kernel.org/linux-ext4/[email protected]/
Thanks,
Yi.
Zhang Yi (12):
jbd2: move load_superblock() dependent functions
jbd2: move load_superblock() into journal_init_common()
jbd2: don't load superblock in jbd2_journal_check_used_features()
jbd2: checking valid features early in journal_get_superblock()
jbd2: open code jbd2_verify_csum_type() helper
jbd2: cleanup load_superblock()
jbd2: add fast_commit space check
jbd2: cleanup journal_init_common()
jbd2: drop useless error tag in jbd2_journal_wipe()
jbd2: jbd2_journal_init_{dev,inode} return proper error return value
ext4: cleanup ext4_get_dev_journal() and ext4_get_journal()
ext4: ext4_get_{dev}_journal return proper error value
fs/ext4/super.c | 154 ++++++++-------
fs/jbd2/journal.c | 466 +++++++++++++++++++++------------------------
fs/ocfs2/journal.c | 8 +-
3 files changed, 300 insertions(+), 328 deletions(-)
--
2.34.3
From: Zhang Yi <[email protected]>
ext4_get_journal() and ext4_get_dev_journal() return NULL if they failed
to init journal, making them return proper error value instead, also
rename them to ext4_open_{inode,dev}_journal().
Signed-off-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/ext4/super.c | 51 +++++++++++++++++++++++++++++--------------------
1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index af44cc825d00..b0c764e8943a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5752,18 +5752,18 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
journal_inode = ext4_iget(sb, journal_inum, EXT4_IGET_SPECIAL);
if (IS_ERR(journal_inode)) {
ext4_msg(sb, KERN_ERR, "no journal found");
- return NULL;
+ return ERR_CAST(journal_inode);
}
if (!journal_inode->i_nlink) {
make_bad_inode(journal_inode);
iput(journal_inode);
ext4_msg(sb, KERN_ERR, "journal inode is deleted");
- return NULL;
+ return ERR_PTR(-EFSCORRUPTED);
}
if (!S_ISREG(journal_inode->i_mode) || IS_ENCRYPTED(journal_inode)) {
ext4_msg(sb, KERN_ERR, "invalid journal inode");
iput(journal_inode);
- return NULL;
+ return ERR_PTR(-EFSCORRUPTED);
}
ext4_debug("Journal inode found at %p: %lld bytes\n",
@@ -5793,21 +5793,21 @@ static int ext4_journal_bmap(journal_t *journal, sector_t *block)
return 0;
}
-static journal_t *ext4_get_journal(struct super_block *sb,
- unsigned int journal_inum)
+static journal_t *ext4_open_inode_journal(struct super_block *sb,
+ unsigned int journal_inum)
{
struct inode *journal_inode;
journal_t *journal;
journal_inode = ext4_get_journal_inode(sb, journal_inum);
- if (!journal_inode)
- return NULL;
+ if (IS_ERR(journal_inode))
+ return ERR_CAST(journal_inode);
journal = jbd2_journal_init_inode(journal_inode);
if (IS_ERR(journal)) {
ext4_msg(sb, KERN_ERR, "Could not load journal inode");
iput(journal_inode);
- return NULL;
+ return ERR_CAST(journal);
}
journal->j_private = sb;
journal->j_bmap = ext4_journal_bmap;
@@ -5825,6 +5825,7 @@ static struct block_device *ext4_get_journal_blkdev(struct super_block *sb,
ext4_fsblk_t sb_block;
unsigned long offset;
struct ext4_super_block *es;
+ int errno;
bdev = blkdev_get_by_dev(j_dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb,
&ext4_holder_ops);
@@ -5832,7 +5833,7 @@ static struct block_device *ext4_get_journal_blkdev(struct super_block *sb,
ext4_msg(sb, KERN_ERR,
"failed to open journal device unknown-block(%u,%u) %ld",
MAJOR(j_dev), MINOR(j_dev), PTR_ERR(bdev));
- return NULL;
+ return ERR_CAST(bdev);
}
blocksize = sb->s_blocksize;
@@ -5840,6 +5841,7 @@ static struct block_device *ext4_get_journal_blkdev(struct super_block *sb,
if (blocksize < hblock) {
ext4_msg(sb, KERN_ERR,
"blocksize too small for journal device");
+ errno = -EINVAL;
goto out_bdev;
}
@@ -5850,6 +5852,7 @@ static struct block_device *ext4_get_journal_blkdev(struct super_block *sb,
if (!bh) {
ext4_msg(sb, KERN_ERR, "couldn't read superblock of "
"external journal");
+ errno = -EINVAL;
goto out_bdev;
}
@@ -5858,6 +5861,7 @@ static struct block_device *ext4_get_journal_blkdev(struct super_block *sb,
!(le32_to_cpu(es->s_feature_incompat) &
EXT4_FEATURE_INCOMPAT_JOURNAL_DEV)) {
ext4_msg(sb, KERN_ERR, "external journal has bad superblock");
+ errno = -EFSCORRUPTED;
goto out_bh;
}
@@ -5865,11 +5869,13 @@ static struct block_device *ext4_get_journal_blkdev(struct super_block *sb,
EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
es->s_checksum != ext4_superblock_csum(sb, es)) {
ext4_msg(sb, KERN_ERR, "external journal has corrupt superblock");
+ errno = -EFSCORRUPTED;
goto out_bh;
}
if (memcmp(EXT4_SB(sb)->s_es->s_journal_uuid, es->s_uuid, 16)) {
ext4_msg(sb, KERN_ERR, "journal UUID does not match");
+ errno = -EFSCORRUPTED;
goto out_bh;
}
@@ -5882,31 +5888,34 @@ static struct block_device *ext4_get_journal_blkdev(struct super_block *sb,
brelse(bh);
out_bdev:
blkdev_put(bdev, sb);
- return NULL;
+ return ERR_PTR(errno);
}
-static journal_t *ext4_get_dev_journal(struct super_block *sb,
- dev_t j_dev)
+static journal_t *ext4_open_dev_journal(struct super_block *sb,
+ dev_t j_dev)
{
journal_t *journal;
ext4_fsblk_t j_start;
ext4_fsblk_t j_len;
struct block_device *journal_bdev;
+ int errno = 0;
journal_bdev = ext4_get_journal_blkdev(sb, j_dev, &j_start, &j_len);
- if (!journal_bdev)
- return NULL;
+ if (IS_ERR(journal_bdev))
+ return ERR_CAST(journal_bdev);
journal = jbd2_journal_init_dev(journal_bdev, sb->s_bdev, j_start,
j_len, sb->s_blocksize);
if (IS_ERR(journal)) {
ext4_msg(sb, KERN_ERR, "failed to create device journal");
+ errno = PTR_ERR(journal);
goto out_bdev;
}
if (be32_to_cpu(journal->j_superblock->s_nr_users) != 1) {
ext4_msg(sb, KERN_ERR, "External journal has more than one "
"user (unsupported) - %d",
be32_to_cpu(journal->j_superblock->s_nr_users));
+ errno = -EINVAL;
goto out_journal;
}
journal->j_private = sb;
@@ -5918,7 +5927,7 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
jbd2_journal_destroy(journal);
out_bdev:
blkdev_put(journal_bdev, sb);
- return NULL;
+ return ERR_PTR(errno);
}
static int ext4_load_journal(struct super_block *sb,
@@ -5950,13 +5959,13 @@ static int ext4_load_journal(struct super_block *sb,
}
if (journal_inum) {
- journal = ext4_get_journal(sb, journal_inum);
- if (!journal)
- return -EINVAL;
+ journal = ext4_open_inode_journal(sb, journal_inum);
+ if (IS_ERR(journal))
+ return PTR_ERR(journal);
} else {
- journal = ext4_get_dev_journal(sb, journal_dev);
- if (!journal)
- return -EINVAL;
+ journal = ext4_open_dev_journal(sb, journal_dev);
+ if (IS_ERR(journal))
+ return PTR_ERR(journal);
}
journal_dev_ro = bdev_read_only(journal->j_dev);
--
2.34.3
From: Zhang Yi <[email protected]>
Adjust the initialization sequence and error handle of journal_t, moving
load superblock to the begin, and classify others initialization.
Signed-off-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/jbd2/journal.c | 45 ++++++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 261483976924..708d58d69592 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1533,6 +1533,16 @@ static journal_t *journal_init_common(struct block_device *bdev,
if (!journal)
return NULL;
+ journal->j_blocksize = blocksize;
+ journal->j_dev = bdev;
+ journal->j_fs_dev = fs_dev;
+ journal->j_blk_offset = start;
+ journal->j_total_len = len;
+
+ err = journal_load_superblock(journal);
+ if (err)
+ goto err_cleanup;
+
init_waitqueue_head(&journal->j_wait_transaction_locked);
init_waitqueue_head(&journal->j_wait_done_commit);
init_waitqueue_head(&journal->j_wait_commit);
@@ -1544,12 +1554,15 @@ static journal_t *journal_init_common(struct block_device *bdev,
mutex_init(&journal->j_checkpoint_mutex);
spin_lock_init(&journal->j_revoke_lock);
spin_lock_init(&journal->j_list_lock);
+ spin_lock_init(&journal->j_history_lock);
rwlock_init(&journal->j_state_lock);
journal->j_commit_interval = (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE);
journal->j_min_batch_time = 0;
journal->j_max_batch_time = 15000; /* 15ms */
atomic_set(&journal->j_reserved_credits, 0);
+ lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
+ &jbd2_trans_commit_key, 0);
/* The journal is marked for error until we succeed with recovery! */
journal->j_flags = JBD2_ABORT;
@@ -1559,18 +1572,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
if (err)
goto err_cleanup;
- spin_lock_init(&journal->j_history_lock);
-
- lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
- &jbd2_trans_commit_key, 0);
-
- /* journal descriptor can store up to n blocks -bzzz */
- journal->j_blocksize = blocksize;
- journal->j_dev = bdev;
- journal->j_fs_dev = fs_dev;
- journal->j_blk_offset = start;
- journal->j_total_len = len;
- /* We need enough buffers to write out full descriptor block. */
+ /*
+ * journal descriptor can store up to n blocks, we need enough
+ * buffers to write out full descriptor block.
+ */
n = journal->j_blocksize / jbd2_min_tag_size();
journal->j_wbufsize = n;
journal->j_fc_wbuf = NULL;
@@ -1579,7 +1584,8 @@ static journal_t *journal_init_common(struct block_device *bdev,
if (!journal->j_wbuf)
goto err_cleanup;
- err = journal_load_superblock(journal);
+ err = percpu_counter_init(&journal->j_checkpoint_jh_count, 0,
+ GFP_KERNEL);
if (err)
goto err_cleanup;
@@ -1588,21 +1594,18 @@ static journal_t *journal_init_common(struct block_device *bdev,
journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
journal->j_shrinker.seeks = DEFAULT_SEEKS;
journal->j_shrinker.batch = journal->j_max_transaction_buffers;
-
- if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
+ err = register_shrinker(&journal->j_shrinker, "jbd2-journal:(%u:%u)",
+ MAJOR(bdev->bd_dev), MINOR(bdev->bd_dev));
+ if (err)
goto err_cleanup;
- if (register_shrinker(&journal->j_shrinker, "jbd2-journal:(%u:%u)",
- MAJOR(bdev->bd_dev), MINOR(bdev->bd_dev))) {
- percpu_counter_destroy(&journal->j_checkpoint_jh_count);
- goto err_cleanup;
- }
return journal;
err_cleanup:
- brelse(journal->j_sb_buffer);
+ percpu_counter_destroy(&journal->j_checkpoint_jh_count);
kfree(journal->j_wbuf);
jbd2_journal_destroy_revoke(journal);
+ journal_fail_superblock(journal);
kfree(journal);
return NULL;
}
--
2.34.3
From: Zhang Yi <[email protected]>
If JBD2_FEATURE_INCOMPAT_FAST_COMMIT bit is set, it means the journal
have fast commit records need to recover, so the fast commit size
should not be too large, and the leftover normal journal size should
never less than JBD2_MIN_JOURNAL_BLOCKS. If it happens, the
journal->j_last is likely to be wrong and will probably lead to
incorrect journal recovery. So add a check into the
journal_check_superblock(), and drop the pointless check when
initializing the fastcommit parameters.
Signed-off-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/jbd2/journal.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index a8d17070073b..261483976924 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1347,6 +1347,7 @@ static void journal_fail_superblock(journal_t *journal)
static int journal_check_superblock(journal_t *journal)
{
journal_superblock_t *sb = journal->j_superblock;
+ int num_fc_blks;
int err = -EINVAL;
if (sb->s_header.h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER) ||
@@ -1389,6 +1390,15 @@ static int journal_check_superblock(journal_t *journal)
return err;
}
+ num_fc_blks = jbd2_has_feature_fast_commit(journal) ?
+ jbd2_journal_get_num_fc_blks(sb) : 0;
+ if (be32_to_cpu(sb->s_maxlen) < JBD2_MIN_JOURNAL_BLOCKS ||
+ be32_to_cpu(sb->s_maxlen) - JBD2_MIN_JOURNAL_BLOCKS < num_fc_blks) {
+ printk(KERN_ERR "JBD2: journal file too short %u,%d\n",
+ be32_to_cpu(sb->s_maxlen), num_fc_blks);
+ return err;
+ }
+
if (jbd2_has_feature_csum2(journal) &&
jbd2_has_feature_csum3(journal)) {
/* Can't have checksum v2 and v3 at the same time! */
@@ -1454,7 +1464,6 @@ static int journal_load_superblock(journal_t *journal)
int err;
struct buffer_head *bh;
journal_superblock_t *sb;
- int num_fc_blocks;
bh = getblk_unmovable(journal->j_dev, journal->j_blk_offset,
journal->j_blocksize);
@@ -1492,9 +1501,8 @@ static int journal_load_superblock(journal_t *journal)
if (jbd2_has_feature_fast_commit(journal)) {
journal->j_fc_last = be32_to_cpu(sb->s_maxlen);
- num_fc_blocks = jbd2_journal_get_num_fc_blks(sb);
- if (journal->j_last - num_fc_blocks >= JBD2_MIN_JOURNAL_BLOCKS)
- journal->j_last = journal->j_fc_last - num_fc_blocks;
+ journal->j_last = journal->j_fc_last -
+ jbd2_journal_get_num_fc_blks(sb);
journal->j_fc_first = journal->j_last + 1;
journal->j_fc_off = 0;
}
--
2.34.3
From: Zhang Yi <[email protected]>
Current jbd2_journal_init_{dev,inode} return NULL if some error
happens, make them to pass out proper error return value.
Signed-off-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/ext4/super.c | 4 ++--
fs/jbd2/journal.c | 19 +++++++++----------
fs/ocfs2/journal.c | 8 ++++----
3 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c94ebf704616..ce2e02b139af 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5827,7 +5827,7 @@ static journal_t *ext4_get_journal(struct super_block *sb,
return NULL;
journal = jbd2_journal_init_inode(journal_inode);
- if (!journal) {
+ if (IS_ERR(journal)) {
ext4_msg(sb, KERN_ERR, "Could not load journal inode");
iput(journal_inode);
return NULL;
@@ -5906,7 +5906,7 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb,
journal = jbd2_journal_init_dev(bdev, sb->s_bdev,
start, len, blocksize);
- if (!journal) {
+ if (IS_ERR(journal)) {
ext4_msg(sb, KERN_ERR, "failed to create device journal");
goto out_bdev;
}
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 061887368ad5..f9bcac3d637e 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1531,7 +1531,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
journal = kzalloc(sizeof(*journal), GFP_KERNEL);
if (!journal)
- return NULL;
+ return ERR_PTR(-ENOMEM);
journal->j_blocksize = blocksize;
journal->j_dev = bdev;
@@ -1576,6 +1576,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
* journal descriptor can store up to n blocks, we need enough
* buffers to write out full descriptor block.
*/
+ err = -ENOMEM;
n = journal->j_blocksize / jbd2_min_tag_size();
journal->j_wbufsize = n;
journal->j_fc_wbuf = NULL;
@@ -1607,7 +1608,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
jbd2_journal_destroy_revoke(journal);
journal_fail_superblock(journal);
kfree(journal);
- return NULL;
+ return ERR_PTR(err);
}
/* jbd2_journal_init_dev and jbd2_journal_init_inode:
@@ -1640,8 +1641,8 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
journal_t *journal;
journal = journal_init_common(bdev, fs_dev, start, len, blocksize);
- if (!journal)
- return NULL;
+ if (IS_ERR(journal))
+ return ERR_CAST(journal);
snprintf(journal->j_devname, sizeof(journal->j_devname),
"%pg", journal->j_dev);
@@ -1667,11 +1668,9 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
blocknr = 0;
err = bmap(inode, &blocknr);
-
if (err || !blocknr) {
- pr_err("%s: Cannot locate journal superblock\n",
- __func__);
- return NULL;
+ pr_err("%s: Cannot locate journal superblock\n", __func__);
+ return err ? ERR_PTR(err) : ERR_PTR(-EINVAL);
}
jbd2_debug(1, "JBD2: inode %s/%ld, size %lld, bits %d, blksize %ld\n",
@@ -1681,8 +1680,8 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
journal = journal_init_common(inode->i_sb->s_bdev, inode->i_sb->s_bdev,
blocknr, inode->i_size >> inode->i_sb->s_blocksize_bits,
inode->i_sb->s_blocksize);
- if (!journal)
- return NULL;
+ if (IS_ERR(journal))
+ return ERR_CAST(journal);
journal->j_inode = inode;
snprintf(journal->j_devname, sizeof(journal->j_devname),
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 25d8072ccfce..f35a1bbf52e2 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -911,9 +911,9 @@ int ocfs2_journal_init(struct ocfs2_super *osb, int *dirty)
/* call the kernels journal init function now */
j_journal = jbd2_journal_init_inode(inode);
- if (j_journal == NULL) {
+ if (IS_ERR(j_journal)) {
mlog(ML_ERROR, "Linux journal layer error\n");
- status = -EINVAL;
+ status = PTR_ERR(journal);
goto done;
}
@@ -1687,9 +1687,9 @@ static int ocfs2_replay_journal(struct ocfs2_super *osb,
}
journal = jbd2_journal_init_inode(inode);
- if (journal == NULL) {
+ if (IS_ERR(journal)) {
mlog(ML_ERROR, "Linux journal layer error\n");
- status = -EIO;
+ status = PTR_ERR(journal);
goto done;
}
--
2.34.3