[syzbot] [xfs?] WARNING in xfs_bmap_extents_to_btree

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 1e760fa3596e Merge tag 'gfs2-v6.3-rc3-fix' of git://git.ke..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16f83651c80000
kernel config: https://syzkaller.appspot.com/x/.config?x=acdb62bf488a8fe5
dashboard link: https://syzkaller.appspot.com/bug?extid=0c383e46e9b4827b01b1
compiler: Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/17229b6e6fe0/disk-1e760fa3.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/69b5d310fba0/vmlinux-1e760fa3.xz
kernel image: https://storage.googleapis.com/syzbot-assets/0c65624aace9/bzImage-1e760fa3.xz

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

------------[ cut here ]------------
WARNING: CPU: 1 PID: 24101 at fs/xfs/libxfs/xfs_bmap.c:660 xfs_bmap_extents_to_btree+0xe1b/0x1190
Modules linked in:
CPU: 1 PID: 24101 Comm: kworker/1:24 Not tainted 6.3.0-rc3-syzkaller-00031-g1e760fa3596e #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/02/2023
Workqueue: xfs-conv/loop0 xfs_end_io
RIP: 0010:xfs_bmap_extents_to_btree+0xe1b/0x1190 fs/xfs/libxfs/xfs_bmap.c:660
Code: 01 00 00 44 0f 44 f0 48 8b 84 24 88 00 00 00 42 0f b6 04 28 84 c0 0f 85 91 02 00 00 45 89 34 24 e9 10 fb ff ff e8 f5 81 75 fe <0f> 0b 41 bf e4 ff ff ff 48 8b 5c 24 18 e9 bd fa ff ff 89 d9 80 e1
RSP: 0018:ffffc9000346efe0 EFLAGS: 00010293
RAX: ffffffff8314eb2b RBX: ffffffffffffffff RCX: ffff8880338657c0
RDX: 0000000000000000 RSI: ffffffffffffffff RDI: ffffffffffffffff
RBP: ffffc9000346f270 R08: ffffffff8314e358 R09: fffffbfff1ca6eae
R10: 0000000000000000 R11: dffffc0000000001 R12: 1ffff110038bc80f
R13: dffffc0000000000 R14: 0000000000000000 R15: ffff88801c5e4000
FS: 0000000000000000(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000558b6f323000 CR3: 0000000042288000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
xfs_bmap_add_extent_unwritten_real+0x1eec/0x31f0 fs/xfs/libxfs/xfs_bmap.c:2426
xfs_bmapi_convert_unwritten+0x505/0x6e0 fs/xfs/libxfs/xfs_bmap.c:4191
xfs_bmapi_write+0xb55/0x1980 fs/xfs/libxfs/xfs_bmap.c:4418
xfs_iomap_write_unwritten+0x45f/0xc40 fs/xfs/xfs_iomap.c:615
xfs_end_ioend+0x232/0x4d0 fs/xfs/xfs_aops.c:131
xfs_end_io+0x2e5/0x370 fs/xfs/xfs_aops.c:173
process_one_work+0x8a0/0x10e0 kernel/workqueue.c:2390
worker_thread+0xa63/0x1210 kernel/workqueue.c:2537
kthread+0x270/0x300 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
</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 syzk...@googlegroups.com.

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

[Dave Chinner]

On Tue, Mar 28, 2023 at 09:08:01PM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 1e760fa3596e Merge tag 'gfs2-v6.3-rc3-fix' of git://git.ke..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=16f83651c80000
> kernel config: https://syzkaller.appspot.com/x/.config?x=acdb62bf488a8fe5
> dashboard link: https://syzkaller.appspot.com/bug?extid=0c383e46e9b4827b01b1
> compiler: Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/17229b6e6fe0/disk-1e760fa3.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/69b5d310fba0/vmlinux-1e760fa3.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/0c65624aace9/bzImage-1e760fa3.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+0c383e...@syzkaller.appspotmail.com
>
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 24101 at fs/xfs/libxfs/xfs_bmap.c:660 xfs_bmap_extents_to_btree+0xe1b/0x1190

Allocation got an unexpected ENOSPC when it was supposed to have a
valid reservation for the space. Likely because of an inconsistency
that had been induced into the filesystem where superblock space
accounting doesn't exactly match the AG space accounting and/or the
tracked free space.

Given this is a maliciously corrupted filesystem image, this sort of
warning is expected and there's probably nothing we can do to avoid
it short of a full filesystem verification pass during mount.
That's not a viable solution, so I think we should just ignore
syzbot when it generates this sort of warning....

i.e. we actually want this warning to be issued if it happens in
normal production situations, but given that it's relatively trivial
to create an inconsistent filesystem image that can trigger this we
should just ignore it when it is generated by such means.

-Dave.
--
Dave Chinner
da...@fromorbit.com

[Aleksandr Nogikh]

If it's not a warning about a kernel bug, then WARN_ON should probably
be replaced by some more suitable reporting mechanism. Kernel coding
style document explicitly says:

"WARN*() must not be used for a condition that is expected to trigger
easily, for example, by user space actions. pr_warn_once() is a
possible alternative, if you need to notify the user of a problem."
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=1e760fa3596e8c7f08412712c168288b79670d78#n1223

--
Aleksandr

[Theodore Ts'o]

On Thu, Mar 30, 2023 at 10:52:37AM +0200, Aleksandr Nogikh wrote:
> > Given this is a maliciously corrupted filesystem image, this sort of
> > warning is expected and there's probably nothing we can do to avoid
> > it short of a full filesystem verification pass during mount.
> > That's not a viable solution, so I think we should just ignore
> > syzbot when it generates this sort of warning....
>
> If it's not a warning about a kernel bug, then WARN_ON should probably
> be replaced by some more suitable reporting mechanism. Kernel coding
> style document explicitly says:
>
> "WARN*() must not be used for a condition that is expected to trigger
> easily, for example, by user space actions. pr_warn_once() is a
> possible alternative, if you need to notify the user of a problem."
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=1e760fa3596e8c7f08412712c168288b79670d78#n1223
>

Well, the question is wether a maliciously corrupted file system is a
condition which is "triggered easily". Note that it requries root
privileges to be able to mount a malciously corrupted file system,
and given that root can do all sorts of thigns that can crash the
system (example: kexec a maliciously created "kernel image" or
creating a high-priority real-time thread which starves kernel
threads), this is actually a much closer call.

- Ted

[Dave Chinner]

On Thu, Mar 30, 2023 at 10:52:37AM +0200, Aleksandr Nogikh wrote:

That's exactly the case here. It should *never* happen in normal
production workloads, and it if does then we have the *potential*
for silent data loss occurring. That's *exactly* the sort of thing
we should be warning admins about in no uncertain terms. Also, we
use WARN_ON_ONCE(), so it's not going to spam the logs.

syzbot is a malicious program - it is injecting broken stuff into
the kernel as root to try to trigger situations like this. That
doesn't make a warning it triggers bad or incorrect - syzbot is
pertubing tightly coupled structures in a way that makes the
information shared across those structures inconsistent and
eventually the code is going to trip over that inconsistency.

IOWs, once someone has used root permissions to mount a maliciously
crafted filesystem image, *all bets are off*. The machine is running
a potentially compromised kernel at this point. Hence it is almost
guaranteed that at some point the kernel is going to discover things
are *badly wrong* and start dumping "this should never happen!"
warnings into the logs. That's what the warnings are supposed to do,
and the fact that syzbot can trigger them doesn't make the warnings
wrong.

> pr_warn_once() is a
> possible alternative, if you need to notify the user of a problem."
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=1e760fa3596e8c7f08412712c168288b79670d78#n1223

It is worth remembering that those are guidelines, not enforcable
rules and any experienced kernel developer will tell you the same
thing. We know the guidelines, we know when to apply them, we know
there are cases that the guidelines simply can't, don't or won't
cover.

[Darrick J. Wong]

...and perhaps the WARNs that can result from corrupted metadata should
be changed to XFS_IS_CORRUPT() ?

We still get a kernel log about something going wrong, only now the
report doesn't trigger everyone's WARN triggers, and we tell the user to
go run xfs_repair.

--D

[Dave Chinner]

Well, I think in the case it isn't -corrupt- metadata, more the case
that there is an inconsistency between different structures that are
internally consistent.

e.g. remove a free space extent from the freespace tree without
removing the space from the global free space counters. Now
delalloc reservation is allowed by the global counters, but when we
got to allocate the extent - or the bmap btree block to index it -
we fail the allocation because the free space btrees are empty.

The allocation structures are not internally inconsistent or
corrupt, so it's done the right thing by returning ENOSPC. The
global counters are not obviously inconsistent or corrupt, either.
So it can be triggered by just the right sort of corruption at
exactly the right time (i.e at 100% ENOSPC), but the chances of this
convoluted set of circumstances happening in production systems is
pretty much infintesimal.

> We still get a kernel log about something going wrong, only now the
> report doesn't trigger everyone's WARN triggers, and we tell the user to
> go run xfs_repair.

I think that is exactly the wrong thing to do.

We have a history of this WARN firing as a result of software bugs
in XFS - typically a transaction space reservation or allocation
parameter setup issue - in which case a WARN_ON_ONCE is more
appropriate here than declaring the filesystem corrupt.

That's the bottom line - this specific WARN has been placed because
it is an indicator of a bug in the code, not because it is something
that occurs because of filesystem corruption. The WARN is an
indicator that the bug needs to be reported, not simply put back on
the user to clean up the mess and continue on blissfully unaware
that they tripped over a kernel bug rather than some nebulous,
unexplainable corruption.

syzbot being able to trip over it by corrupting the fs in just the
right way doesn't mean we should change it - syzbot is a malicious
attacker, not a production workload, and I really don't think we
should be changing warnings that we actually want users to report
just to shut up syzbot.

[Hillf Danton]

On 1 Apr 2023 07:46:27 +1100 Dave Chinner <da...@fromorbit.com>

>
> syzbot being able to trip over it by corrupting the fs in just the
> right way doesn't mean we should change it - syzbot is a malicious
> attacker, not a production workload, and I really don't think we
> should be changing warnings that we actually want users to report
> just to shut up syzbot.

If it could be tagged so without a skull hoofed, I have difficult
time understanding why the Intel people use such a malicious tool[1].
Are they unable to afford/develop non-malicious ones? Did they send
report with a skull hoofed on their shoulders, Dave?

[1] https://lore.kernel.org/lkml/ZA%2FYz6xv...@xpf.sh.intel.com/

[syzbot]

Auto-closing this bug as obsolete.
Crashes did not happen for a while, no reproducer and no activity.