ext4 fast-commit fixes for 5.15-stable
cover ext4 fast-commit fixes for 5.15-stable
This series backports 6 commits with 'Cc stable' that had failed to be applied, and 4 related commits that made the backports much easier. Please apply this series to 5.15-stable.
I verified that this series does not cause any regressions with 'gce-xfstests -c ext4/fast_commit -g auto'. There is one test failure both before and after (ext4/050).
Eric Biggers (5):
ext4: disable fast-commit of encrypted dir operations
ext4: don't set up encryption key during jbd2 transaction
ext4: add missing validation of fast-commit record lengths
ext4: fix unaligned memory access in ext4_fc_reserve_space()
ext4: fix off-by-one errors in fast-commit block filling
Jan Kara (1):
ext4: use ext4_debug() instead of jbd_debug()
Ritesh Harjani (1):
ext4: remove unused enum EXT4_FC_COMMIT_FAILED
Ye Bin (3):
ext4: introduce EXT4_FC_TAG_BASE_LEN helper
ext4: factor out ext4_fc_get_tl()
ext4: fix potential out of bound read in ext4_fc_replay_scan()
fs/ext4/balloc.c | 2 +- fs/ext4/ext4.h | 4 +- fs/ext4/ext4_jbd2.c | 3 +- fs/ext4/fast_commit.c | 284 +++++++++++++++++++++--------------- fs/ext4/fast_commit.h | 7 +- fs/ext4/indirect.c | 4 +- fs/ext4/inode.c | 2 +- fs/ext4/namei.c | 44 +++--- fs/ext4/orphan.c | 24 +-- fs/ext4/super.c | 2 +- include/trace/events/ext4.h | 7 +- 11 files changed, 222 insertions(+), 161 deletions(-)
Signature · 1 line
2.39.0
PATCH 1/10 ext4: remove unused enum EXT4_FC_COMMIT_FAILED
From: Ritesh Harjani <[email protected]>
commit c864ccd182d6ff2730a0f5b636c6b7c48f6f4f7f upstream.
Below commit removed all references of EXT4_FC_COMMIT_FAILED. commit 0915e464cb274 ("ext4: simplify updating of fast commit stats")
Just remove it since it is not used anymore.
Signed-off-by Ritesh Harjani <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Harshad Shirwadkar <[email protected]>
Link https://lore.kernel.org/r/c941357e476be07a1138c7319ca5faab7fb80fc6.1647057583.git.riteshh@linux.ibm.com open here
Signed-off-by Theodore Ts'o <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
--- fs/ext4/fast_commit.h | 1 - 1 file changed, 1 deletion(-)
fs/ext4/fast_commit.h +0 -1
@@ -93,7 +93,6 @@ enum {EXT4_FC_REASON_RENAME_DIR ,EXT4_FC_REASON_FALLOC_RANGE ,EXT4_FC_REASON_INODE_JOURNAL_DATA , -EXT4_FC_COMMIT_FAILED , EXT4_FC_REASON_MAX };
Signature · 1 line
2.39.0
PATCH 2/10 ext4: use ext4_debug() instead of jbd_debug()
In reply to Eric BiggersFrom: Jan Kara <[email protected]>
commit 4978c659e7b5c1926cdb4b556e4ca1fd2de8ad42 upstream.
We use jbd_debug() in some places in ext4. It seems a bit strange to use jbd2 debugging output function for ext4 code. Also these days ext4_debug() uses dynamic printk so each debug message can be enabled / disabled on its own so the time when it made some sense to have these combined (to allow easier common selecting of messages to report) has passed. Just convert all jbd_debug() uses in ext4 to ext4_debug().
Signed-off-by Jan Kara <[email protected]>
Reviewed-by Lukas Czerner <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
--- fs/ext4/balloc.c | 2 +- fs/ext4/ext4_jbd2.c | 3 +-- fs/ext4/fast_commit.c | 44 +++++++++++++++++++++---------------------- fs/ext4/indirect.c | 4 ++-- fs/ext4/inode.c | 2 +- fs/ext4/orphan.c | 24 +++++++++++------------ fs/ext4/super.c | 2 +- 7 files changed, 40 insertions(+), 41 deletions(-)
fs/ext4/balloc.c +1 -1
@@ -665,7 +665,7 @@ int ext4_should_retry_alloc(struct super_block *sb, int *retries)* it's possible we've just missed a transaction commit here, * so ignore the returned status */ -jbd_debug (1 ,"%s: retrying operation after ENOSPC\n" , sb->s_id); +ext4_debug ("%s: retrying operation after ENOSPC\n" , sb->s_id); (void )jbd2_journal_force_commit_nested (sbi->s_journal);return 1 ; }
fs/ext4/ext4_jbd2.c +1 -2
@@ -267,8 +267,7 @@ int __ext4_forget(const char *where, unsigned int line, handle_t *handle,trace_ext4_forget (inode, is_metadata, blocknr);BUFFER_TRACE (bh,"enter" ); -jbd_debug (4 ,"forgetting bh %p: is_metadata = %d, mode %o, " -"data mode %x\n" , +ext4_debug ("forgetting bh %p: is_metadata=%d, mode %o, data mode %x\n" , bh, is_metadata, inode->i_mode,test_opt (inode->i_sb,DATA_FLAGS ));
fs/ext4/fast_commit.c +22 -22
170 lines not on this page · raw message
fs/ext4/indirect.c +2 -2
@@ -467,7 +467,7 @@ static int ext4_splice_branch(handle_t *handle,* the new i_size. But that is not done here - it is done in * generic_commit_write->__mark_inode_dirty->ext4_dirty_inode. */ -jbd_debug (5 ,"splicing indirect only\n" ); +ext4_debug ("splicing indirect only\n" );BUFFER_TRACE (where->bh,"call ext4_handle_dirty_metadata" ); err =ext4_handle_dirty_metadata (handle, ar->inode, where->bh);if (err)
@@ -479,7 +479,7 @@ static int ext4_splice_branch(handle_t *handle, err =ext4_mark_inode_dirty (handle, ar->inode);if (unlikely (err)) goto err_out; -jbd_debug (5 ,"splicing direct\n" ); +ext4_debug ("splicing direct\n" ); }return err;
fs/ext4/inode.c +1 -1
@@ -5220,7 +5220,7 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc)if (EXT4_SB (inode->i_sb)->s_journal) {if (ext4_journal_current_handle ()) { -jbd_debug (1 ,"called recursively, non-PF_MEMALLOC!\n" ); +ext4_debug ("called recursively, non-PF_MEMALLOC!\n" );dump_stack ();return -EIO ; }
fs/ext4/orphan.c +12 -12
@@ -181,8 +181,8 @@ int ext4_orphan_add(handle_t *handle, struct inode *inode) }else brelse (iloc.bh); -jbd_debug (4 ,"superblock will point to %lu\n" , inode->i_ino); -jbd_debug (4 ,"orphan inode %lu will point to %d\n" , +ext4_debug ("superblock will point to %lu\n" , inode->i_ino); +ext4_debug ("orphan inode %lu will point to %d\n" , inode->i_ino,NEXT_ORPHAN (inode)); out:ext4_std_error (sb, err);
@@ -251,7 +251,7 @@ int ext4_orphan_del(handle_t *handle, struct inode *inode) }mutex_lock (&sbi->s_orphan_lock); -jbd_debug (4 ,"remove inode %lu from orphan list\n" , inode->i_ino); +ext4_debug ("remove inode %lu from orphan list\n" , inode->i_ino); prev = ei->i_orphan.prev;list_del_init (&ei->i_orphan);
@@ -267,7 +267,7 @@ int ext4_orphan_del(handle_t *handle, struct inode *inode) ino_next =NEXT_ORPHAN (inode);if (prev == &sbi->s_orphan) { -jbd_debug (4 ,"superblock will point to %u\n" , ino_next); +ext4_debug ("superblock will point to %u\n" , ino_next);BUFFER_TRACE (sbi->s_sbh,"get_write_access" ); err =ext4_journal_get_write_access (handle, inode->i_sb, sbi->s_sbh,EXT4_JTR_NONE );
@@ -286,7 +286,7 @@ int ext4_orphan_del(handle_t *handle, struct inode *inode)struct inode *i_prev = &list_entry(prev, struct ext4_inode_info, i_orphan)->vfs_inode; -jbd_debug (4 ,"orphan inode %lu will point to %u\n" , +ext4_debug ("orphan inode %lu will point to %u\n" , i_prev->i_ino, ino_next); err =ext4_reserve_inode_write (handle, i_prev, &iloc2);if (err) {
@@ -332,8 +332,8 @@ static void ext4_process_orphan(struct inode *inode,ext4_msg (sb,KERN_DEBUG ,"%s: truncating inode %lu to %lld bytes" , __func__, inode->i_ino, inode->i_size); -jbd_debug (2 ,"truncating inode %lu to %lld bytes\n" , - inode->i_ino, inode->i_size); +ext4_debug ("truncating inode %lu to %lld bytes\n" , + inode->i_ino, inode->i_size);inode_lock (inode);truncate_inode_pages (inode->i_mapping, inode->i_size); ret =ext4_truncate (inode);
@@ -353,8 +353,8 @@ static void ext4_process_orphan(struct inode *inode,ext4_msg (sb,KERN_DEBUG ,"%s: deleting unreferenced inode %lu" , __func__, inode->i_ino); -jbd_debug (2 ,"deleting unreferenced inode %lu\n" , - inode->i_ino); +ext4_debug ("deleting unreferenced inode %lu\n" , + inode->i_ino); (*nr_orphans)++; }iput (inode);/* The delete magic happens here! */
@@ -391,7 +391,7 @@ void ext4_orphan_cleanup(struct super_block *sb, struct ext4_super_block *es)int inodes_per_ob =ext4_inodes_per_orphan_block (sb);if (!es->s_last_orphan && !oi->of_blocks) { -jbd_debug (4 ,"no orphan inodes to clean up\n" ); +ext4_debug ("no orphan inodes to clean up\n" );return ; }
@@ -415,7 +415,7 @@ void ext4_orphan_cleanup(struct super_block *sb, struct ext4_super_block *es) "clearing orphan list."); es->s_last_orphan =0 ; } -jbd_debug (1 ,"Skipping orphan recovery on fs with errors.\n" ); +ext4_debug ("Skipping orphan recovery on fs with errors.\n" );return ; }
@@ -459,7 +459,7 @@ void ext4_orphan_cleanup(struct super_block *sb, struct ext4_super_block *es)* so, skip the rest. */ if (EXT4_SB (sb)->s_mount_state &EXT4_ERROR_FS ) { -jbd_debug (1 ,"Skipping orphan recovery on fs with errors.\n" ); +ext4_debug ("Skipping orphan recovery on fs with errors.\n" ); es->s_last_orphan =0 ;break ; }
fs/ext4/super.c +1 -1
@@ -5154,7 +5154,7 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,return NULL ; } -jbd_debug (2 ,"Journal inode found at %p: %lld bytes\n" , +ext4_debug ("Journal inode found at %p: %lld bytes\n" , journal_inode, journal_inode->i_size);if (!S_ISREG (journal_inode->i_mode)) {ext4_msg (sb,KERN_ERR ,"invalid journal inode" );
Signature · 1 line
2.39.0
PATCH 3/10 ext4: introduce EXT4_FC_TAG_BASE_LEN helper
In reply to Eric BiggersFrom: Ye Bin <[email protected]>
commit fdc2a3c75dd8345c5b48718af90bad1a7811bedb upstream.
Introduce EXT4_FC_TAG_BASE_LEN helper for calculate length of struct ext4_fc_tl.
Signed-off-by Ye Bin <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
--- fs/ext4/fast_commit.c | 54 ++++++++++++++++++++++--------------------- fs/ext4/fast_commit.h | 3 +++ 2 files changed, 31 insertions(+), 26 deletions(-)
fs/ext4/fast_commit.c +28 -26
@@ -631,10 +631,10 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)* After allocating len, we should have space at least for a 0 byte * padding. */ -if (len +sizeof (struct ext4_fc_tl ) > bsize) +if (len +EXT4_FC_TAG_BASE_LEN > bsize)return NULL ; -if (bsize - off -1 > len +sizeof (struct ext4_fc_tl )) { +if (bsize - off -1 > len +EXT4_FC_TAG_BASE_LEN ) {/* * Only allocate from current buffer if we have enough space for * this request AND we have space to add a zero byte padding.
@@ -651,10 +651,10 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)/* Need to add PAD tag */ tl = (struct ext4_fc_tl *)(sbi->s_fc_bh->b_data + off); tl->fc_tag =cpu_to_le16 (EXT4_FC_TAG_PAD ); - pad_len = bsize - off -1 -sizeof (struct ext4_fc_tl ); + pad_len = bsize - off -1 -EXT4_FC_TAG_BASE_LEN ; tl->fc_len =cpu_to_le16 (pad_len);if (crc) - *crc =ext4_chksum (sbi, *crc, tl,sizeof (*tl)); + *crc =ext4_chksum (sbi, *crc, tl,EXT4_FC_TAG_BASE_LEN );if (pad_len >0 )ext4_fc_memzero (sb, tl +1 , pad_len, crc);/* Don't leak uninitialized memory in the unused last byte. */
@@ -699,7 +699,7 @@ static int ext4_fc_write_tail(struct super_block *sb, u32 crc)* ext4_fc_reserve_space takes care of allocating an extra block if * there's no enough space on this block for accommodating this tail. */ - dst =ext4_fc_reserve_space (sb,sizeof (tl) +sizeof (tail), &crc); + dst =ext4_fc_reserve_space (sb,EXT4_FC_TAG_BASE_LEN +sizeof (tail), &crc);if (!dst)return -ENOSPC ;
@@ -709,8 +709,8 @@ static int ext4_fc_write_tail(struct super_block *sb, u32 crc) tl.fc_len =cpu_to_le16 (bsize - off -1 +sizeof (struct ext4_fc_tail )); sbi->s_fc_bytes =round_up (sbi->s_fc_bytes, bsize); -ext4_fc_memcpy (sb, dst, &tl,sizeof (tl), &crc); - dst +=sizeof (tl); +ext4_fc_memcpy (sb, dst, &tl,EXT4_FC_TAG_BASE_LEN , &crc); + dst +=EXT4_FC_TAG_BASE_LEN ; tail.fc_tid =cpu_to_le32 (sbi->s_journal->j_running_transaction->t_tid);ext4_fc_memcpy (sb, dst, &tail.fc_tid,sizeof (tail.fc_tid), &crc); dst +=sizeof (tail.fc_tid);
@@ -734,15 +734,15 @@ static bool ext4_fc_add_tlv(struct super_block *sb, u16 tag, u16 len, u8 *val,struct ext4_fc_tl tl;u8 *dst; - dst =ext4_fc_reserve_space (sb,sizeof (tl) + len, crc); + dst =ext4_fc_reserve_space (sb,EXT4_FC_TAG_BASE_LEN + len, crc);if (!dst)return false; tl.fc_tag =cpu_to_le16 (tag); tl.fc_len =cpu_to_le16 (len); -ext4_fc_memcpy (sb, dst, &tl,sizeof (tl), crc); -ext4_fc_memcpy (sb, dst +sizeof (tl), val, len, crc); +ext4_fc_memcpy (sb, dst, &tl,EXT4_FC_TAG_BASE_LEN , crc); +ext4_fc_memcpy (sb, dst +EXT4_FC_TAG_BASE_LEN , val, len, crc);return true; }
@@ -754,8 +754,8 @@ static bool ext4_fc_add_dentry_tlv(struct super_block *sb, u32 *crc,struct ext4_fc_dentry_info fcd;struct ext4_fc_tl tl;int dlen = fc_dentry->fcd_name.len; -u8 *dst =ext4_fc_reserve_space (sb,sizeof (tl) +sizeof (fcd) + dlen, - crc); +u8 *dst =ext4_fc_reserve_space (sb, +EXT4_FC_TAG_BASE_LEN +sizeof (fcd) + dlen, crc);if (!dst)return false;
@@ -764,8 +764,8 @@ static bool ext4_fc_add_dentry_tlv(struct super_block *sb, u32 *crc, fcd.fc_ino =cpu_to_le32 (fc_dentry->fcd_ino); tl.fc_tag =cpu_to_le16 (fc_dentry->fcd_op); tl.fc_len =cpu_to_le16 (sizeof (fcd) + dlen); -ext4_fc_memcpy (sb, dst, &tl,sizeof (tl), crc); - dst +=sizeof (tl); +ext4_fc_memcpy (sb, dst, &tl,EXT4_FC_TAG_BASE_LEN , crc); + dst +=EXT4_FC_TAG_BASE_LEN ;ext4_fc_memcpy (sb, dst, &fcd,sizeof (fcd), crc); dst +=sizeof (fcd);ext4_fc_memcpy (sb, dst, fc_dentry->fcd_name.name, dlen, crc);
@@ -801,13 +801,13 @@ static int ext4_fc_write_inode(struct inode *inode, u32 *crc) ret = -ECANCELED ; dst =ext4_fc_reserve_space (inode->i_sb, -sizeof (tl) + inode_len +sizeof (fc_inode.fc_ino), crc); +EXT4_FC_TAG_BASE_LEN + inode_len +sizeof (fc_inode.fc_ino), crc);if (!dst) goto err; -if (!ext4_fc_memcpy (inode->i_sb, dst, &tl,sizeof (tl), crc)) +if (!ext4_fc_memcpy (inode->i_sb, dst, &tl,EXT4_FC_TAG_BASE_LEN , crc)) goto err; - dst +=sizeof (tl); + dst +=EXT4_FC_TAG_BASE_LEN ;if (!ext4_fc_memcpy (inode->i_sb, dst, &fc_inode,sizeof (fc_inode), crc)) goto err; dst +=sizeof (fc_inode);
@@ -1957,9 +1957,10 @@ static int ext4_fc_replay_scan(journal_t *journal, } state->fc_replay_expected_off++; -for (cur = start; cur < end; cur = cur +sizeof (tl) +le16_to_cpu (tl.fc_len)) { -memcpy (&tl, cur,sizeof (tl)); - val = cur +sizeof (tl); +for (cur = start; cur < end; + cur = cur +EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len)) { +memcpy (&tl, cur,EXT4_FC_TAG_BASE_LEN ); + val = cur +EXT4_FC_TAG_BASE_LEN ;ext4_debug ("Scan phase, tag:%s, blk %lld\n" ,tag2str (le16_to_cpu (tl.fc_tag)), bh->b_blocknr);switch (le16_to_cpu (tl.fc_tag)) {
@@ -1982,13 +1983,13 @@ static int ext4_fc_replay_scan(journal_t *journal,case EXT4_FC_TAG_PAD : state->fc_cur_tag++; state->fc_crc =ext4_chksum (sbi, state->fc_crc, cur, -sizeof (tl) +le16_to_cpu (tl.fc_len)); +EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len));break ;case EXT4_FC_TAG_TAIL : state->fc_cur_tag++;memcpy (&tail, val,sizeof (tail)); state->fc_crc =ext4_chksum (sbi, state->fc_crc, cur, -sizeof (tl) + +EXT4_FC_TAG_BASE_LEN + offsetof(struct ext4_fc_tail , fc_crc)); if (le32_to_cpu(tail.fc_tid) == expected_tid &&
@@ -2015,7 +2016,7 @@ static int ext4_fc_replay_scan(journal_t *journal, } state->fc_cur_tag++; state->fc_crc =ext4_chksum (sbi, state->fc_crc, cur, -sizeof (tl) +le16_to_cpu (tl.fc_len)); +EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len));break ;default : ret = state->fc_replay_num_tags ?
@@ -2070,9 +2071,10 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh, start = (u8 *)bh->b_data; end = (__u8 *)bh->b_data + journal->j_blocksize -1 ; -for (cur = start; cur < end; cur = cur +sizeof (tl) +le16_to_cpu (tl.fc_len)) { -memcpy (&tl, cur,sizeof (tl)); - val = cur +sizeof (tl); +for (cur = start; cur < end; + cur = cur +EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len)) { +memcpy (&tl, cur,EXT4_FC_TAG_BASE_LEN ); + val = cur +EXT4_FC_TAG_BASE_LEN ;if (state->fc_replay_num_tags ==0 ) { ret =JBD2_FC_REPLAY_STOP ;
fs/ext4/fast_commit.h +3 -0
@@ -70,6 +70,9 @@ struct ext4_fc_tail {__le32 fc_crc; }; +/* Tag base length */ +#define EXT4_FC_TAG_BASE_LEN (sizeof(struct ext4_fc_tl)) +/* * Fast commit status codes */
Signature · 1 line
2.39.0
PATCH 4/10 ext4: factor out ext4_fc_get_tl()
In reply to Eric BiggersFrom: Ye Bin <[email protected]>
commit dcc5827484d6e53ccda12334f8bbfafcc593ceda upstream.
Factor out ext4_fc_get_tl() to fill 'tl' with host byte order.
Signed-off-by Ye Bin <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
--- fs/ext4/fast_commit.c | 46 +++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 21 deletions(-)
fs/ext4/fast_commit.c +25 -21
@@ -1271,7 +1271,7 @@ struct dentry_info_args { };static inline void tl_to_darg (struct dentry_info_args *darg, -struct ext4_fc_tl *tl,u8 *val) +struct ext4_fc_tl *tl,u8 *val) {struct ext4_fc_dentry_info fcd;
@@ -1280,8 +1280,14 @@ static inline void tl_to_darg(struct dentry_info_args *darg, darg->parent_ino =le32_to_cpu (fcd.fc_parent_ino); darg->ino =le32_to_cpu (fcd.fc_ino); darg->dname = val + offsetof(struct ext4_fc_dentry_info , fc_dname); - darg->dname_len =le16_to_cpu (tl->fc_len) - -sizeof (struct ext4_fc_dentry_info ); + darg->dname_len = tl->fc_len -sizeof (struct ext4_fc_dentry_info ); +} + +static inline void ext4_fc_get_tl (struct ext4_fc_tl *tl,u8 *val) +{ +memcpy (tl, val,EXT4_FC_TAG_BASE_LEN ); + tl->fc_len =le16_to_cpu (tl->fc_len); + tl->fc_tag =le16_to_cpu (tl->fc_tag); }/* Unlink replay function */
@@ -1446,7 +1452,7 @@ static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl,struct ext4_inode *raw_fc_inode;struct inode *inode =NULL ;struct ext4_iloc iloc; -int inode_len, ino, ret, tag =le16_to_cpu (tl->fc_tag); +int inode_len, ino, ret, tag = tl->fc_tag;struct ext4_extent_header *eh;memcpy (&fc_inode, val,sizeof (fc_inode));
@@ -1471,7 +1477,7 @@ static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl,if (ret) goto out; - inode_len =le16_to_cpu (tl->fc_len) -sizeof (struct ext4_fc_inode ); + inode_len = tl->fc_len -sizeof (struct ext4_fc_inode ); raw_inode =ext4_raw_inode (&iloc);memcpy (raw_inode, raw_fc_inode, offsetof(struct ext4_inode , i_block));
@@ -1958,12 +1964,12 @@ static int ext4_fc_replay_scan(journal_t *journal, state->fc_replay_expected_off++;for (cur = start; cur < end; - cur = cur +EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len)) { -memcpy (&tl, cur,EXT4_FC_TAG_BASE_LEN ); + cur = cur +EXT4_FC_TAG_BASE_LEN + tl.fc_len) { +ext4_fc_get_tl (&tl, cur); val = cur +EXT4_FC_TAG_BASE_LEN ;ext4_debug ("Scan phase, tag:%s, blk %lld\n" , -tag2str (le16_to_cpu (tl.fc_tag)), bh->b_blocknr); -switch (le16_to_cpu (tl.fc_tag)) { +tag2str (tl.fc_tag), bh->b_blocknr); +switch (tl.fc_tag) {case EXT4_FC_TAG_ADD_RANGE :memcpy (&ext, val,sizeof (ext)); ex = (struct ext4_extent *)&ext.fc_ex;
@@ -1983,7 +1989,7 @@ static int ext4_fc_replay_scan(journal_t *journal,case EXT4_FC_TAG_PAD : state->fc_cur_tag++; state->fc_crc =ext4_chksum (sbi, state->fc_crc, cur, -EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len)); +EXT4_FC_TAG_BASE_LEN + tl.fc_len);break ;case EXT4_FC_TAG_TAIL : state->fc_cur_tag++;
@@ -2016,7 +2022,7 @@ static int ext4_fc_replay_scan(journal_t *journal, } state->fc_cur_tag++; state->fc_crc =ext4_chksum (sbi, state->fc_crc, cur, -EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len)); +EXT4_FC_TAG_BASE_LEN + tl.fc_len);break ;default : ret = state->fc_replay_num_tags ?
@@ -2072,8 +2078,8 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh, end = (__u8 *)bh->b_data + journal->j_blocksize -1 ;for (cur = start; cur < end; - cur = cur +EXT4_FC_TAG_BASE_LEN +le16_to_cpu (tl.fc_len)) { -memcpy (&tl, cur,EXT4_FC_TAG_BASE_LEN ); + cur = cur +EXT4_FC_TAG_BASE_LEN + tl.fc_len) { +ext4_fc_get_tl (&tl, cur); val = cur +EXT4_FC_TAG_BASE_LEN ;if (state->fc_replay_num_tags ==0 ) {
@@ -2081,10 +2087,9 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,ext4_fc_set_bitmaps_and_counters (sb);break ; } -ext4_debug ("Replay phase, tag:%s\n" , -tag2str (le16_to_cpu (tl.fc_tag))); +ext4_debug ("Replay phase, tag:%s\n" ,tag2str (tl.fc_tag)); state->fc_replay_num_tags--; -switch (le16_to_cpu (tl.fc_tag)) { +switch (tl.fc_tag) {case EXT4_FC_TAG_LINK : ret =ext4_fc_replay_link (sb, &tl, val);break ;
@@ -2105,19 +2110,18 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,break ;case EXT4_FC_TAG_PAD :trace_ext4_fc_replay (sb,EXT4_FC_TAG_PAD ,0 , -le16_to_cpu (tl.fc_len),0 ); + tl.fc_len,0 );break ;case EXT4_FC_TAG_TAIL : -trace_ext4_fc_replay (sb,EXT4_FC_TAG_TAIL ,0 , -le16_to_cpu (tl.fc_len),0 ); +trace_ext4_fc_replay (sb,EXT4_FC_TAG_TAIL , +0 , tl.fc_len,0 );memcpy (&tail, val,sizeof (tail));WARN_ON (le32_to_cpu (tail.fc_tid) != expected_tid);break ;case EXT4_FC_TAG_HEAD :break ;default : -trace_ext4_fc_replay (sb,le16_to_cpu (tl.fc_tag),0 , -le16_to_cpu (tl.fc_len),0 ); +trace_ext4_fc_replay (sb, tl.fc_tag,0 , tl.fc_len,0 ); ret = -ECANCELED ;break ; }
Signature · 1 line
2.39.0
PATCH 5/10 ext4: fix potential out of bound read in ext4_fc_replay_scan()
In reply to Eric BiggersFrom: Ye Bin <[email protected]>
commit 1b45cc5c7b920fd8bf72e5a888ec7abeadf41e09 upstream.
For scan loop must ensure that at least EXT4_FC_TAG_BASE_LEN space. If remain space less than EXT4_FC_TAG_BASE_LEN which will lead to out of bound read when mounting corrupt file system image.
ADD_RANGE/HEAD/TAIL is needed to add extra check when do journal scan, as this three tags will read data during scan, tag length couldn't less than data length which will read.
Signed-off-by Ye Bin <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
--- fs/ext4/fast_commit.c | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-)
fs/ext4/fast_commit.c +36 -2
@@ -1907,6 +1907,34 @@ void ext4_fc_replay_cleanup(struct super_block *sb)kfree (sbi->s_fc_replay_state.fc_modified_inodes); } +static inline bool ext4_fc_tag_len_isvalid (struct ext4_fc_tl *tl, +u8 *val,u8 *end) +{ +if (val + tl->fc_len > end) +return false; + +/* Here only check ADD_RANGE/TAIL/HEAD which will read data when do +* journal rescan before do CRC check. Other tags length check will +* rely on CRC check. +*/ +switch (tl->fc_tag) { +case EXT4_FC_TAG_ADD_RANGE : +return (sizeof (struct ext4_fc_add_range ) == tl->fc_len); +case EXT4_FC_TAG_TAIL : +return (sizeof (struct ext4_fc_tail ) <= tl->fc_len); +case EXT4_FC_TAG_HEAD : +return (sizeof (struct ext4_fc_head ) == tl->fc_len); +case EXT4_FC_TAG_DEL_RANGE : +case EXT4_FC_TAG_LINK : +case EXT4_FC_TAG_UNLINK : +case EXT4_FC_TAG_CREAT : +case EXT4_FC_TAG_INODE : +case EXT4_FC_TAG_PAD : +default : +return true; + } +} +/* * Recovery Scan phase handler *
@@ -1963,10 +1991,15 @@ static int ext4_fc_replay_scan(journal_t *journal, } state->fc_replay_expected_off++; -for (cur = start; cur < end; +for (cur = start; cur < end -EXT4_FC_TAG_BASE_LEN ; cur = cur +EXT4_FC_TAG_BASE_LEN + tl.fc_len) {ext4_fc_get_tl (&tl, cur); val = cur +EXT4_FC_TAG_BASE_LEN ; +if (!ext4_fc_tag_len_isvalid (&tl, val, end)) { + ret = state->fc_replay_num_tags ? +JBD2_FC_REPLAY_STOP : -ECANCELED ; + goto out_err; + }ext4_debug ("Scan phase, tag:%s, blk %lld\n" ,tag2str (tl.fc_tag), bh->b_blocknr);switch (tl.fc_tag) {
@@ -2077,7 +2110,7 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh, start = (u8 *)bh->b_data; end = (__u8 *)bh->b_data + journal->j_blocksize -1 ; -for (cur = start; cur < end; +for (cur = start; cur < end -EXT4_FC_TAG_BASE_LEN ; cur = cur +EXT4_FC_TAG_BASE_LEN + tl.fc_len) {ext4_fc_get_tl (&tl, cur); val = cur +EXT4_FC_TAG_BASE_LEN ;
@@ -2087,6 +2120,7 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,ext4_fc_set_bitmaps_and_counters (sb);break ; } +ext4_debug ("Replay phase, tag:%s\n" ,tag2str (tl.fc_tag)); state->fc_replay_num_tags--;switch (tl.fc_tag) {
Signature · 1 line
2.39.0
PATCH 6/10 ext4: disable fast-commit of encrypted dir operations
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
commit 0fbcb5251fc81b58969b272c4fb7374a7b922e3e upstream.
fast-commit of create, link, and unlink operations in encrypted directories is completely broken because the unencrypted filenames are being written to the fast-commit journal instead of the encrypted filenames. These operations can't be replayed, as encryption keys aren't present at journal replay time. It is also an information leak.
Until if/when we can get this working properly, make encrypted directory operations ineligible for fast-commit.
Note that fast-commit operations on encrypted regular files continue to be allowed, as they seem to work.
Fixes aa75f4d3daae ("ext4: main fast-commit commit path")
Cc <[email protected]> # v5.10+
Signed-off-by Eric Biggers <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
--- fs/ext4/fast_commit.c | 41 ++++++++++++++++++++++--------------- fs/ext4/fast_commit.h | 1 + include/trace/events/ext4.h | 7 +++++-- 3 files changed, 31 insertions(+), 18 deletions(-)
fs/ext4/fast_commit.c +25 -16
@@ -399,25 +399,34 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)struct __track_dentry_update_args *dentry_update = (struct __track_dentry_update_args *)arg;struct dentry *dentry = dentry_update->dentry; -struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +struct inode *dir = dentry->d_parent->d_inode; +struct super_block *sb = inode->i_sb; +struct ext4_sb_info *sbi =EXT4_SB (sb);mutex_unlock (&ei->i_fc_lock); + +if (IS_ENCRYPTED (dir)) { +ext4_fc_mark_ineligible (sb,EXT4_FC_REASON_ENCRYPTED_FILENAME , +NULL ); +mutex_lock (&ei->i_fc_lock); +return -EOPNOTSUPP ; + } + node =kmem_cache_alloc (ext4_fc_dentry_cachep,GFP_NOFS );if (!node) { -ext4_fc_mark_ineligible (inode->i_sb,EXT4_FC_REASON_NOMEM ,NULL ); +ext4_fc_mark_ineligible (sb,EXT4_FC_REASON_NOMEM ,NULL );mutex_lock (&ei->i_fc_lock);return -ENOMEM ; } node->fcd_op = dentry_update->op; - node->fcd_parent = dentry->d_parent->d_inode->i_ino; + node->fcd_parent = dir->i_ino; node->fcd_ino = inode->i_ino;if (dentry->d_name.len >DNAME_INLINE_LEN ) { node->fcd_name.name =kmalloc (dentry->d_name.len,GFP_NOFS );if (!node->fcd_name.name) {kmem_cache_free (ext4_fc_dentry_cachep, node); -ext4_fc_mark_ineligible (inode->i_sb, -EXT4_FC_REASON_NOMEM ,NULL ); +ext4_fc_mark_ineligible (sb,EXT4_FC_REASON_NOMEM ,NULL );mutex_lock (&ei->i_fc_lock);return -ENOMEM ; }
@@ -2179,17 +2188,17 @@ void ext4_fc_init(struct super_block *sb, journal_t *journal) journal->j_fc_cleanup_callback = ext4_fc_cleanup; } -static const char *fc_ineligible_reasons[] = { -"Extended attributes changed" , -"Cross rename" , -"Journal flag changed" , -"Insufficient memory" , -"Swap boot" , -"Resize" , -"Dir renamed" , -"Falloc range op" , -"Data journalling" , -"FC Commit Failed" +static const char *const fc_ineligible_reasons[] = { + [EXT4_FC_REASON_XATTR ] ="Extended attributes changed" , + [EXT4_FC_REASON_CROSS_RENAME ] ="Cross rename" , + [EXT4_FC_REASON_JOURNAL_FLAG_CHANGE ] ="Journal flag changed" , + [EXT4_FC_REASON_NOMEM ] ="Insufficient memory" , + [EXT4_FC_REASON_SWAP_BOOT ] ="Swap boot" , + [EXT4_FC_REASON_RESIZE ] ="Resize" , + [EXT4_FC_REASON_RENAME_DIR ] ="Dir renamed" , + [EXT4_FC_REASON_FALLOC_RANGE ] ="Falloc range op" , + [EXT4_FC_REASON_INODE_JOURNAL_DATA ] ="Data journalling" , + [EXT4_FC_REASON_ENCRYPTED_FILENAME ] ="Encrypted filename" , };int ext4_fc_info_show (struct seq_file *seq,void *v)
fs/ext4/fast_commit.h +1 -0
@@ -96,6 +96,7 @@ enum {EXT4_FC_REASON_RENAME_DIR ,EXT4_FC_REASON_FALLOC_RANGE ,EXT4_FC_REASON_INODE_JOURNAL_DATA , +EXT4_FC_REASON_ENCRYPTED_FILENAME , EXT4_FC_REASON_MAX };
include/trace/events/ext4.h +5 -2
@@ -104,6 +104,7 @@ TRACE_DEFINE_ENUM(EXT4_FC_REASON_RESIZE);TRACE_DEFINE_ENUM (EXT4_FC_REASON_RENAME_DIR );TRACE_DEFINE_ENUM (EXT4_FC_REASON_FALLOC_RANGE );TRACE_DEFINE_ENUM (EXT4_FC_REASON_INODE_JOURNAL_DATA ); +TRACE_DEFINE_ENUM (EXT4_FC_REASON_ENCRYPTED_FILENAME );TRACE_DEFINE_ENUM (EXT4_FC_REASON_MAX );#define show_fc_reason (reason) \
@@ -116,7 +117,8 @@ TRACE_DEFINE_ENUM(EXT4_FC_REASON_MAX); { EXT4_FC_REASON_RESIZE, "RESIZE"}, \ { EXT4_FC_REASON_RENAME_DIR, "RENAME_DIR"}, \ { EXT4_FC_REASON_FALLOC_RANGE, "FALLOC_RANGE"}, \ - { EXT4_FC_REASON_INODE_JOURNAL_DATA, "INODE_JOURNAL_DATA"}) + { EXT4_FC_REASON_INODE_JOURNAL_DATA, "INODE_JOURNAL_DATA"}, \ + { EXT4_FC_REASON_ENCRYPTED_FILENAME, "ENCRYPTED_FILENAME"}) TRACE_EVENT(ext4_other_inode_update_time, TP_PROTO(struct inode *inode, ino_t orig_ino),
@@ -2764,7 +2766,7 @@ TRACE_EVENT(ext4_fc_stats, ), TP_printk("dev %d,%d fc ineligible reasons:\n" - "%s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u " + "%s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u" "num_commits:%lu, ineligible: %lu, numblks: %lu", MAJOR(__entry->dev), MINOR(__entry->dev), FC_REASON_NAME_STAT(EXT4_FC_REASON_XATTR),
@@ -2776,6 +2778,7 @@ TRACE_EVENT(ext4_fc_stats,FC_REASON_NAME_STAT (EXT4_FC_REASON_RENAME_DIR ),FC_REASON_NAME_STAT (EXT4_FC_REASON_FALLOC_RANGE ),FC_REASON_NAME_STAT (EXT4_FC_REASON_INODE_JOURNAL_DATA ), +FC_REASON_NAME_STAT (EXT4_FC_REASON_ENCRYPTED_FILENAME ), __entry->fc_commits, __entry->fc_ineligible_commits, __entry->fc_numblks) );
Signature · 1 line
2.39.0
PATCH 7/10 ext4: don't set up encryption key during jbd2 transaction
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
commit 4c0d5778385cb3618ff26a561ce41de2b7d9de70 upstream.
Commit a80f7fcf1867 ("ext4: fixup ext4_fc_track_* functions' signature") extended the scope of the transaction in ext4_unlink() too far, making it include the call to ext4_find_entry(). However, ext4_find_entry() can deadlock when called from within a transaction because it may need to set up the directory's encryption key.
Fix this by restoring the transaction to its original scope.
Reported-by [email protected]
Fixes a80f7fcf1867 ("ext4: fixup ext4_fc_track_* functions' signature")
Cc <[email protected]> # v5.10+
Signed-off-by Eric Biggers <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
--- fs/ext4/ext4.h | 4 ++-- fs/ext4/fast_commit.c | 2 +- fs/ext4/namei.c | 44 +++++++++++++++++++++++-------------------- 3 files changed, 27 insertions(+), 23 deletions(-)
fs/ext4/ext4.h +2 -2
@@ -3647,8 +3647,8 @@ extern void ext4_initialize_dirent_tail(struct buffer_head *bh, unsigned int blocksize);extern int ext4_handle_dirty_dirblock (handle_t *handle,struct inode *inode,struct buffer_head *bh); -extern int __ext4_unlink (handle_t *handle,struct inode *dir,const struct qstr *d_name, -struct inode *inode); +extern int __ext4_unlink (struct inode *dir,const struct qstr *d_name, +struct inode *inode,struct dentry *dentry);extern int __ext4_link (struct inode *dir,struct inode *inode,struct dentry *dentry);
fs/ext4/fast_commit.c +1 -1
@@ -1330,7 +1330,7 @@ static int ext4_fc_replay_unlink(struct super_block *sb, struct ext4_fc_tl *tl,return 0 ; } - ret =__ext4_unlink (NULL , old_parent, &entry, inode); + ret =__ext4_unlink (old_parent, &entry, inode,NULL );/* -ENOENT ok coz it might not exist anymore. */ if (ret == -ENOENT ) ret =0 ;
fs/ext4/namei.c +24 -20
96 lines not on this page · raw message
Signature · 1 line
2.39.0
PATCH 8/10 ext4: add missing validation of fast-commit record lengths
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
commit 64b4a25c3de81a69724e888ec2db3533b43816e2 upstream.
Validate the inode and filename lengths in fast-commit journal records so that a malicious fast-commit journal cannot cause a crash by having invalid values for these. Also validate EXT4_FC_TAG_DEL_RANGE.
Fixes aa75f4d3daae ("ext4: main fast-commit commit path")
Cc <[email protected]> # v5.10+
Signed-off-by Eric Biggers <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
--- fs/ext4/fast_commit.c | 38 +++++++++++++++++++------------------- fs/ext4/fast_commit.h | 2 +- 2 files changed, 20 insertions(+), 20 deletions(-)
fs/ext4/fast_commit.c +19 -19
@@ -1916,32 +1916,31 @@ void ext4_fc_replay_cleanup(struct super_block *sb)kfree (sbi->s_fc_replay_state.fc_modified_inodes); } -static inline bool ext4_fc_tag_len_isvalid (struct ext4_fc_tl *tl, -u8 *val,u8 *end) +static bool ext4_fc_value_len_isvalid (struct ext4_sb_info *sbi, +int tag,int len) { -if (val + tl->fc_len > end) -return false; - -/* Here only check ADD_RANGE/TAIL/HEAD which will read data when do -* journal rescan before do CRC check. Other tags length check will -* rely on CRC check. -*/ -switch (tl->fc_tag) { +switch (tag) {case EXT4_FC_TAG_ADD_RANGE : -return (sizeof (struct ext4_fc_add_range ) == tl->fc_len); -case EXT4_FC_TAG_TAIL : -return (sizeof (struct ext4_fc_tail ) <= tl->fc_len); -case EXT4_FC_TAG_HEAD : -return (sizeof (struct ext4_fc_head ) == tl->fc_len); +return len ==sizeof (struct ext4_fc_add_range );case EXT4_FC_TAG_DEL_RANGE : +return len ==sizeof (struct ext4_fc_del_range ); +case EXT4_FC_TAG_CREAT :case EXT4_FC_TAG_LINK :case EXT4_FC_TAG_UNLINK : -case EXT4_FC_TAG_CREAT : + len -=sizeof (struct ext4_fc_dentry_info ); +return len >=1 && len <=EXT4_NAME_LEN ;case EXT4_FC_TAG_INODE : + len -=sizeof (struct ext4_fc_inode ); +return len >=EXT4_GOOD_OLD_INODE_SIZE && + len <= sbi->s_inode_size;case EXT4_FC_TAG_PAD : -default : -return true; +return true;/* padding can have any length */ +case EXT4_FC_TAG_TAIL : +return len >=sizeof (struct ext4_fc_tail ); +case EXT4_FC_TAG_HEAD : +return len ==sizeof (struct ext4_fc_head ); } +return false; }/*
@@ -2004,7 +2003,8 @@ static int ext4_fc_replay_scan(journal_t *journal, cur = cur + EXT4_FC_TAG_BASE_LEN + tl.fc_len) {ext4_fc_get_tl (&tl, cur); val = cur +EXT4_FC_TAG_BASE_LEN ; -if (!ext4_fc_tag_len_isvalid (&tl, val, end)) { +if (tl.fc_len > end - val || + !ext4_fc_value_len_isvalid (sbi, tl.fc_tag, tl.fc_len)) { ret = state->fc_replay_num_tags ?JBD2_FC_REPLAY_STOP : -ECANCELED ; goto out_err;
fs/ext4/fast_commit.h +1 -1
@@ -58,7 +58,7 @@ struct ext4_fc_dentry_info {__u8 fc_dname[0 ]; }; -/* Value structure for EXT4_FC_TAG_INODE and EXT4_FC_TAG_INODE_PARTIAL. */ +/* Value structure for EXT4_FC_TAG_INODE. */ struct ext4_fc_inode {__le32 fc_ino;__u8 fc_raw_inode[0 ];
Signature · 1 line
2.39.0
PATCH 9/10 ext4: fix unaligned memory access in ext4_fc_reserve_space()
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
commit 8415ce07ecf0cc25efdd5db264a7133716e503cf upstream.
As is done elsewhere in the file, build the struct ext4_fc_tl on the stack and memcpy() it into the buffer, rather than directly writing it to a potentially-unaligned location in the buffer.
Fixes aa75f4d3daae ("ext4: main fast-commit commit path")
Cc <[email protected]> # v5.10+
Signed-off-by Eric Biggers <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
--- fs/ext4/fast_commit.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-)
fs/ext4/fast_commit.c +21 -18
@@ -604,6 +604,15 @@ static void ext4_fc_submit_bh(struct super_block *sb, bool is_tail)/* Ext4 commit path routines */ +/* memcpy to fc reserved space and update CRC */ +static void *ext4_fc_memcpy (struct super_block *sb,void *dst,const void *src, +int len,u32 *crc) +{ +if (crc) + *crc =ext4_chksum (EXT4_SB (sb), *crc, src, len); +return memcpy (dst, src, len); +} +/* memzero and update CRC */ static void *ext4_fc_memzero (struct super_block *sb,void *dst,int len,u32 *crc)
@@ -629,12 +638,13 @@ static void *ext4_fc_memzero(struct super_block *sb, void *dst, int len,*/ static u8 *ext4_fc_reserve_space (struct super_block *sb,int len,u32 *crc) { -struct ext4_fc_tl *tl; +struct ext4_fc_tl tl;struct ext4_sb_info *sbi =EXT4_SB (sb);struct buffer_head *bh;int bsize = sbi->s_journal->j_blocksize;int ret, off = sbi->s_fc_bytes % bsize;int pad_len; +u8 *dst;/* * After allocating len, we should have space at least for a 0 byte
@@ -658,16 +668,18 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)return sbi->s_fc_bh->b_data + off; }/* Need to add PAD tag */ - tl = (struct ext4_fc_tl *)(sbi->s_fc_bh->b_data + off); - tl->fc_tag =cpu_to_le16 (EXT4_FC_TAG_PAD ); + dst = sbi->s_fc_bh->b_data + off; + tl.fc_tag =cpu_to_le16 (EXT4_FC_TAG_PAD ); pad_len = bsize - off -1 -EXT4_FC_TAG_BASE_LEN ; - tl->fc_len =cpu_to_le16 (pad_len); -if (crc) - *crc =ext4_chksum (sbi, *crc, tl,EXT4_FC_TAG_BASE_LEN ); -if (pad_len >0 ) -ext4_fc_memzero (sb, tl +1 , pad_len, crc); + tl.fc_len =cpu_to_le16 (pad_len); +ext4_fc_memcpy (sb, dst, &tl,EXT4_FC_TAG_BASE_LEN , crc); + dst +=EXT4_FC_TAG_BASE_LEN ; +if (pad_len >0 ) { +ext4_fc_memzero (sb, dst, pad_len, crc); + dst += pad_len; + }/* Don't leak uninitialized memory in the unused last byte. */ - *((u8 *)(tl +1 ) + pad_len) =0 ; + *dst =0 ;ext4_fc_submit_bh (sb, false);
@@ -679,15 +691,6 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)return sbi->s_fc_bh->b_data; } -/* memcpy to fc reserved space and update CRC */ -static void *ext4_fc_memcpy (struct super_block *sb,void *dst,const void *src, -int len,u32 *crc) -{ -if (crc) - *crc =ext4_chksum (EXT4_SB (sb), *crc, src, len); -return memcpy (dst, src, len); -} -/* * Complete a fast commit by writing tail tag. *
Signature · 1 line
2.39.0
PATCH 10/10 ext4: fix off-by-one errors in fast-commit block filling
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
commit 48a6a66db82b8043d298a630f22c62d43550cae5 upstream.
Due to several different off-by-one errors, or perhaps due to a late change in design that wasn't fully reflected in the code that was actually merged, there are several very strange constraints on how fast-commit blocks are filled with tlv entries:
- tlvs must start at least 10 bytes before the end of the block, even though the minimum tlv length is 8. Otherwise, the replay code will ignore them. (BUG: ext4_fc_reserve_space() could violate this requirement if called with a len of blocksize - 9 or blocksize - 8. Fortunately, this doesn't seem to happen currently.)
- tlvs must end at least 1 byte before the end of the block. Otherwise the replay code will consider them to be invalid. This quirk contributed to a bug (fixed by an earlier commit) where uninitialized memory was being leaked to disk in the last byte of blocks.
Also, strangely these constraints don't apply to the replay code in e2fsprogs, which will accept any tlvs in the blocks (with no bounds checks at all, but that is a separate issue...).
Given that this all seems to be a bug, let's fix it by just filling blocks with tlv entries in the natural way.
Note that old kernels will be unable to replay fast-commit journals created by kernels that have this commit.
Fixes aa75f4d3daae ("ext4: main fast-commit commit path")
Cc <[email protected]> # v5.10+
Signed-off-by Eric Biggers <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
--- fs/ext4/fast_commit.c | 66 +++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 33 deletions(-)
fs/ext4/fast_commit.c +33 -33
@@ -643,43 +643,43 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)struct buffer_head *bh;int bsize = sbi->s_journal->j_blocksize;int ret, off = sbi->s_fc_bytes % bsize; -int pad_len; +int remaining;u8 *dst;/* -* After allocating len, we should have space at least for a 0 byte -* padding. +* If 'len' is too long to fit in any block alongside a PAD tlv, then we +* cannot fulfill the request. */ -if (len +EXT4_FC_TAG_BASE_LEN > bsize) +if (len > bsize -EXT4_FC_TAG_BASE_LEN )return NULL ; -if (bsize - off -1 > len +EXT4_FC_TAG_BASE_LEN ) { -/* -* Only allocate from current buffer if we have enough space for -* this request AND we have space to add a zero byte padding. -*/ -if (!sbi->s_fc_bh) { - ret =jbd2_fc_get_buf (EXT4_SB (sb)->s_journal, &bh); -if (ret) -return NULL ; - sbi->s_fc_bh = bh; - } - sbi->s_fc_bytes += len; -return sbi->s_fc_bh->b_data + off; +if (!sbi->s_fc_bh) { + ret =jbd2_fc_get_buf (EXT4_SB (sb)->s_journal, &bh); +if (ret) +return NULL ; + sbi->s_fc_bh = bh; } -/* Need to add PAD tag */ dst = sbi->s_fc_bh->b_data + off; + +/* +* Allocate the bytes in the current block if we can do so while still +* leaving enough space for a PAD tlv. +*/ + remaining = bsize -EXT4_FC_TAG_BASE_LEN - off; +if (len <= remaining) { + sbi->s_fc_bytes += len; +return dst; + } + +/* +* Else, terminate the current block with a PAD tlv, then allocate a new +* block and allocate the bytes at the start of that new block. +*/ + tl.fc_tag =cpu_to_le16 (EXT4_FC_TAG_PAD ); - pad_len = bsize - off -1 -EXT4_FC_TAG_BASE_LEN ; - tl.fc_len =cpu_to_le16 (pad_len); + tl.fc_len =cpu_to_le16 (remaining);ext4_fc_memcpy (sb, dst, &tl,EXT4_FC_TAG_BASE_LEN , crc); - dst +=EXT4_FC_TAG_BASE_LEN ; -if (pad_len >0 ) { -ext4_fc_memzero (sb, dst, pad_len, crc); - dst += pad_len; - } -/* Don't leak uninitialized memory in the unused last byte. */ - *dst =0 ; +ext4_fc_memzero (sb, dst +EXT4_FC_TAG_BASE_LEN , remaining, crc);ext4_fc_submit_bh (sb, false);
@@ -687,7 +687,7 @@ static u8 *ext4_fc_reserve_space(struct super_block *sb, int len, u32 *crc)if (ret)return NULL ; sbi->s_fc_bh = bh; - sbi->s_fc_bytes = (sbi->s_fc_bytes / bsize +1 ) * bsize + len; + sbi->s_fc_bytes += bsize - off + len;return sbi->s_fc_bh->b_data; }
@@ -718,7 +718,7 @@ static int ext4_fc_write_tail(struct super_block *sb, u32 crc) off = sbi->s_fc_bytes % bsize; tl.fc_tag =cpu_to_le16 (EXT4_FC_TAG_TAIL ); - tl.fc_len =cpu_to_le16 (bsize - off -1 +sizeof (struct ext4_fc_tail )); + tl.fc_len =cpu_to_le16 (bsize - off +sizeof (struct ext4_fc_tail )); sbi->s_fc_bytes =round_up (sbi->s_fc_bytes, bsize);ext4_fc_memcpy (sb, dst, &tl,EXT4_FC_TAG_BASE_LEN , &crc);
@@ -1981,7 +1981,7 @@ static int ext4_fc_replay_scan(journal_t *journal, state = &sbi->s_fc_replay_state; start = (u8 *)bh->b_data; - end = (__u8 *)bh->b_data + journal->j_blocksize -1 ; + end = start + journal->j_blocksize;if (state->fc_replay_expected_off ==0 ) { state->fc_cur_tag =0 ;
@@ -2002,7 +2002,7 @@ static int ext4_fc_replay_scan(journal_t *journal, } state->fc_replay_expected_off++; -for (cur = start; cur < end -EXT4_FC_TAG_BASE_LEN ; +for (cur = start; cur <= end -EXT4_FC_TAG_BASE_LEN ; cur = cur +EXT4_FC_TAG_BASE_LEN + tl.fc_len) {ext4_fc_get_tl (&tl, cur); val = cur +EXT4_FC_TAG_BASE_LEN ;
@@ -2120,9 +2120,9 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,#endif start = (u8 *)bh->b_data; - end = (__u8 *)bh->b_data + journal->j_blocksize -1 ; + end = start + journal->j_blocksize; -for (cur = start; cur < end -EXT4_FC_TAG_BASE_LEN ; +for (cur = start; cur <= end -EXT4_FC_TAG_BASE_LEN ; cur = cur +EXT4_FC_TAG_BASE_LEN + tl.fc_len) {ext4_fc_get_tl (&tl, cur); val = cur +EXT4_FC_TAG_BASE_LEN ;
Signature · 1 line
2.39.0
Quoted 7 lines · Eric Biggers
This series backports 6 commits with 'Cc stable' that had failed to be applied, and 4 related commits that made the backports much easier. Please apply this series to 5.15-stable.
I verified that this series does not cause any regressions with 'gce-xfstests -c ext4/fast_commit -g auto'. There is one test failure both before and after (ext4/050).
All now queued up, thanks.
greg k-h
Hi Greg,
Quoted 5 lines · Greg KH
Quoted 4 lines · Eric Biggers
This series backports 6 commits with 'Cc stable' that had failed to be applied, and 4 related commits that made the backports much easier. Please apply this series to 5.15-stable.
Quoted 7 lines
Quoted 3 lines
I verified that this series does not cause any regressions with 'gce-xfstests -c ext4/fast_commit -g auto'. There is one test failure both before and after (ext4/050).
All now queued up, thanks.
greg k-h
It's too late to fix now, but the commits in 5.15-stable all use "Eric Biggers <[email protected]>" as the author instead of the From line in the patch itself. For example, patch 1 became:
commit b0ed9a032e52a175683d18e2e2e8eec0f9ba1ff9 Author: Eric Biggers <[email protected]> Date: Wed Jan 4 23:13:50 2023 -0800 ext4: remove unused enum EXT4_FC_COMMIT_FAILED From: Ritesh Harjani <[email protected]> commit c864ccd182d6ff2730a0f5b636c6b7c48f6f4f7f upstream.
For reference, the upstream commit is:
commit c864ccd182d6ff2730a0f5b636c6b7c48f6f4f7f Author: Ritesh Harjani <[email protected]> Date: Sat Mar 12 11:09:46 2022 +0530 ext4: remove unused enum EXT4_FC_COMMIT_FAILED
Do you know how this happened, and how it can be prevented in the future? I think I sent everything out correctly, so I think this is something on your end.
- Eric
Quoted 33 lines · Eric Biggers
Hi Greg,
Quoted 5 lines · Greg KH
Quoted 4 lines · Eric Biggers
This series backports 6 commits with 'Cc stable' that had failed to be applied, and 4 related commits that made the backports much easier. Please apply this series to 5.15-stable.
Quoted 7 lines
Quoted 3 lines
I verified that this series does not cause any regressions with 'gce-xfstests -c ext4/fast_commit -g auto'. There is one test failure both before and after (ext4/050).
All now queued up, thanks.
greg k-h
It's too late to fix now, but the commits in 5.15-stable all use "Eric Biggers <[email protected]>" as the author instead of the From line in the patch itself. For example, patch 1 became:
commit b0ed9a032e52a175683d18e2e2e8eec0f9ba1ff9 Author: Eric Biggers <[email protected]> Date: Wed Jan 4 23:13:50 2023 -0800 ext4: remove unused enum EXT4_FC_COMMIT_FAILED From: Ritesh Harjani <[email protected]> commit c864ccd182d6ff2730a0f5b636c6b7c48f6f4f7f upstream.For reference, the upstream commit is:
Quoted 8 lines
commit c864ccd182d6ff2730a0f5b636c6b7c48f6f4f7f Author: Ritesh Harjani <[email protected]> Date: Sat Mar 12 11:09:46 2022 +0530 ext4: remove unused enum EXT4_FC_COMMIT_FAILEDDo you know how this happened, and how it can be prevented in the future? I think I sent everything out correctly, so I think this is something on your end.
Yes, this is on my end, sorry, my scripts mess this up when dealing with mbox files and I missed having to edit the header "by hand" like I normally do when it happens.
My fault,
greg k-h