[SLOF] [PATCH v4] slof/fs/packages/disk-label.fs: improve checking for DOS boot partitions

Fri Apr 5 19:16:44 AEDT 2024

Hi,

On 2024-04-05 14:46:14, Alexey Kardashevskiy wrote:
> 
> 
> On Thu, 4 Apr 2024, at 18:18, Kautuk Consul wrote:
> > On 2024-04-04 11:35:43, Alexey Kardashevskiy wrote:
> > > First, sorry I am late into the discussion. Comments below.
> > > 
> > > 
> > > On Thu, 28 Mar 2024, at 17:00, Kautuk Consul wrote:
> > > > While testing with a qcow2 with a DOS boot partition it was found that
> > > > when we set the logical_block_size in the guest XML to >512 then the
> > > > boot would fail in the following interminable loop:
> > > 
> > > Why would anyone tweak this? And when you do, what happens inside the SLOF, does it keep using 512?
> > Well, we had an image with DOS boot partition and we tested it with
> > logical_block_size = 1024 and got this infinite loop.
> 
> This does not really answer to "why" ;)

Well, my point is that it is tweakable in virsh/qemu and maybe we should be
handling this error in configuration in SLOF properly ? There shouldn't
be the possibility of an interminable loop in the software anywhere,
right ? :-)

> 
> > In SLOF the block-size becomes what we configure in the
> > logical_block_size parameter. This same issue doesn't arise with GPT.
> 
> How is GPT different in this regard?

In GPT the sector number 1 contains the GPT headers, not
sector 0 as in the case of DOS boot partition. So if the block-size is
incorrect, the GPT magic number itself isn't found and the "E3404: Not a
bootable device!" error is immediately thrown.

> 
> > > > <SNIP>
> > > > Trying to load:  from: /pci at 800000020000000/scsi at 3 ...
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > </SNIP>
> > > > 
> > > > Change the "read-sector" Forth subroutine to throw an exception whenever
> > > > it fails to read a full block-size length of sector from the disk.
> > > 
> > > Why not throwing an exception from the "beyond end" message point?
> > > Or fail to open a device if SLOF does not like the block size? I forgot the internals :(
> > This loop is interminable and this "Access beyond end of device!"
> > message continues forever.
> 
> Where is that loop exactly? Put CATCH in there.

That loop is in count-dos-logical-partitions. The reason why I didn't
put a CATCH in there is because there is already another CATCH statement
in do-load in slof/fs/boot.fs which covers this throw. For the other
path that doesn't have a CATCH I have inserted a CATCH in the open
subroutine in disk-label.fs.

> 
> > SLOF doesn't have any option other than to use the block-size that was
> > set in the logical_block_size parameter. It doesn't have any preference
> > as the code is very generic for both DOS as well as GPT.
> > > 
> > > > Also change the "open" method to initiate CATCH exception handling for the calls to
> > > > try-partitions/try-files which will also call read-sector which could potentially
> > > > now throw this new exception.
> > > > 
> > > > After making the above changes, it fails properly with the correct error
> > > > message as follows:
> > > > <SNIP>
> > > > Trying to load:  from: /pci at 800000020000000/scsi at 3 ...
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > virtioblk_transfer: Access beyond end of device!
> > > > 
> > > > E3404: Not a bootable device!
> > > > 
> > > > E3407: Load failed
> > > > 
> > > >   Type 'boot' and press return to continue booting the system.
> > > >   Type 'reset-all' and press return to reboot the system.
> > > > 
> > > > Ready!
> > > > 0 >
> > > > </SNIP>
> > > > 
> > > > Signed-off-by: Kautuk Consul <kconsul at linux.ibm.com>
> > > > ---
> > > > slof/fs/packages/disk-label.fs | 12 +++++++++---
> > > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
> > > > index 661c6b0..a6fb231 100644
> > > > --- a/slof/fs/packages/disk-label.fs
> > > > +++ b/slof/fs/packages/disk-label.fs
> > > > @@ -136,7 +136,8 @@ CONSTANT /gpt-part-entry
> > > > : read-sector ( sector-number -- )
> > > >     \ block-size is 0x200 on disks, 0x800 on cdrom drives
> > > >     block-size * 0 seek drop      \ seek to sector
> > > > -   block block-size read drop    \ read sector
> > > > +   block block-size read         \ read sector
> > > > +   block-size < IF throw THEN    \ if we read less than the block-size then throw an exception
> > > 
> > > When it fails, is the number of bytes ever non zero? Thanks,
> > No, it doesn't reach 0. It is lesser than the block-size. For example if
> > we set the logcial_block_size to 1024, the block-size is that much. if
> > we are reading the last sector which is physically only 512 bytes long
> > then we read that 512 bytes which is lesser than 1024, which should be
> > regarded as an error.
> 
> Ah so it only happens when there is an odd number of 512 sectors so reading the last one with block-size==1024 only reads a half => failure, is that right?

Yes. Or, block-size if set to 2048 or 4096 will also show the same problem.

> 
> > > 
> > > > ;
> > > >  
> > > > : (.part-entry) ( part-entry )
> > > > @@ -723,10 +724,15 @@ CREATE GPT-LINUX-PARTITION 10 allot
> > > >     THEN
> > > >  
> > > >     partition IF
> > > > -       try-partitions
> > > > +       ['] try-partitions
> > > >     ELSE
> > > > -       try-files
> > > > +       ['] try-files
> > > >     THEN
> > > > +
> > > > +   \ Catch any exception that might happen due to read-sector failing to read
> > > > +   \ block-size number of bytes from any sector of the disk.
> > > > +   CATCH IF false THEN
> > Segher/Alexey, can we keep this CATCH block or should I remove it ?
> 
> imho the original bug should be handled more gracefully. Seeing exceptions in the code just triggers exceptions in my small brain :) Thanks,

:-). But this is the only other path that doesn't have a CATCH
like the do-load subroutine in slof/fs/boot.fs. According to Segher
there shouldn't ever be a problem with throw because if nothing else the
outer-most interpreter loop's CATCH will catch the exception. But I
thought to cover this throw in read-sector more locally in places near
to this functionality. Because the outermost FORTH SLOF interpreter loop is not
really so related to reading a sector in disk-label.fs.

> 
> > > > +
> > > >     dup 0= IF debug-disk-label? IF ." not found." cr THEN close THEN \ free memory again
> > > > ;
> > > >  
> > > > -- 
> > > > 2.31.1
> > > > 
> > > > 
> > 