bcache read ahead may attempt to read past disk end

[tsoome]

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.

[np]

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]

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]

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

[juan.molina_club.fr]

Typos.

sys/boot/common/bcache.c

279

s/bcahce/bcache

280

s/is/if

[tsoome]

typos fixed, thanks.

[tsoome]

Updated to revision 298697.

[jhb]

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]

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]

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.

[tsoome]

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;)

[tsoome]

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.

[tsoome]

missed the patch in bc_read()

[jhb]

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

[ler]

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