From: Baokun Li <[email protected]>
Baokun Li (2):
ext4: fix mb_cache_entry's e_refcnt leak in
ext4_xattr_block_cache_find()
ext4: propagate errors from ext4_sb_bread() in
ext4_xattr_block_cache_find()
fs/ext4/xattr.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
--
2.39.2
From: Baokun Li <[email protected]>
In ext4_xattr_block_cache_find(), when ext4_sb_bread() returns an error,
we will either continue to find the next ea block or return NULL to try to
insert a new ea block. But whether ext4_sb_bread() returns -EIO or -ENOMEM,
the next operation is most likely to fail with the same error. So propagate
the error returned by ext4_sb_bread() to make ext4_xattr_block_set() fail
to reduce pointless operations.
Signed-off-by: Baokun Li <[email protected]>
---
fs/ext4/xattr.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 9fdd13422073..11742e1f16d7 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2059,8 +2059,13 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
inserted:
if (!IS_LAST_ENTRY(s->first)) {
- new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
- &ce);
+ new_bh = ext4_xattr_block_cache_find(inode, header(s->base), &ce);
+ if (IS_ERR(new_bh)) {
+ error = PTR_ERR(new_bh);
+ new_bh = NULL;
+ goto cleanup;
+ }
+
if (new_bh) {
/* We found an identical block in the cache. */
if (new_bh == bs->bh)
@@ -3090,8 +3095,8 @@ ext4_xattr_cmp(struct ext4_xattr_header *header1,
*
* Find an identical extended attribute block.
*
- * Returns a pointer to the block found, or NULL if such a block was
- * not found or an error occurred.
+ * Returns a pointer to the block found, or NULL if such a block was not
+ * found, or an error pointer if an error occurred while reading ea block.
*/
static struct buffer_head *
ext4_xattr_block_cache_find(struct inode *inode,
@@ -3113,13 +3118,11 @@ ext4_xattr_block_cache_find(struct inode *inode,
bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
if (IS_ERR(bh)) {
- if (PTR_ERR(bh) == -ENOMEM) {
- mb_cache_entry_put(ea_block_cache, ce);
- return NULL;
- }
- bh = NULL;
- EXT4_ERROR_INODE(inode, "block %lu read error",
- (unsigned long)ce->e_value);
+ if (PTR_ERR(bh) != -ENOMEM)
+ EXT4_ERROR_INODE(inode, "block %lu read error",
+ (unsigned long)ce->e_value);
+ mb_cache_entry_put(ea_block_cache, ce);
+ return bh;
} else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
*pce = ce;
return bh;
--
2.39.2
From: Baokun Li <[email protected]>
Syzbot reports a warning as follows:
============================================
WARNING: CPU: 0 PID: 5075 at fs/mbcache.c:419 mb_cache_destroy+0x224/0x290
Modules linked in:
CPU: 0 PID: 5075 Comm: syz-executor199 Not tainted 6.9.0-rc6-gb947cc5bf6d7
RIP: 0010:mb_cache_destroy+0x224/0x290 fs/mbcache.c:419
Call Trace:
<TASK>
ext4_put_super+0x6d4/0xcd0 fs/ext4/super.c:1375
generic_shutdown_super+0x136/0x2d0 fs/super.c:641
kill_block_super+0x44/0x90 fs/super.c:1675
ext4_kill_sb+0x68/0xa0 fs/ext4/super.c:7327
[...]
============================================
This is because when finding an entry in ext4_xattr_block_cache_find(), if
ext4_sb_bread() returns -ENOMEM, the ce's e_refcnt, which has already grown
in the __entry_find(), won't be put away, and eventually trigger the above
issue in mb_cache_destroy() due to reference count leakage.
So call mb_cache_entry_put() on the -ENOMEM error branch as a quick fix.
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=dd43bd0f7474512edc47
Fixes: fb265c9cb49e ("ext4: add ext4_sb_bread() to disambiguate ENOMEM cases")
Cc: [email protected]
Signed-off-by: Baokun Li <[email protected]>
---
fs/ext4/xattr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index b67a176bfcf9..9fdd13422073 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -3113,8 +3113,10 @@ ext4_xattr_block_cache_find(struct inode *inode,
bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
if (IS_ERR(bh)) {
- if (PTR_ERR(bh) == -ENOMEM)
+ if (PTR_ERR(bh) == -ENOMEM) {
+ mb_cache_entry_put(ea_block_cache, ce);
return NULL;
+ }
bh = NULL;
EXT4_ERROR_INODE(inode, "block %lu read error",
(unsigned long)ce->e_value);
--
2.39.2
On Sat 04-05-24 15:55:25, [email protected] wrote:
> From: Baokun Li <[email protected]>
>
> Syzbot reports a warning as follows:
>
> ============================================
> WARNING: CPU: 0 PID: 5075 at fs/mbcache.c:419 mb_cache_destroy+0x224/0x290
> Modules linked in:
> CPU: 0 PID: 5075 Comm: syz-executor199 Not tainted 6.9.0-rc6-gb947cc5bf6d7
> RIP: 0010:mb_cache_destroy+0x224/0x290 fs/mbcache.c:419
> Call Trace:
> <TASK>
> ext4_put_super+0x6d4/0xcd0 fs/ext4/super.c:1375
> generic_shutdown_super+0x136/0x2d0 fs/super.c:641
> kill_block_super+0x44/0x90 fs/super.c:1675
> ext4_kill_sb+0x68/0xa0 fs/ext4/super.c:7327
> [...]
> ============================================
>
> This is because when finding an entry in ext4_xattr_block_cache_find(), if
> ext4_sb_bread() returns -ENOMEM, the ce's e_refcnt, which has already grown
> in the __entry_find(), won't be put away, and eventually trigger the above
> issue in mb_cache_destroy() due to reference count leakage.
>
> So call mb_cache_entry_put() on the -ENOMEM error branch as a quick fix.
>
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=dd43bd0f7474512edc47
> Fixes: fb265c9cb49e ("ext4: add ext4_sb_bread() to disambiguate ENOMEM cases")
> Cc: [email protected]
> Signed-off-by: Baokun Li <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/ext4/xattr.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index b67a176bfcf9..9fdd13422073 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -3113,8 +3113,10 @@ ext4_xattr_block_cache_find(struct inode *inode,
>
> bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
> if (IS_ERR(bh)) {
> - if (PTR_ERR(bh) == -ENOMEM)
> + if (PTR_ERR(bh) == -ENOMEM) {
> + mb_cache_entry_put(ea_block_cache, ce);
> return NULL;
> + }
> bh = NULL;
> EXT4_ERROR_INODE(inode, "block %lu read error",
> (unsigned long)ce->e_value);
> --
> 2.39.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat 04-05-24 15:55:26, [email protected] wrote:
> From: Baokun Li <[email protected]>
>
> In ext4_xattr_block_cache_find(), when ext4_sb_bread() returns an error,
> we will either continue to find the next ea block or return NULL to try to
> insert a new ea block. But whether ext4_sb_bread() returns -EIO or -ENOMEM,
> the next operation is most likely to fail with the same error. So propagate
> the error returned by ext4_sb_bread() to make ext4_xattr_block_set() fail
> to reduce pointless operations.
>
> Signed-off-by: Baokun Li <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/ext4/xattr.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 9fdd13422073..11742e1f16d7 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2059,8 +2059,13 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>
> inserted:
> if (!IS_LAST_ENTRY(s->first)) {
> - new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
> - &ce);
> + new_bh = ext4_xattr_block_cache_find(inode, header(s->base), &ce);
> + if (IS_ERR(new_bh)) {
> + error = PTR_ERR(new_bh);
> + new_bh = NULL;
> + goto cleanup;
> + }
> +
> if (new_bh) {
> /* We found an identical block in the cache. */
> if (new_bh == bs->bh)
> @@ -3090,8 +3095,8 @@ ext4_xattr_cmp(struct ext4_xattr_header *header1,
> *
> * Find an identical extended attribute block.
> *
> - * Returns a pointer to the block found, or NULL if such a block was
> - * not found or an error occurred.
> + * Returns a pointer to the block found, or NULL if such a block was not
> + * found, or an error pointer if an error occurred while reading ea block.
> */
> static struct buffer_head *
> ext4_xattr_block_cache_find(struct inode *inode,
> @@ -3113,13 +3118,11 @@ ext4_xattr_block_cache_find(struct inode *inode,
>
> bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
> if (IS_ERR(bh)) {
> - if (PTR_ERR(bh) == -ENOMEM) {
> - mb_cache_entry_put(ea_block_cache, ce);
> - return NULL;
> - }
> - bh = NULL;
> - EXT4_ERROR_INODE(inode, "block %lu read error",
> - (unsigned long)ce->e_value);
> + if (PTR_ERR(bh) != -ENOMEM)
> + EXT4_ERROR_INODE(inode, "block %lu read error",
> + (unsigned long)ce->e_value);
> + mb_cache_entry_put(ea_block_cache, ce);
> + return bh;
> } else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
> *pce = ce;
> return bh;
> --
> 2.39.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat, 04 May 2024 15:55:24 +0800, [email protected] wrote:
> Baokun Li (2):
> ext4: fix mb_cache_entry's e_refcnt leak in
> ext4_xattr_block_cache_find()
> ext4: propagate errors from ext4_sb_bread() in
> ext4_xattr_block_cache_find()
>
> fs/ext4/xattr.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> [...]
Applied, thanks!
[1/2] ext4: fix mb_cache_entry's e_refcnt leak in ext4_xattr_block_cache_find()
commit: 0c0b4a49d3e7f49690a6827a41faeffad5df7e21
[2/2] ext4: propagate errors from ext4_sb_bread() in ext4_xattr_block_cache_find()
commit: dc1c4663bc493f323d6b2f9dd55c044ea920dacf
Best regards,
--
Theodore Ts'o <[email protected]>