linux-kernel - Re: [PATCH -next] ext4: Fix symlink file size not match to file content

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]

Message-ID: <20220321141236.7rmbh77ntsmpc2m6@quack3.lan>
Date:   Mon, 21 Mar 2022 15:12:36 +0100
From:   Jan Kara <jack@...e.cz>
To:     yebin <yebin10@...wei.com>
Cc:     Jan Kara <jack@...e.cz>, tytso@....edu, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        lczerner@...hat.com
Subject: Re: [PATCH -next] ext4: Fix symlink file size not match to file
 content

On Mon 21-03-22 21:35:01, yebin wrote:
> 
> 
> On 2022/3/21 19:37, Jan Kara wrote:
> > On Mon 21-03-22 19:34:08, Ye Bin wrote:
> > > We got issue as follows:
> > > [home]# fsck.ext4  -fn  ram0yb
> > > e2fsck 1.45.6 (20-Mar-2020)
> > > Pass 1: Checking inodes, blocks, and sizes
> > > Pass 2: Checking directory structure
> > > Symlink /p3/d14/d1a/l3d (inode #3494) is invalid.
> > > Clear? no
> > > Entry 'l3d' in /p3/d14/d1a (3383) has an incorrect filetype (was 7, should be 0).
> > > Fix? no
> > > 
> > > As symlink file size not match to file content. If symlink data block
> > > writback failed, will call ext4_finish_bio to end io. In this path don't
> > > mark buffer error. When umount do checkpoint can't detect buffer error,
> > > then will cleanup jounral. Actually, correct data maybe in journal area.
> > > To solve this issue, mark buffer error when detect bio error in
> > > ext4_finish_bio.
> > Thanks for the patch! Let me rephrase the text a bit:
> > 
> > As the symlink file size does not match the file content. If the writeback
> > of the symlink data block failed, ext4_finish_bio() handles the end of IO.
> > However this function fails to mark the buffer with BH_write_io_error and
> > so when unmount does journal checkpoint it cannot detect the writeback
> > error and will cleanup the journal. Thus we've lost the correct data in the
> > journal area. To solve this issue, mark the buffer as BH_write_io_error in
> > ext4_finish_bio().
> > 
> > > Signed-off-by: Ye Bin <yebin10@...wei.com>
> > > ---
> > >   fs/ext4/page-io.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> > > index 495ce59fb4ad..14695e2b5042 100644
> > > --- a/fs/ext4/page-io.c
> > > +++ b/fs/ext4/page-io.c
> > > @@ -134,8 +134,10 @@ static void ext4_finish_bio(struct bio *bio)
> > >   				continue;
> > >   			}
> > >   			clear_buffer_async_write(bh);
> > > -			if (bio->bi_status)
> > > +			if (bio->bi_status) {
> > > +				set_buffer_write_io_error(bh);
> > Why don't you use mark_buffer_write_io_error()? It will also update other IO
> > error counters properly so that e.g. fsync(2) or sync_filesystem() can properly
> > report IO error etc. Granted we'll abort the journal in response to
> > checkpointing error so the failure will be hard to miss anyway but still
> > :).
> > 
> > 								Honza
> 
> 'ext4_finish_bio' already call 'mapping_set_error' set mapping error , I think fsync
> and sync_filesystem can report IO error.
> 
> static inline void mapping_set_error(struct address_space *mapping, int error)
> {
>         if (likely(!error))
>                 return;
> 
>         /* Record in wb_err for checkers using errseq_t based tracking */
>         __filemap_set_wb_err(mapping, error);
> 
>         /* Record it in superblock */
>         if (mapping->host)
>                 errseq_set(&mapping->host->i_sb->s_wb_err, error);
> 
>         /* Record it in flags for now, for legacy callers */
>         if (error == -ENOSPC)
>                 set_bit(AS_ENOSPC, &mapping->flags);
>         else
>                 set_bit(AS_EIO, &mapping->flags);
> }

Good, I've missed that. OK, then feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR