Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap

From: Andreas Dilger <adilger@dilger.ca>
To: Carlos Maiolino <cmaiolino@redhat.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
	Christoph Hellwig <hch@lst.de>, Jaegeuk Kim <jaegeuk@kernel.org>,
	Miklos Szeredi <miklos@szeredi.hu>,
	rpeterso@redhat.com, linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap
Date: Thu, 8 Aug 2019 12:53:25 -0600	[thread overview]
Message-ID: <69E22C32-5EDC-4507-9407-A1622BC31560@dilger.ca> (raw)
In-Reply-To: <20190808071257.ufbk5i35xpkf4byh@pegasus.maiolino.io>

[-- Attachment #1: Type: text/plain, Size: 2847 bytes --]

On Aug 8, 2019, at 1:12 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote:
> 
>>> 
>>>> Maybe I am not seeing something or having a different thinking you have, but
>>>> this is the behavior we have now, without my patches. And we can't really change
>>>> it; the user view of this implementation.
>>>> That's why I didn't try to change the result, so the truncation still happens.
>>> 
>>> I understand that we're not generally supposed to change existing
>>> userspace interfaces, but the fact remains that allowing truncated
>>> responses causes *filesystem corruption*.
>>> 
>>> We know that the most well known FIBMAP callers are bootloaders, and we
>>> know what they do with the information they get -- they use it to record
>>> the block map of boot files.  So if the IPL/grub/whatever installer
>>> queries the boot file and the boot file is at block 12345678901 (a
>>> 34-bit number), this interface truncates that to 3755744309 (a 32-bit
>>> number) and that's where the bootloader will think its boot files are.
>>> The installation succeeds, the user reboots and *kaboom* the system no
>>> longer boots because the contents of block 3755744309 is not a bootloader.
>>> 
>>> Worse yet, grub1 used FIBMAP data to record the location of the grub
>>> environment file and installed itself between the MBR and the start of
>>> partition 1.  If the environment file is at offset 1234578901, grub will
>>> write status data to its environment file (which it thinks is at
>>> 3755744309) and *KABOOM* we've just destroyed whatever was in that
>>> block.
>>> 
>>> Far better for the bootloader installation script to hit an error and
>>> force the admin to deal with the situation than for the system to become
>>> unbootable.  That's *why* the (newer) iomap bmap implementation does not
>>> return truncated mappings, even though the classic implementation does.
>>> 
>>> The classic code returning truncated results is a broken behavior.
>> 
>> How long as it been broken for? And if we do fix it, I'd just like for
>> a nice commit lot describing potential risks of not applying it. *If*
>> the issue exists as-is today, the above contains a lot of information
>> for addressing potential issues, even if theoretical.
>> 
> 
> It's broken since forever. This has always been the FIBMAP behavior.

It's been broken since forever, but only for filesystems larger than 4TB or
16TB (2^32 blocks), which are only becoming commonplace for root disks recently.
Also, doesn't LILO have a limit on the location of the kernel image, in the
first 1GB or similar?

So maybe this is not an issue that FIBMAP users ever hit in practise anyway,
but I agree that it doesn't make sense to return bad data (32-bit wrapped block
numbers) and 0 should be returned in such cases.

Cheers, Andreas

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]