From: Jan Kara <jack@suse.cz>
To: Zhang Yi <yi.zhang@huawei.com>
Cc: linux-ext4@vger.kernel.org, tytso@mit.edu,
adilger.kernel@dilger.ca, jack@suse.cz, yukuai3@huawei.com
Subject: Re: [PATCH 3/3] ext4: prevent getting empty inode buffer
Date: Fri, 13 Aug 2021 15:44:40 +0200 [thread overview]
Message-ID: <20210813134440.GE11955@quack2.suse.cz> (raw)
In-Reply-To: <20210810142722.923175-4-yi.zhang@huawei.com>
On Tue 10-08-21 22:27:22, Zhang Yi wrote:
> In ext4_get_inode_loc(), we may skip IO and get an zero && uptodate
> inode buffer when the inode monopolize an inode block for performance
> reason. For most cases, ext4_mark_iloc_dirty() will fill the inode
> buffer to make it fine, but we could miss this call if something bad
> happened. Finally, __ext4_get_inode_loc_noinmem() may probably get an
> empty inode buffer and trigger ext4 error.
>
> For example, if we remove a nonexistent xattr on inode A,
> ext4_xattr_set_handle() will return ENODATA before invoking
> ext4_mark_iloc_dirty(), it will left an uptodate but zero buffer. We
> will get checksum error message in ext4_iget() when getting inode again.
>
> EXT4-fs error (device sda): ext4_lookup:1784: inode #131074: comm cat: iget: checksum invalid
>
> Even worse, if we allocate another inode B at the same inode block, it
> will corrupt the inode A on disk when write back inode B.
>
> So this patch clear uptodate flag and mark buffer new if we get an empty
> buffer, clear it after we fill inode data or making read IO.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Thanks for the fix! Really good catch! The patch looks correct but
honestly, I'm not very happy about the special buffer_new handling. It
looks correct but I'm a bit uneasy that e.g. the block device code can
access this buffer and manipulate its state. Cannot we instead e.g. check
whether the buffer is uptodate in ext4_mark_iloc_dirty(), if not, lock it,
if still not uptodate, zero it, mark as uptodate, unlock it and then call
ext4_do_update_inode()? That would seem like a bit more foolproof solution
to me. Basically the fact that the buffer is not uptodate in
ext4_mark_iloc_dirty() would mean that nobody else is past
__ext4_get_inode_loc() for another inode in that buffer and so zeroing is
safe.
Honza
> ---
> fs/ext4/inode.c | 27 +++++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index eae1b2d0b550..1f36289e9ef6 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4292,6 +4292,18 @@ int ext4_truncate(struct inode *inode)
> return err;
> }
>
> +static void ext4_end_inode_loc_read(struct buffer_head *bh, int uptodate)
> +{
> + if (buffer_new(bh))
> + clear_buffer_new(bh);
> + if (uptodate)
> + set_buffer_uptodate(bh);
> + else
> + clear_buffer_uptodate(bh);
> + unlock_buffer(bh);
> + put_bh(bh);
> +}
> +
> /*
> * ext4_get_inode_loc returns with an extra refcount against the inode's
> * underlying buffer_head on success. If 'in_mem' is true, we have all
> @@ -4367,9 +4379,11 @@ static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
> }
> brelse(bitmap_bh);
> if (i == start + inodes_per_block) {
> - /* all other inodes are free, so skip I/O */
> - memset(bh->b_data, 0, bh->b_size);
> - set_buffer_uptodate(bh);
> + if (!buffer_new(bh)) {
> + /* all other inodes are free, so skip I/O */
> + memset(bh->b_data, 0, bh->b_size);
> + set_buffer_new(bh);
> + }
> unlock_buffer(bh);
> goto has_buffer;
> }
> @@ -4408,7 +4422,7 @@ static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
> * Read the block from disk.
> */
> trace_ext4_load_inode(sb, ino);
> - ext4_read_bh_nowait(bh, REQ_META | REQ_PRIO, NULL);
> + ext4_read_bh_nowait(bh, REQ_META | REQ_PRIO, ext4_end_inode_loc_read);
> blk_finish_plug(&plug);
> wait_on_buffer(bh);
> ext4_simulate_fail_bh(sb, bh, EXT4_SIM_INODE_EIO);
> @@ -5132,6 +5146,11 @@ static int ext4_do_update_inode(handle_t *handle,
> if (err)
> goto out_brelse;
> ext4_clear_inode_state(inode, EXT4_STATE_NEW);
> + if (buffer_new(bh)) {
> + clear_buffer_new(bh);
> + set_buffer_uptodate(bh);
> + }
> +
> if (set_large_file) {
> BUFFER_TRACE(EXT4_SB(sb)->s_sbh, "get write access");
> err = ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh);
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2021-08-13 13:46 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-10 14:27 [PATCH 0/3] ext4: fix a inode checksum error Zhang Yi
2021-08-10 14:27 ` [PATCH 1/3] ext4: move inode eio simulation behind io completeion Zhang Yi
2021-08-13 12:55 ` Jan Kara
2021-08-10 14:27 ` [PATCH 2/3] ext4: remove an unnecessary if statement in __ext4_get_inode_loc() Zhang Yi
2021-08-13 13:00 ` Jan Kara
2021-08-10 14:27 ` [PATCH 3/3] ext4: prevent getting empty inode buffer Zhang Yi
2021-08-13 13:44 ` Jan Kara [this message]
2021-08-16 14:29 ` Zhang Yi
2021-08-16 17:14 ` Jan Kara
2021-08-18 12:15 ` Zhang Yi
2021-08-18 13:11 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210813134440.GE11955@quack2.suse.cz \
--to=jack@suse.cz \
--cc=adilger.kernel@dilger.ca \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=yi.zhang@huawei.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).