[syzbot] [ext4?] WARNING in mb_cache_destroy

[syzbot] [ext4?] WARNING in mb_cache_destroy

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    b947cc5bf6d7 Merge tag 'erofs-for-6.9-rc7-fixes' of git://..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=11175d5f180000
kernel config:  https://syzkaller.appspot.com/x/.config?x=d2f00edef461175
dashboard link: https://syzkaller.appspot.com/bug?extid=dd43bd0f7474512edc47
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11d2957f180000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1620ca40980000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/7318118d629d/disk-b947cc5b.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/88b2ce2fc8ea/vmlinux-b947cc5b.xz
kernel image: https://storage.googleapis.com/syzbot-assets/3f3ffc239871/bzImage-b947cc5b.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/224b657d209f/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+dd43bd0f7474512edc47@syzkaller.appspotmail.com

loop0: detected capacity change from 512 to 64
EXT4-fs (loop0): unmounting filesystem 00000000-0000-0000-0000-000000000000.
------------[ cut here ]------------
WARNING: CPU: 0 PID: 5075 at fs/mbcache.c:419 mb_cache_destroy+0x224/0x290 fs/mbcache.c:419
Modules linked in:
CPU: 0 PID: 5075 Comm: syz-executor199 Not tainted 6.9.0-rc6-syzkaller-00005-gb947cc5bf6d7 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
RIP: 0010:mb_cache_destroy+0x224/0x290 fs/mbcache.c:419
Code: 24 08 4c 89 f6 e8 9c e6 ff ff eb 05 e8 45 3b 6e ff 4c 8b 34 24 49 39 ee 74 33 e8 37 3b 6e ff e9 6a fe ff ff e8 2d 3b 6e ff 90 <0f> 0b 90 eb 83 44 89 e9 80 e1 07 80 c1 03 38 c1 0f 8c 58 ff ff ff
RSP: 0018:ffffc90003677a88 EFLAGS: 00010293
RAX: ffffffff8227d393 RBX: 0000000000000002 RCX: ffff88807c9ebc00
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 0000000000000001
RBP: ffff88801aeb3858 R08: ffffffff8227d312 R09: 1ffff1100dd2e204
R10: dffffc0000000000 R11: ffffed100dd2e205 R12: 1ffff1100dd2e200
R13: ffff88806e971020 R14: ffff88806e971000 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff8880b9400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055ec96f85460 CR3: 000000000e134000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
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
 deactivate_locked_super+0xc4/0x130 fs/super.c:472
 cleanup_mnt+0x426/0x4c0 fs/namespace.c:1267
 task_work_run+0x24f/0x310 kernel/task_work.c:180
 exit_task_work include/linux/task_work.h:38 [inline]
 do_exit+0xa1b/0x27e0 kernel/exit.c:878
 do_group_exit+0x207/0x2c0 kernel/exit.c:1027
 __do_sys_exit_group kernel/exit.c:1038 [inline]
 __se_sys_exit_group kernel/exit.c:1036 [inline]
 __x64_sys_exit_group+0x3f/0x40 kernel/exit.c:1036
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f4207bbec89
Code: Unable to access opcode bytes at 0x7f4207bbec5f.
RSP: 002b:00007ffd518b18c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f4207bbec89
RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000001
RBP: 00007f4207c3b390 R08: ffffffffffffffb8 R09: 00007ffd518b19a0
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f4207c3b390
R13: 0000000000000000 R14: 00007f4207c3c100 R15: 00007f4207b8cf60
 </TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[syzbot]

syzbot has bisected this issue to:

commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
Author: Baokun Li <libaokun1@huawei.com>
Date:   Thu Jun 16 02:13:56 2022 +0000

    ext4: fix use-after-free in ext4_xattr_set_entry

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1011bcf8980000
start commit:   b947cc5bf6d7 Merge tag 'erofs-for-6.9-rc7-fixes' of git://..
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=1211bcf8980000
console output: https://syzkaller.appspot.com/x/log.txt?x=1411bcf8980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=d2f00edef461175
dashboard link: https://syzkaller.appspot.com/bug?extid=dd43bd0f7474512edc47
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11d2957f180000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1620ca40980000

Reported-by: syzbot+dd43bd0f7474512edc47@syzkaller.appspotmail.com
Fixes: 67d7d8ad99be ("ext4: fix use-after-free in ext4_xattr_set_entry")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[Jan Kara]

On Tue 30-04-24 08:04:03, syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
> Author: Baokun Li <libaokun1@huawei.com>
> Date:   Thu Jun 16 02:13:56 2022 +0000
> 
>     ext4: fix use-after-free in ext4_xattr_set_entry

So I'm not sure the bisect is correct since the change is looking harmless.
But it is sufficiently related that there indeed may be some relationship.
Anyway, the kernel log has:

[   44.932900][ T1063] EXT4-fs warning (device loop0): ext4_evict_inode:297: xattr delete (err -12)
[   44.943316][ T1063] EXT4-fs (loop0): unmounting filesystem.
[   44.949531][ T1063] ------------[ cut here ]------------
[   44.955050][ T1063] WARNING: CPU: 0 PID: 1063 at fs/mbcache.c:409 mb_cache_destroy+0xda/0x110

So ext4_xattr_delete_inode() called when removing inode has failed with
ENOMEM and later mb_cache_destroy() was eventually complaining about having
mbcache entry with increased refcount. So likely some error cleanup path is
forgetting to drop mbcache entry reference somewhere but at this point I
cannot find where. We'll likely need to play with the reproducer to debug
that. Baokun, any chance for looking into this?

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

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[Baokun Li]

On 2024/5/2 18:33, Jan Kara wrote:
> On Tue 30-04-24 08:04:03, syzbot wrote:
>> syzbot has bisected this issue to:
>>
>> commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
>> Author: Baokun Li <libaokun1@huawei.com>
>> Date:   Thu Jun 16 02:13:56 2022 +0000
>>
>>      ext4: fix use-after-free in ext4_xattr_set_entry
> So I'm not sure the bisect is correct since the change is looking harmless.
> But it is sufficiently related that there indeed may be some relationship.
> Anyway, the kernel log has:
>
> [   44.932900][ T1063] EXT4-fs warning (device loop0): ext4_evict_inode:297: xattr delete (err -12)
> [   44.943316][ T1063] EXT4-fs (loop0): unmounting filesystem.
> [   44.949531][ T1063] ------------[ cut here ]------------
> [   44.955050][ T1063] WARNING: CPU: 0 PID: 1063 at fs/mbcache.c:409 mb_cache_destroy+0xda/0x110
>
> So ext4_xattr_delete_inode() called when removing inode has failed with
> ENOMEM and later mb_cache_destroy() was eventually complaining about having
> mbcache entry with increased refcount. So likely some error cleanup path is
> forgetting to drop mbcache entry reference somewhere but at this point I
> cannot find where. We'll likely need to play with the reproducer to debug
> that. Baokun, any chance for looking into this?
>
> 								Honza

Hi Honza,

Sorry for the late reply, the first two days were May Day holidays.
Thanks for the heads up, I'll take a look at this soon.

Regards,
Baokun

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[Baokun Li]

Hi Honza,

On 2024/5/2 18:33, Jan Kara wrote:
> On Tue 30-04-24 08:04:03, syzbot wrote:
>> syzbot has bisected this issue to:
>>
>> commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
>> Author: Baokun Li <libaokun1@huawei.com>
>> Date:   Thu Jun 16 02:13:56 2022 +0000
>>
>>      ext4: fix use-after-free in ext4_xattr_set_entry
> So I'm not sure the bisect is correct since the change is looking harmless.
Yes, the root cause of the problem has nothing to do with this patch,
and please see the detailed analysis below.
> But it is sufficiently related that there indeed may be some relationship.
> Anyway, the kernel log has:
>
> [   44.932900][ T1063] EXT4-fs warning (device loop0): ext4_evict_inode:297: xattr delete (err -12)
> [   44.943316][ T1063] EXT4-fs (loop0): unmounting filesystem.
> [   44.949531][ T1063] ------------[ cut here ]------------
> [   44.955050][ T1063] WARNING: CPU: 0 PID: 1063 at fs/mbcache.c:409 mb_cache_destroy+0xda/0x110
>
> So ext4_xattr_delete_inode() called when removing inode has failed with
> ENOMEM and later mb_cache_destroy() was eventually complaining about having
> mbcache entry with increased refcount. So likely some error cleanup path is
> forgetting to drop mbcache entry reference somewhere but at this point I
> cannot find where. We'll likely need to play with the reproducer to debug
> that. Baokun, any chance for looking into this?
>
> 								Honza
As you guessed, when -ENOMEM is returned in ext4_sb_bread(),
the reference count of ce is not properly released, as follows.

ext4_create
  __ext4_new_inode
   security_inode_init_security
    ext4_initxattrs
     ext4_xattr_set_handle
      ext4_xattr_block_find
      ext4_xattr_block_set
       ext4_xattr_block_cache_find
         ce = mb_cache_entry_find_first
             __entry_find
             atomic_inc_not_zero(&entry->e_refcnt)
         bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
         if (PTR_ERR(bh) == -ENOMEM)
             return NULL;

Before merging into commit 67d7d8ad99be("ext4: fix use-after-free
in ext4_xattr_set_entry"), it will not return early in 
ext4_xattr_ibody_find(),
so it tries to find it in iboy, fails the check in xattr_check_inode() and
returns without executing ext4_xattr_block_find(). Thus it will bisect
the patch, but actually has nothing to do with it.

ext4_xattr_ibody_get
  xattr_check_inode
   __xattr_check_inode
    check_xattrs
     if (end - (void *)header < sizeof(*header) + sizeof(u32))
       "in-inode xattr block too small"

Here's the patch in testing, I'll send it out officially after it is tested.
(PS:  I'm not sure if propagating the ext4_xattr_block_cache_find() 
errors would be better.)

Regards,
Baokun


From: Baokun Li <libaokun1@huawei.com>
Date: Fri, 3 May 2024 16:51:43 +0800
Subject: [PATCH] ext4: fix mb_cache_entry's e_refcnt leak in
  ext4_xattr_block_cache_find()

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 correct the
handling of the -ENOMEM error branch to avoid the above issue.

Reported-by: syzbot+dd43bd0f7474512edc47@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=dd43bd0f7474512edc47
Fixes: fb265c9cb49e ("ext4: add ext4_sb_bread() to disambiguate ENOMEM 
cases")
Cc: stable@kernel.org # v5.0-rc1
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
  fs/ext4/xattr.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index b67a176bfcf9..5c9e751915fd 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -3113,11 +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)
-                return NULL;
+            if (PTR_ERR(bh) != -ENOMEM)
+                EXT4_ERROR_INODE(inode, "block %lu read error",
+                         (unsigned long)ce->e_value);
              bh = NULL;
-            EXT4_ERROR_INODE(inode, "block %lu read error",
-                     (unsigned long)ce->e_value);
          } else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
              *pce = ce;
              return bh;
-- 
2.39.2

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[Jan Kara]

Hi!

On Fri 03-05-24 17:51:07, Baokun Li wrote:
> On 2024/5/2 18:33, Jan Kara wrote:
> > On Tue 30-04-24 08:04:03, syzbot wrote:
> > > syzbot has bisected this issue to:
> > > 
> > > commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
> > > Author: Baokun Li <libaokun1@huawei.com>
> > > Date:   Thu Jun 16 02:13:56 2022 +0000
> > > 
> > >      ext4: fix use-after-free in ext4_xattr_set_entry
> > So I'm not sure the bisect is correct since the change is looking harmless.
> Yes, the root cause of the problem has nothing to do with this patch,
> and please see the detailed analysis below.
> > But it is sufficiently related that there indeed may be some relationship.
> > Anyway, the kernel log has:
> > 
> > [   44.932900][ T1063] EXT4-fs warning (device loop0): ext4_evict_inode:297: xattr delete (err -12)
> > [   44.943316][ T1063] EXT4-fs (loop0): unmounting filesystem.
> > [   44.949531][ T1063] ------------[ cut here ]------------
> > [   44.955050][ T1063] WARNING: CPU: 0 PID: 1063 at fs/mbcache.c:409 mb_cache_destroy+0xda/0x110
> > 
> > So ext4_xattr_delete_inode() called when removing inode has failed with
> > ENOMEM and later mb_cache_destroy() was eventually complaining about having
> > mbcache entry with increased refcount. So likely some error cleanup path is
> > forgetting to drop mbcache entry reference somewhere but at this point I
> > cannot find where. We'll likely need to play with the reproducer to debug
> > that. Baokun, any chance for looking into this?
> > 
> > 								Honza
> As you guessed, when -ENOMEM is returned in ext4_sb_bread(),
> the reference count of ce is not properly released, as follows.
> 
> ext4_create
>  __ext4_new_inode
>   security_inode_init_security
>    ext4_initxattrs
>     ext4_xattr_set_handle
>      ext4_xattr_block_find
>      ext4_xattr_block_set
>       ext4_xattr_block_cache_find
>         ce = mb_cache_entry_find_first
>             __entry_find
>             atomic_inc_not_zero(&entry->e_refcnt)
>         bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
>         if (PTR_ERR(bh) == -ENOMEM)
>             return NULL;
> 
> Before merging into commit 67d7d8ad99be("ext4: fix use-after-free
> in ext4_xattr_set_entry"), it will not return early in
> ext4_xattr_ibody_find(),
> so it tries to find it in iboy, fails the check in xattr_check_inode() and
> returns without executing ext4_xattr_block_find(). Thus it will bisect
> the patch, but actually has nothing to do with it.
> 
> ext4_xattr_ibody_get
>  xattr_check_inode
>   __xattr_check_inode
>    check_xattrs
>     if (end - (void *)header < sizeof(*header) + sizeof(u32))
>       "in-inode xattr block too small"
> 
> Here's the patch in testing, I'll send it out officially after it is tested.
> (PS:  I'm not sure if propagating the ext4_xattr_block_cache_find() errors
> would be better.)

Great! Thanks for debugging this! Some comments to your fix below:

> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index b67a176bfcf9..5c9e751915fd 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -3113,11 +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)
> -                return NULL;
> +            if (PTR_ERR(bh) != -ENOMEM)
> +                EXT4_ERROR_INODE(inode, "block %lu read error",
> +                         (unsigned long)ce->e_value);
>              bh = NULL;
> -            EXT4_ERROR_INODE(inode, "block %lu read error",
> -                     (unsigned long)ce->e_value);
>          } else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
>              *pce = ce;
>              return bh;

So if we get the ENOMEM error, continuing the iteration seems to be
pointless as we'll likely get it for the following entries as well. I think
the original behavior of aborting the iteration in case of ENOMEM is
actually better. We just have to do mb_cache_entry_put(ea_block_cache, ce)
before returning...

								Honza

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

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[Baokun Li]

On 2024/5/3 18:23, Jan Kara wrote:
> Hi!
>
> On Fri 03-05-24 17:51:07, Baokun Li wrote:
>> On 2024/5/2 18:33, Jan Kara wrote:
>>> On Tue 30-04-24 08:04:03, syzbot wrote:
>>>> syzbot has bisected this issue to:
>>>>
>>>> commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
>>>> Author: Baokun Li <libaokun1@huawei.com>
>>>> Date:   Thu Jun 16 02:13:56 2022 +0000
>>>>
>>>>       ext4: fix use-after-free in ext4_xattr_set_entry
>>> So I'm not sure the bisect is correct since the change is looking harmless.
>> Yes, the root cause of the problem has nothing to do with this patch,
>> and please see the detailed analysis below.
>>> But it is sufficiently related that there indeed may be some relationship.
>>> Anyway, the kernel log has:
>>>
>>> [   44.932900][ T1063] EXT4-fs warning (device loop0): ext4_evict_inode:297: xattr delete (err -12)
>>> [   44.943316][ T1063] EXT4-fs (loop0): unmounting filesystem.
>>> [   44.949531][ T1063] ------------[ cut here ]------------
>>> [   44.955050][ T1063] WARNING: CPU: 0 PID: 1063 at fs/mbcache.c:409 mb_cache_destroy+0xda/0x110
>>>
>>> So ext4_xattr_delete_inode() called when removing inode has failed with
>>> ENOMEM and later mb_cache_destroy() was eventually complaining about having
>>> mbcache entry with increased refcount. So likely some error cleanup path is
>>> forgetting to drop mbcache entry reference somewhere but at this point I
>>> cannot find where. We'll likely need to play with the reproducer to debug
>>> that. Baokun, any chance for looking into this?
>>>
>>> 								Honza
>> As you guessed, when -ENOMEM is returned in ext4_sb_bread(),
>> the reference count of ce is not properly released, as follows.
>>
>> ext4_create
>>   __ext4_new_inode
>>    security_inode_init_security
>>     ext4_initxattrs
>>      ext4_xattr_set_handle
>>       ext4_xattr_block_find
>>       ext4_xattr_block_set
>>        ext4_xattr_block_cache_find
>>          ce = mb_cache_entry_find_first
>>              __entry_find
>>              atomic_inc_not_zero(&entry->e_refcnt)
>>          bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
>>          if (PTR_ERR(bh) == -ENOMEM)
>>              return NULL;
>>
>> Before merging into commit 67d7d8ad99be("ext4: fix use-after-free
>> in ext4_xattr_set_entry"), it will not return early in
>> ext4_xattr_ibody_find(),
>> so it tries to find it in iboy, fails the check in xattr_check_inode() and
>> returns without executing ext4_xattr_block_find(). Thus it will bisect
>> the patch, but actually has nothing to do with it.
>>
>> ext4_xattr_ibody_get
>>   xattr_check_inode
>>    __xattr_check_inode
>>     check_xattrs
>>      if (end - (void *)header < sizeof(*header) + sizeof(u32))
>>        "in-inode xattr block too small"
>>
>> Here's the patch in testing, I'll send it out officially after it is tested.
>> (PS:  I'm not sure if propagating the ext4_xattr_block_cache_find() errors
>> would be better.)
> Great! Thanks for debugging this! Some comments to your fix below:
>
>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>> index b67a176bfcf9..5c9e751915fd 100644
>> --- a/fs/ext4/xattr.c
>> +++ b/fs/ext4/xattr.c
>> @@ -3113,11 +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)
>> -                return NULL;
>> +            if (PTR_ERR(bh) != -ENOMEM)
>> +                EXT4_ERROR_INODE(inode, "block %lu read error",
>> +                         (unsigned long)ce->e_value);
>>               bh = NULL;
>> -            EXT4_ERROR_INODE(inode, "block %lu read error",
>> -                     (unsigned long)ce->e_value);
>>           } else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
>>               *pce = ce;
>>               return bh;
> So if we get the ENOMEM error, continuing the iteration seems to be
> pointless as we'll likely get it for the following entries as well. I think
> the original behavior of aborting the iteration in case of ENOMEM is
> actually better. We just have to do mb_cache_entry_put(ea_block_cache, ce)
> before returning...
>
> 								Honza
Returning NULL here would normally attempt to allocate a new
xattr_block in ext4_xattr_block_set(), and when ext4_sb_bread() fails,
allocating the new block and inserting it would most likely fail as well,
so my initial thought was to propagate the error from ext4_sb_bread()
to also make ext4_xattr_block_set() fail when ext4_sb_bread() fails.

But I noticed that before Ted added the special handling for -ENOMEM,
EXT4_ERROR_INODE was called to set the ERROR_FS flag no matter
what error ext4_sb_bread() returned, and after we can distinguish
between -EIO and -ENOMEM, we don't have to set the ERROR_FS flag
in the case of -ENOMEM. So there's this conservative fix now.

In short, in my personal opinion, for -EIO and -ENOMEM, they should
be the same except whether or not the ERROR_FS flag is set.
Otherwise, I think adding mb_cache_entry_put() directly is the easiest
and most straightforward fix.  Honza, do you have any other thoughts?

Thanks,
Baokun

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[Jan Kara]

On Fri 03-05-24 19:38:21, Baokun Li wrote:
> On 2024/5/3 18:23, Jan Kara wrote:
> > Hi!
> > 
> > On Fri 03-05-24 17:51:07, Baokun Li wrote:
> > > On 2024/5/2 18:33, Jan Kara wrote:
> > > > On Tue 30-04-24 08:04:03, syzbot wrote:
> > > > > syzbot has bisected this issue to:
> > > > > 
> > > > > commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
> > > > > Author: Baokun Li <libaokun1@huawei.com>
> > > > > Date:   Thu Jun 16 02:13:56 2022 +0000
> > > > > 
> > > > >       ext4: fix use-after-free in ext4_xattr_set_entry
> > > > So I'm not sure the bisect is correct since the change is looking harmless.
> > > Yes, the root cause of the problem has nothing to do with this patch,
> > > and please see the detailed analysis below.
> > > > But it is sufficiently related that there indeed may be some relationship.
> > > > Anyway, the kernel log has:
> > > > 
> > > > [   44.932900][ T1063] EXT4-fs warning (device loop0): ext4_evict_inode:297: xattr delete (err -12)
> > > > [   44.943316][ T1063] EXT4-fs (loop0): unmounting filesystem.
> > > > [   44.949531][ T1063] ------------[ cut here ]------------
> > > > [   44.955050][ T1063] WARNING: CPU: 0 PID: 1063 at fs/mbcache.c:409 mb_cache_destroy+0xda/0x110
> > > > 
> > > > So ext4_xattr_delete_inode() called when removing inode has failed with
> > > > ENOMEM and later mb_cache_destroy() was eventually complaining about having
> > > > mbcache entry with increased refcount. So likely some error cleanup path is
> > > > forgetting to drop mbcache entry reference somewhere but at this point I
> > > > cannot find where. We'll likely need to play with the reproducer to debug
> > > > that. Baokun, any chance for looking into this?
> > > > 
> > > > 								Honza
> > > As you guessed, when -ENOMEM is returned in ext4_sb_bread(),
> > > the reference count of ce is not properly released, as follows.
> > > 
> > > ext4_create
> > >   __ext4_new_inode
> > >    security_inode_init_security
> > >     ext4_initxattrs
> > >      ext4_xattr_set_handle
> > >       ext4_xattr_block_find
> > >       ext4_xattr_block_set
> > >        ext4_xattr_block_cache_find
> > >          ce = mb_cache_entry_find_first
> > >              __entry_find
> > >              atomic_inc_not_zero(&entry->e_refcnt)
> > >          bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
> > >          if (PTR_ERR(bh) == -ENOMEM)
> > >              return NULL;
> > > 
> > > Before merging into commit 67d7d8ad99be("ext4: fix use-after-free
> > > in ext4_xattr_set_entry"), it will not return early in
> > > ext4_xattr_ibody_find(),
> > > so it tries to find it in iboy, fails the check in xattr_check_inode() and
> > > returns without executing ext4_xattr_block_find(). Thus it will bisect
> > > the patch, but actually has nothing to do with it.
> > > 
> > > ext4_xattr_ibody_get
> > >   xattr_check_inode
> > >    __xattr_check_inode
> > >     check_xattrs
> > >      if (end - (void *)header < sizeof(*header) + sizeof(u32))
> > >        "in-inode xattr block too small"
> > > 
> > > Here's the patch in testing, I'll send it out officially after it is tested.
> > > (PS:  I'm not sure if propagating the ext4_xattr_block_cache_find() errors
> > > would be better.)
> > Great! Thanks for debugging this! Some comments to your fix below:
> > 
> > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > > index b67a176bfcf9..5c9e751915fd 100644
> > > --- a/fs/ext4/xattr.c
> > > +++ b/fs/ext4/xattr.c
> > > @@ -3113,11 +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)
> > > -                return NULL;
> > > +            if (PTR_ERR(bh) != -ENOMEM)
> > > +                EXT4_ERROR_INODE(inode, "block %lu read error",
> > > +                         (unsigned long)ce->e_value);
> > >               bh = NULL;
> > > -            EXT4_ERROR_INODE(inode, "block %lu read error",
> > > -                     (unsigned long)ce->e_value);
> > >           } else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
> > >               *pce = ce;
> > >               return bh;
> > So if we get the ENOMEM error, continuing the iteration seems to be
> > pointless as we'll likely get it for the following entries as well. I think
> > the original behavior of aborting the iteration in case of ENOMEM is
> > actually better. We just have to do mb_cache_entry_put(ea_block_cache, ce)
> > before returning...
> > 
> > 								Honza
> Returning NULL here would normally attempt to allocate a new
> xattr_block in ext4_xattr_block_set(), and when ext4_sb_bread() fails,
> allocating the new block and inserting it would most likely fail as well,
> so my initial thought was to propagate the error from ext4_sb_bread()
> to also make ext4_xattr_block_set() fail when ext4_sb_bread() fails.

Yes, this would be probably even better solution.

> But I noticed that before Ted added the special handling for -ENOMEM,
> EXT4_ERROR_INODE was called to set the ERROR_FS flag no matter
> what error ext4_sb_bread() returned, and after we can distinguish
> between -EIO and -ENOMEM, we don't have to set the ERROR_FS flag
> in the case of -ENOMEM. So there's this conservative fix now.
> 
> In short, in my personal opinion, for -EIO and -ENOMEM, they should
> be the same except whether or not the ERROR_FS flag is set.
> Otherwise, I think adding mb_cache_entry_put() directly is the easiest
> and most straightforward fix.  Honza, do you have any other thoughts?

Yeah. I'd go for adding mb_cache_entry_put() now as a quick fix and then
work on propagating the error from ext4_xattr_block_cache_find() as a
cleaner solution...

								Honza

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

Re: [syzbot] [ext4?] WARNING in mb_cache_destroy

[Baokun Li]

On 2024/5/3 22:09, Jan Kara wrote:
> On Fri 03-05-24 19:38:21, Baokun Li wrote:
>> On 2024/5/3 18:23, Jan Kara wrote:
>>> Hi!
>>>
>>> On Fri 03-05-24 17:51:07, Baokun Li wrote:
>>>> On 2024/5/2 18:33, Jan Kara wrote:
>>>>> On Tue 30-04-24 08:04:03, syzbot wrote:
>>>>>> syzbot has bisected this issue to:
>>>>>>
>>>>>> commit 67d7d8ad99beccd9fe92d585b87f1760dc9018e3
>>>>>> Author: Baokun Li <libaokun1@huawei.com>
>>>>>> Date:   Thu Jun 16 02:13:56 2022 +0000
>>>>>>
>>>>>>        ext4: fix use-after-free in ext4_xattr_set_entry
>>>>> So I'm not sure the bisect is correct since the change is looking harmless.
>>>> Yes, the root cause of the problem has nothing to do with this patch,
>>>> and please see the detailed analysis below.
>>>>> But it is sufficiently related that there indeed may be some relationship.
>>>>> Anyway, the kernel log has:
>>>>>
>>>>> [   44.932900][ T1063] EXT4-fs warning (device loop0): ext4_evict_inode:297: xattr delete (err -12)
>>>>> [   44.943316][ T1063] EXT4-fs (loop0): unmounting filesystem.
>>>>> [   44.949531][ T1063] ------------[ cut here ]------------
>>>>> [   44.955050][ T1063] WARNING: CPU: 0 PID: 1063 at fs/mbcache.c:409 mb_cache_destroy+0xda/0x110
>>>>>
>>>>> So ext4_xattr_delete_inode() called when removing inode has failed with
>>>>> ENOMEM and later mb_cache_destroy() was eventually complaining about having
>>>>> mbcache entry with increased refcount. So likely some error cleanup path is
>>>>> forgetting to drop mbcache entry reference somewhere but at this point I
>>>>> cannot find where. We'll likely need to play with the reproducer to debug
>>>>> that. Baokun, any chance for looking into this?
>>>>>
>>>>> 								Honza
>>>> As you guessed, when -ENOMEM is returned in ext4_sb_bread(),
>>>> the reference count of ce is not properly released, as follows.
>>>>
>>>> ext4_create
>>>>    __ext4_new_inode
>>>>     security_inode_init_security
>>>>      ext4_initxattrs
>>>>       ext4_xattr_set_handle
>>>>        ext4_xattr_block_find
>>>>        ext4_xattr_block_set
>>>>         ext4_xattr_block_cache_find
>>>>           ce = mb_cache_entry_find_first
>>>>               __entry_find
>>>>               atomic_inc_not_zero(&entry->e_refcnt)
>>>>           bh = ext4_sb_bread(inode->i_sb, ce->e_value, REQ_PRIO);
>>>>           if (PTR_ERR(bh) == -ENOMEM)
>>>>               return NULL;
>>>>
>>>> Before merging into commit 67d7d8ad99be("ext4: fix use-after-free
>>>> in ext4_xattr_set_entry"), it will not return early in
>>>> ext4_xattr_ibody_find(),
>>>> so it tries to find it in iboy, fails the check in xattr_check_inode() and
>>>> returns without executing ext4_xattr_block_find(). Thus it will bisect
>>>> the patch, but actually has nothing to do with it.
>>>>
>>>> ext4_xattr_ibody_get
>>>>    xattr_check_inode
>>>>     __xattr_check_inode
>>>>      check_xattrs
>>>>       if (end - (void *)header < sizeof(*header) + sizeof(u32))
>>>>         "in-inode xattr block too small"
>>>>
>>>> Here's the patch in testing, I'll send it out officially after it is tested.
>>>> (PS:  I'm not sure if propagating the ext4_xattr_block_cache_find() errors
>>>> would be better.)
>>> Great! Thanks for debugging this! Some comments to your fix below:
>>>
>>>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>>>> index b67a176bfcf9..5c9e751915fd 100644
>>>> --- a/fs/ext4/xattr.c
>>>> +++ b/fs/ext4/xattr.c
>>>> @@ -3113,11 +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)
>>>> -                return NULL;
>>>> +            if (PTR_ERR(bh) != -ENOMEM)
>>>> +                EXT4_ERROR_INODE(inode, "block %lu read error",
>>>> +                         (unsigned long)ce->e_value);
>>>>                bh = NULL;
>>>> -            EXT4_ERROR_INODE(inode, "block %lu read error",
>>>> -                     (unsigned long)ce->e_value);
>>>>            } else if (ext4_xattr_cmp(header, BHDR(bh)) == 0) {
>>>>                *pce = ce;
>>>>                return bh;
>>> So if we get the ENOMEM error, continuing the iteration seems to be
>>> pointless as we'll likely get it for the following entries as well. I think
>>> the original behavior of aborting the iteration in case of ENOMEM is
>>> actually better. We just have to do mb_cache_entry_put(ea_block_cache, ce)
>>> before returning...
>>>
>>> 								Honza
>> Returning NULL here would normally attempt to allocate a new
>> xattr_block in ext4_xattr_block_set(), and when ext4_sb_bread() fails,
>> allocating the new block and inserting it would most likely fail as well,
>> so my initial thought was to propagate the error from ext4_sb_bread()
>> to also make ext4_xattr_block_set() fail when ext4_sb_bread() fails.
> Yes, this would be probably even better solution.
Okay.
>
>> But I noticed that before Ted added the special handling for -ENOMEM,
>> EXT4_ERROR_INODE was called to set the ERROR_FS flag no matter
>> what error ext4_sb_bread() returned, and after we can distinguish
>> between -EIO and -ENOMEM, we don't have to set the ERROR_FS flag
>> in the case of -ENOMEM. So there's this conservative fix now.
>>
>> In short, in my personal opinion, for -EIO and -ENOMEM, they should
>> be the same except whether or not the ERROR_FS flag is set.
>> Otherwise, I think adding mb_cache_entry_put() directly is the easiest
>> and most straightforward fix.  Honza, do you have any other thoughts?
> Yeah. I'd go for adding mb_cache_entry_put() now as a quick fix and then
> work on propagating the error from ext4_xattr_block_cache_find() as a
> cleaner solution...
>
> 								Honza
>
Ok, thank you very much for the suggestion!
I'll send the quick fix out right away.

Cheers,
Baokun