* [PATCH] ext4: lock the the xattr block before calculating its checksum @ 2017-02-28 13:56 Theodore Ts'o 2017-02-28 16:12 ` Darrick J. Wong 0 siblings, 1 reply; 4+ messages in thread From: Theodore Ts'o @ 2017-02-28 13:56 UTC (permalink / raw) To: Ext4 Developers List; +Cc: Theodore Ts'o We must lock the xattr block in order to avoid spurious checksum failures. https://bugzilla.kernel.org/show_bug.cgi?id=193661 Reported-by: Colin Ian King <colin.king@canonical.com> Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/ext4/xattr.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 67636acf7624..3247057ef5ff 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -131,30 +131,27 @@ static __le32 ext4_xattr_block_csum(struct inode *inode, } static int ext4_xattr_block_csum_verify(struct inode *inode, - sector_t block_nr, - struct ext4_xattr_header *hdr) + struct buffer_head *bh) { - if (ext4_has_metadata_csum(inode->i_sb) && - (hdr->h_checksum != ext4_xattr_block_csum(inode, block_nr, hdr))) - return 0; - return 1; -} - -static void ext4_xattr_block_csum_set(struct inode *inode, - sector_t block_nr, - struct ext4_xattr_header *hdr) -{ - if (!ext4_has_metadata_csum(inode->i_sb)) - return; + struct ext4_xattr_header *hdr = BHDR(bh); + int ret = 1; - hdr->h_checksum = ext4_xattr_block_csum(inode, block_nr, hdr); + if (ext4_has_metadata_csum(inode->i_sb)) { + lock_buffer(bh); + ret = hdr->h_checksum == ext4_xattr_block_csum(inode, + bh->b_blocknr, hdr); + unlock_buffer(bh); + } + return ret; } static inline int ext4_handle_dirty_xattr_block(handle_t *handle, struct inode *inode, struct buffer_head *bh) { - ext4_xattr_block_csum_set(inode, bh->b_blocknr, BHDR(bh)); + if (ext4_has_metadata_csum(inode->i_sb)) + BHDR(bh)->h_checksum = ext4_xattr_block_csum(inode, + bh->b_blocknr, BHDR(bh)); return ext4_handle_dirty_metadata(handle, inode, bh); } @@ -233,7 +230,7 @@ ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh) if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) || BHDR(bh)->h_blocks != cpu_to_le32(1)) return -EFSCORRUPTED; - if (!ext4_xattr_block_csum_verify(inode, bh->b_blocknr, BHDR(bh))) + if (!ext4_xattr_block_csum_verify(inode, bh)) return -EFSBADCRC; error = ext4_xattr_check_names(BFIRST(bh), bh->b_data + bh->b_size, bh->b_data); -- 2.11.0.rc0.7.gbe5a750 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: lock the the xattr block before calculating its checksum 2017-02-28 13:56 [PATCH] ext4: lock the the xattr block before calculating its checksum Theodore Ts'o @ 2017-02-28 16:12 ` Darrick J. Wong 2017-03-25 21:24 ` [PATCH -v2] ext4: lock the xattr block before checksuming it Theodore Ts'o 0 siblings, 1 reply; 4+ messages in thread From: Darrick J. Wong @ 2017-02-28 16:12 UTC (permalink / raw) To: Theodore Ts'o; +Cc: Ext4 Developers List On Tue, Feb 28, 2017 at 08:56:50AM -0500, Theodore Ts'o wrote: > We must lock the xattr block in order to avoid spurious checksum > failures. > > https://bugzilla.kernel.org/show_bug.cgi?id=193661 > > Reported-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> Looks good, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/ext4/xattr.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 67636acf7624..3247057ef5ff 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -131,30 +131,27 @@ static __le32 ext4_xattr_block_csum(struct inode *inode, > } > > static int ext4_xattr_block_csum_verify(struct inode *inode, > - sector_t block_nr, > - struct ext4_xattr_header *hdr) > + struct buffer_head *bh) > { > - if (ext4_has_metadata_csum(inode->i_sb) && > - (hdr->h_checksum != ext4_xattr_block_csum(inode, block_nr, hdr))) > - return 0; > - return 1; > -} > - > -static void ext4_xattr_block_csum_set(struct inode *inode, > - sector_t block_nr, > - struct ext4_xattr_header *hdr) > -{ > - if (!ext4_has_metadata_csum(inode->i_sb)) > - return; > + struct ext4_xattr_header *hdr = BHDR(bh); > + int ret = 1; > > - hdr->h_checksum = ext4_xattr_block_csum(inode, block_nr, hdr); > + if (ext4_has_metadata_csum(inode->i_sb)) { > + lock_buffer(bh); > + ret = hdr->h_checksum == ext4_xattr_block_csum(inode, > + bh->b_blocknr, hdr); > + unlock_buffer(bh); > + } > + return ret; > } > > static inline int ext4_handle_dirty_xattr_block(handle_t *handle, > struct inode *inode, > struct buffer_head *bh) > { > - ext4_xattr_block_csum_set(inode, bh->b_blocknr, BHDR(bh)); > + if (ext4_has_metadata_csum(inode->i_sb)) > + BHDR(bh)->h_checksum = ext4_xattr_block_csum(inode, > + bh->b_blocknr, BHDR(bh)); > return ext4_handle_dirty_metadata(handle, inode, bh); > } > > @@ -233,7 +230,7 @@ ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh) > if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) || > BHDR(bh)->h_blocks != cpu_to_le32(1)) > return -EFSCORRUPTED; > - if (!ext4_xattr_block_csum_verify(inode, bh->b_blocknr, BHDR(bh))) > + if (!ext4_xattr_block_csum_verify(inode, bh)) > return -EFSBADCRC; > error = ext4_xattr_check_names(BFIRST(bh), bh->b_data + bh->b_size, > bh->b_data); > -- > 2.11.0.rc0.7.gbe5a750 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH -v2] ext4: lock the xattr block before checksuming it 2017-02-28 16:12 ` Darrick J. Wong @ 2017-03-25 21:24 ` Theodore Ts'o 2017-03-26 0:20 ` Colin Ian King 0 siblings, 1 reply; 4+ messages in thread From: Theodore Ts'o @ 2017-03-25 21:24 UTC (permalink / raw) To: Ext4 Developers List; +Cc: colin.king, Theodore Ts'o, stable We must lock the xattr block before calculating or verifying the checksum in order to avoid spurious checksum failures. https://bugzilla.kernel.org/show_bug.cgi?id=193661 Reported-by: Colin Ian King <colin.king@canonical.com> Signed-off-by: Theodore Ts'o <tytso@mit.edu> Cc: stable@vger.kernel.org --- fs/ext4/xattr.c | 65 +++++++++++++++++++++++++++------------------------------ 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 67636acf7624..996e7900d4c8 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -131,31 +131,26 @@ static __le32 ext4_xattr_block_csum(struct inode *inode, } static int ext4_xattr_block_csum_verify(struct inode *inode, - sector_t block_nr, - struct ext4_xattr_header *hdr) + struct buffer_head *bh) { - if (ext4_has_metadata_csum(inode->i_sb) && - (hdr->h_checksum != ext4_xattr_block_csum(inode, block_nr, hdr))) - return 0; - return 1; -} - -static void ext4_xattr_block_csum_set(struct inode *inode, - sector_t block_nr, - struct ext4_xattr_header *hdr) -{ - if (!ext4_has_metadata_csum(inode->i_sb)) - return; + struct ext4_xattr_header *hdr = BHDR(bh); + int ret = 1; - hdr->h_checksum = ext4_xattr_block_csum(inode, block_nr, hdr); + if (ext4_has_metadata_csum(inode->i_sb)) { + lock_buffer(bh); + ret = (hdr->h_checksum == ext4_xattr_block_csum(inode, + bh->b_blocknr, hdr)); + unlock_buffer(bh); + } + return ret; } -static inline int ext4_handle_dirty_xattr_block(handle_t *handle, - struct inode *inode, - struct buffer_head *bh) +static void ext4_xattr_block_csum_set(struct inode *inode, + struct buffer_head *bh) { - ext4_xattr_block_csum_set(inode, bh->b_blocknr, BHDR(bh)); - return ext4_handle_dirty_metadata(handle, inode, bh); + if (ext4_has_metadata_csum(inode->i_sb)) + BHDR(bh)->h_checksum = ext4_xattr_block_csum(inode, + bh->b_blocknr, BHDR(bh)); } static inline const struct xattr_handler * @@ -233,7 +228,7 @@ ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh) if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) || BHDR(bh)->h_blocks != cpu_to_le32(1)) return -EFSCORRUPTED; - if (!ext4_xattr_block_csum_verify(inode, bh->b_blocknr, BHDR(bh))) + if (!ext4_xattr_block_csum_verify(inode, bh)) return -EFSBADCRC; error = ext4_xattr_check_names(BFIRST(bh), bh->b_data + bh->b_size, bh->b_data); @@ -618,23 +613,22 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode, } } + ext4_xattr_block_csum_set(inode, bh); /* * Beware of this ugliness: Releasing of xattr block references * from different inodes can race and so we have to protect * from a race where someone else frees the block (and releases * its journal_head) before we are done dirtying the buffer. In * nojournal mode this race is harmless and we actually cannot - * call ext4_handle_dirty_xattr_block() with locked buffer as + * call ext4_handle_dirty_metadata() with locked buffer as * that function can call sync_dirty_buffer() so for that case * we handle the dirtying after unlocking the buffer. */ if (ext4_handle_valid(handle)) - error = ext4_handle_dirty_xattr_block(handle, inode, - bh); + error = ext4_handle_dirty_metadata(handle, inode, bh); unlock_buffer(bh); if (!ext4_handle_valid(handle)) - error = ext4_handle_dirty_xattr_block(handle, inode, - bh); + error = ext4_handle_dirty_metadata(handle, inode, bh); if (IS_SYNC(inode)) ext4_handle_sync(handle); dquot_free_block(inode, EXT4_C2B(EXT4_SB(inode->i_sb), 1)); @@ -863,13 +857,14 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, ext4_xattr_cache_insert(ext4_mb_cache, bs->bh); } + ext4_xattr_block_csum_set(inode, bs->bh); unlock_buffer(bs->bh); if (error == -EFSCORRUPTED) goto bad_block; if (!error) - error = ext4_handle_dirty_xattr_block(handle, - inode, - bs->bh); + error = ext4_handle_dirty_metadata(handle, + inode, + bs->bh); if (error) goto cleanup; goto inserted; @@ -967,10 +962,11 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, ce->e_reusable = 0; ea_bdebug(new_bh, "reusing; refcount now=%d", ref); + ext4_xattr_block_csum_set(inode, new_bh); unlock_buffer(new_bh); - error = ext4_handle_dirty_xattr_block(handle, - inode, - new_bh); + error = ext4_handle_dirty_metadata(handle, + inode, + new_bh); if (error) goto cleanup_dquot; } @@ -1020,11 +1016,12 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, goto getblk_failed; } memcpy(new_bh->b_data, s->base, new_bh->b_size); + ext4_xattr_block_csum_set(inode, new_bh); set_buffer_uptodate(new_bh); unlock_buffer(new_bh); ext4_xattr_cache_insert(ext4_mb_cache, new_bh); - error = ext4_handle_dirty_xattr_block(handle, - inode, new_bh); + error = ext4_handle_dirty_metadata(handle, inode, + new_bh); if (error) goto cleanup; } -- 2.11.0.rc0.7.gbe5a750 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH -v2] ext4: lock the xattr block before checksuming it 2017-03-25 21:24 ` [PATCH -v2] ext4: lock the xattr block before checksuming it Theodore Ts'o @ 2017-03-26 0:20 ` Colin Ian King 0 siblings, 0 replies; 4+ messages in thread From: Colin Ian King @ 2017-03-26 0:20 UTC (permalink / raw) To: Theodore Ts'o, Ext4 Developers List; +Cc: stable On 25/03/17 21:24, Theodore Ts'o wrote: > We must lock the xattr block before calculating or verifying the > checksum in order to avoid spurious checksum failures. > > https://bugzilla.kernel.org/show_bug.cgi?id=193661 > > Reported-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > Cc: stable@vger.kernel.org > --- > fs/ext4/xattr.c | 65 +++++++++++++++++++++++++++------------------------------ > 1 file changed, 31 insertions(+), 34 deletions(-) > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 67636acf7624..996e7900d4c8 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -131,31 +131,26 @@ static __le32 ext4_xattr_block_csum(struct inode *inode, > } > > static int ext4_xattr_block_csum_verify(struct inode *inode, > - sector_t block_nr, > - struct ext4_xattr_header *hdr) > + struct buffer_head *bh) > { > - if (ext4_has_metadata_csum(inode->i_sb) && > - (hdr->h_checksum != ext4_xattr_block_csum(inode, block_nr, hdr))) > - return 0; > - return 1; > -} > - > -static void ext4_xattr_block_csum_set(struct inode *inode, > - sector_t block_nr, > - struct ext4_xattr_header *hdr) > -{ > - if (!ext4_has_metadata_csum(inode->i_sb)) > - return; > + struct ext4_xattr_header *hdr = BHDR(bh); > + int ret = 1; > > - hdr->h_checksum = ext4_xattr_block_csum(inode, block_nr, hdr); > + if (ext4_has_metadata_csum(inode->i_sb)) { > + lock_buffer(bh); > + ret = (hdr->h_checksum == ext4_xattr_block_csum(inode, > + bh->b_blocknr, hdr)); > + unlock_buffer(bh); > + } > + return ret; > } > > -static inline int ext4_handle_dirty_xattr_block(handle_t *handle, > - struct inode *inode, > - struct buffer_head *bh) > +static void ext4_xattr_block_csum_set(struct inode *inode, > + struct buffer_head *bh) > { > - ext4_xattr_block_csum_set(inode, bh->b_blocknr, BHDR(bh)); > - return ext4_handle_dirty_metadata(handle, inode, bh); > + if (ext4_has_metadata_csum(inode->i_sb)) > + BHDR(bh)->h_checksum = ext4_xattr_block_csum(inode, > + bh->b_blocknr, BHDR(bh)); > } > > static inline const struct xattr_handler * > @@ -233,7 +228,7 @@ ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh) > if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) || > BHDR(bh)->h_blocks != cpu_to_le32(1)) > return -EFSCORRUPTED; > - if (!ext4_xattr_block_csum_verify(inode, bh->b_blocknr, BHDR(bh))) > + if (!ext4_xattr_block_csum_verify(inode, bh)) > return -EFSBADCRC; > error = ext4_xattr_check_names(BFIRST(bh), bh->b_data + bh->b_size, > bh->b_data); > @@ -618,23 +613,22 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode, > } > } > > + ext4_xattr_block_csum_set(inode, bh); > /* > * Beware of this ugliness: Releasing of xattr block references > * from different inodes can race and so we have to protect > * from a race where someone else frees the block (and releases > * its journal_head) before we are done dirtying the buffer. In > * nojournal mode this race is harmless and we actually cannot > - * call ext4_handle_dirty_xattr_block() with locked buffer as > + * call ext4_handle_dirty_metadata() with locked buffer as > * that function can call sync_dirty_buffer() so for that case > * we handle the dirtying after unlocking the buffer. > */ > if (ext4_handle_valid(handle)) > - error = ext4_handle_dirty_xattr_block(handle, inode, > - bh); > + error = ext4_handle_dirty_metadata(handle, inode, bh); > unlock_buffer(bh); > if (!ext4_handle_valid(handle)) > - error = ext4_handle_dirty_xattr_block(handle, inode, > - bh); > + error = ext4_handle_dirty_metadata(handle, inode, bh); > if (IS_SYNC(inode)) > ext4_handle_sync(handle); > dquot_free_block(inode, EXT4_C2B(EXT4_SB(inode->i_sb), 1)); > @@ -863,13 +857,14 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, > ext4_xattr_cache_insert(ext4_mb_cache, > bs->bh); > } > + ext4_xattr_block_csum_set(inode, bs->bh); > unlock_buffer(bs->bh); > if (error == -EFSCORRUPTED) > goto bad_block; > if (!error) > - error = ext4_handle_dirty_xattr_block(handle, > - inode, > - bs->bh); > + error = ext4_handle_dirty_metadata(handle, > + inode, > + bs->bh); > if (error) > goto cleanup; > goto inserted; > @@ -967,10 +962,11 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, > ce->e_reusable = 0; > ea_bdebug(new_bh, "reusing; refcount now=%d", > ref); > + ext4_xattr_block_csum_set(inode, new_bh); > unlock_buffer(new_bh); > - error = ext4_handle_dirty_xattr_block(handle, > - inode, > - new_bh); > + error = ext4_handle_dirty_metadata(handle, > + inode, > + new_bh); > if (error) > goto cleanup_dquot; > } > @@ -1020,11 +1016,12 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, > goto getblk_failed; > } > memcpy(new_bh->b_data, s->base, new_bh->b_size); > + ext4_xattr_block_csum_set(inode, new_bh); > set_buffer_uptodate(new_bh); > unlock_buffer(new_bh); > ext4_xattr_cache_insert(ext4_mb_cache, new_bh); > - error = ext4_handle_dirty_xattr_block(handle, > - inode, new_bh); > + error = ext4_handle_dirty_metadata(handle, inode, > + new_bh); > if (error) > goto cleanup; > } > I've given this a good soak test on 32 bit and 64 bit x86 builds and it fixes the issue. Thanks Ted. Tested-by: Colin Ian King <colin.king@canonical.com> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-26 0:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-28 13:56 [PATCH] ext4: lock the the xattr block before calculating its checksum Theodore Ts'o 2017-02-28 16:12 ` Darrick J. Wong 2017-03-25 21:24 ` [PATCH -v2] ext4: lock the xattr block before checksuming it Theodore Ts'o 2017-03-26 0:20 ` Colin Ian King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox