Page MenuHomeFreeBSD
Differential D6109

bcache read ahead may attempt to read past disk end
ClosedPublic
Actions

Authored by tsoome on Apr 26 2016, 11:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 6:11 PM
Unknown Object (File)
Fri, Nov 15, 8:05 PM
Unknown Object (File)
Sat, Nov 2, 2:35 AM
Unknown Object (File)
Thu, Oct 31, 8:37 PM
Unknown Object (File)
Sat, Oct 26, 1:25 PM
Unknown Object (File)
Oct 2 2024, 12:31 PM
Unknown Object (File)
Sep 30 2024, 9:06 AM
Unknown Object (File)
Sep 24 2024, 3:23 PM
View All Files
Subscribers
emaste
imp
jhb
juan.molina_club.fr
kevans
ler
rm
View All 9 Subscribers

Details

Reviewers
jhb
np
Commits
rS298900: bcache read ahead may attempt to read past end of disk
Summary

Since disk info is opaque, bcache has no way to know the size of the
disk, and therefore attempt to read past disk end. Also not all disk
backends do provide disk size. Since we really dont want to make
bcache to hack around in disk data, the solution would be to cope
with partial read in bcache and to implement proper bounds check in
backend disk module. This patch does provide such check for biosdisk.c.

Note this patch does not provide full solution as I just do not have
capabilities to test. Also so far only known case is with GPT partition
table, which does implement the backup copy at the last sectors of the disk.

Test Plan

I have tested this patch with specially crafted disk read program, attempting
to read last sectors and past disk end.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3477
Build 3517: arc lint + arc unit

Event Timeline

tsoome updated this revision to Diff 15620.Apr 26 2016, 11:51 AM
tsoome retitled this revision from to bcache read ahead may attempt to read past disk end.
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptApr 26 2016, 11:51 AM
tsoome added a reviewer: np.Apr 26 2016, 1:09 PM
np edited edge metadata.Apr 26 2016, 2:10 PM

I tested this patch and it fixes a long delay between the last of the "BIOS disk <x> is <y>" messages and the appearance of the loader menu.

tsoome added a comment.Apr 26 2016, 2:18 PM
In D6109#129954, @np wrote:

I tested this patch and it fixes a long delay between the last of the "BIOS disk <x> is <y>" messages and the appearance of the loader menu.

thanks!

imp added a comment.Apr 26 2016, 3:19 PM

This looks good to my eye as well, though I haven't tested it.

juan.molina_club.fr added a subscriber: juan.molina_club.fr.Apr 27 2016, 2:00 PM

Typos.

sys/boot/common/bcache.c
279

s/bcahce/bcache

280

s/is/if

tsoome marked 2 inline comments as done.Apr 27 2016, 2:32 PM

typos fixed, thanks.

tsoome updated this revision to Diff 15654.Apr 27 2016, 2:32 PM
tsoome edited edge metadata.

Updated to revision 298697.

jhb added a subscriber: jhb.Apr 27 2016, 3:42 PM

The other option is to add a callback to the devsw to fetch the size (or add an ioctl that returns the size).

Actually, we already have that. DIOCGMEDIASIZE returns the raw size in bytes. It is not yet implemented for bioscd.c or the EFI backends, but that is easily fixable and it is already present in biosdisk.c.

You could treat a backing device where the ioctl fails as having infinite size in the bcache layer. This would be simpler in the long run as the read truncation handling would be in one place (bcache.c) instead of in all the device backends.

tsoome added a comment.Apr 27 2016, 4:07 PM
In D6109#130283, @jhb wrote:

The other option is to add a callback to the devsw to fetch the size (or add an ioctl that returns the size).

Actually, we already have that. DIOCGMEDIASIZE returns the raw size in bytes. It is not yet implemented for bioscd.c or the EFI backends, but that is easily fixable and it is already present in biosdisk.c.

You could treat a backing device where the ioctl fails as having infinite size in the bcache layer. This would be simpler in the long run as the read truncation handling would be in one place (bcache.c) instead of in all the device backends.

I was actually thinking a bit about it, but the problem is, the disk backend really should verify the read anyhow (I'm pretty sure if you look into any strategy implementation in kernel, you will find the checks in place;), also the ioctl() would imply descriptor which we do not have;). So this is the background for approach taken. Still, I'm not entirely happy about the bcache-disk integration, it does make things more complicated as it should be. And for example, if you think of geli, where should it go in this chain to get best results... So I would take baby steps here, play safe against disk backend because you really don't know how some devices will respond and I think there is still a lot of space for future development.

jhb added a comment.Apr 27 2016, 4:23 PM

Ok. Outside of the ioctl variant I think this approach would be my second choice, and if we need to not assume a device we can ioctl on that is fine.

It would be good to fix some other backends as well. For x86 that would mean bioscd.c and efipart.c. I know that the bioscd structure has the 'specification packet' that has the sector count as one of its members.

Actually, efipart.c already has this check except that it does not do a short read but just fails entirely. Should be simple to adjust it to do a short read instead.

rm added a subscriber: rm.Apr 28 2016, 8:25 AM
tsoome added a comment.Apr 28 2016, 12:24 PM
In D6109#130314, @jhb wrote:

Ok. Outside of the ioctl variant I think this approach would be my second choice, and if we need to not assume a device we can ioctl on that is fine.

It would be good to fix some other backends as well. For x86 that would mean bioscd.c and efipart.c. I know that the bioscd structure has the 'specification packet' that has the sector count as one of its members.

Actually, efipart.c already has this check except that it does not do a short read but just fails entirely. Should be simple to adjust it to do a short read instead.

yep, well, efipart fix was quite trivial. however, bioscd is much more complicated, I'm having hard time to find an way to get actual size for media. the packet interface 4B01 does not return geometry on vmware fusion/qemu, therefore also 08 is useless (as its related to packet data anyhow), and 48 does work on those two platforms, but only returns sector size, the geometry data is garbage (FFFF/FFFF/FFFF on qemu and 1FFFFF/0/0 on vmware fusion). I have some ideas to check, but also meanwhile I'm open for suggestions;)

kevans added a subscriber: kevans.Apr 28 2016, 5:09 PM
tsoome updated this revision to Diff 15705.Apr 28 2016, 6:16 PM

Add read check for efipart and bioscd.
Note there appeares to be no reliable interface to get size of the
CD in BIOS, so on read error, we fall back reading sector by sector
and return sort read if we got any data, error otherwise.
Updated to revision 298751.

Herald added a subscriber: rpokala. · View Herald TranscriptApr 28 2016, 6:16 PM
tsoome updated this revision to Diff 15706.Apr 28 2016, 6:24 PM

missed the patch in bc_read()

jhb accepted this revision.Apr 28 2016, 6:51 PM
jhb added a reviewer: jhb.

Ok, the BIOS CD workaround sounds good to me. Thanks.

This revision is now accepted and ready to land.Apr 28 2016, 6:51 PM
emaste added a subscriber: emaste.Apr 30 2016, 7:19 PM
ler added a subscriber: ler.May 1 2016, 2:16 AM

This fixes my slow boot to loader as well. Can we get it committed?

Closed by commit rS298900: bcache read ahead may attempt to read past end of disk (authored by allanjude). · Explain WhyMay 1 2016, 9:07 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
boot/
common/
47 lines
i386/
libi386/
12 lines

Diff 15654

View Options

sys/boot/common/bcache.c

Loading...
View Options

sys/boot/i386/libi386/biosdisk.c

Loading...