On Thu, Dec 22, 2022 at 12:41:58PM -0500, Theodore Ts'o wrote:
> On Thu, Dec 22, 2022 at 10:02:44AM +0800, Jun Nie wrote:
> > There is case that s_first_data_block is not 0 and block nr is smaller than
> > s_first_data_block when calculating group bitmap during allocation. This
> > underflow make index exceed es->s_groups_count in ext4_get_group_info()
> > and trigger the BUG_ON.
> >
> > Fix it with protection of underflow.
>
> When was this happening, and why? If blocknr is less than
> s_first_data_block, this is either a insufficient input validation,
> insufficient validation to detection file system corruption. or some
> other kernel bug.
>
> Looking quickly at the code and the repro, it appears that issue is
> that FS_IOC_GETFSMAP is getting passed a stating physical block of 0
> in fmh_keys[0] when on a file system with a blocksize of 1k (in which
> case s_first_data_block is 1). It's unclear to me what
Question -- on a 1k-block filesystem, are the first 1024 bytes of the
device *reserved* by ext4 for whatever bootloader crud goes in there?
Or is that space undefined in the filesystem specification?
I never did figure that out when I was writing the ondisk specification
that's in the kernel, but maybe you remember?
> FS_IOC_GETFSMAP should *do* when passed a value which requests that it
> provide a mapping for a block which is out of bounds (either too big,
> or too small)?. Should it return an error? Should it simply not
> return a mapping? The map page for ioctl_getfsmap() doesn't shed any
> light on this question.
>
> Darrick, you designed the interface and wrote most of fs/ext4/fsmap.c.
> Can you let us know what is supposed to happen in this case? Many
> thanks!!
If those first 1024 bytes are defined to be reserved in the ondisk
format, then you could return a mapping for those bytes with the owner
code set to EXT4_FMR_OWN_UNKNOWN.
If, however, the space is undefined, then going off this statement in
the manpage:
"For example, if the low key (fsmap_head.fmh_keys[0]) is set to (8:0,
36864, 0, 0, 0), the filesystem will only return records for extents
starting at or above 36 KiB on disk."
I think the 'at or above' clause means that ext4 should not pass back
any mapping for the byte range 0-1023 on a 1k-block filesystem.
If the low key is set to (8:0, 0, 0, 0, 0) and high key is set to (8:0,
1023, 0, 0, 0) then ext4 shouldn't return any mapping at all, because
there's no space usage defined for that region of the disk.
If the low key is set to (8:0, 0, 0, 0, 0) and high key is set to all
ones, then ext4 can return mappings for the primary superblock at offset
1024.
--D
>
> > Fixes: 72b64b594081ef ("ext4 uninline ext4_get_group_no_and_offset()")
>
> This makes ***no*** sense; the commit in question is from 2006, which
> means that in some jourisdictions it's old enough to drive a car. :-)
> Futhermore, all it does is move the function from an inline function
> to a C file (in this case, balloc.c). It also long predates
> introduction of FS_IOC_GETFSMAP support, which was in 2017.
>
> I'm guessing you just did a "git blame" and blindly assumed that
> whatever commit last touched the C code in question was what
> introduced the problem?
>
> Anyway, please try to understand what is going on instead of doing the
> moral equivalent of taking a sledgehammer to the code until the
> reproducer stops triggering a BUG. It's not enough to shut up the
> reproducer; you should understand what is happening, and why, and then
> strive to find the best fix to the problem. Papering over problems in
> the end will result in more fragile code, and the goal of syzkaller is
> to improve kernel quality. But syzkaller is just a tool and used
> wrongly, it can have the opposite effect.
>
> Regards,
>
> - Ted
