7599 loader biosdisk fix for 2+TB disks.

Review Request #272 — Created Nov. 20, 2016 and submitted

tsoome
illumos-gate
7599
271
88a9bab...
general

7599 loader biosdisk fix for 2+TB disks.

This fix is implementing partition based boundary check for
disk IO and updates disk mediasize (if needed), based on information
from partition table.

As it appeared, the signed int based approach still has corner cases,
and the wrapover based behavior is non-standard.

The idea for this fix is based on two assumptions:

The bug about media size is hitting large (2+TB) disks, lesser disks
hopefully, are not affected.

Large disks are using GPT (which does include information about disk size).
Since our concern is about boot support and boot disks are partitioned,
implementing partition boundaries based IO verification should make the
media size issues mostly disappear.

However, for large disk case, we do have the disk size available from GPT table.
If non-GPT cases will appear, we still can make approximate calculation about
disk size based on defined partition(s), however, this is not the objective
of this patch, and can be added later if there is any need.

This patch does implement disk media size adjustment (if needed) in bd_open(),
and boundary check in bd_realstrategy().



  • 0
  • 0
  • 11
  • 1
  • 12
Description From Last Updated
tsoome
rm
  1. 
      
  2. Since we're worried about buggy values, should we be worried about the potential for the multiplication to overflow?
    1. I am really not sure there; for one, so far it seems to be no issue; if the sectors count is wrong, its rather smaller than larger. Also, I do not have good idea what to do if we overflow - should we error out or something else. As it has been not an issue so far, perhaps we should just let it to be as is and deal when there is actual case - afterall, there is pretty large user base at fbsd side.

    2. Well, if it's happening we may never notice it. I'm not sure it the multiplication overflow is defined behavior, so it's going to end up rather compiler and processor specific in terms of what happens. So I'm not sure really just saying let's just hope it never happens is the right call.
    3. I think, we should be quite good now:)

  3. Should this also be an unsigned value? So that way you have defined wrap-around semantics for your subtraction later on?
    1. I did drop the wrap-around code as we are mostly covered by partition end check and for large disk case, we get disk size fixed from GPT partition.

      Since the value of the size argument can not be very large, the corresponding blks value is definitely smaller and we never get to the point where we should have unsigned value.

  4. Should we really be using an int for the target of a uint64_t division? What guarantees us that blks will fit this value and not result in truncation?
    1. Implemented the size > INT_MAX check at the beginning of the function. Since our heap is 64MB, the INT_MAX is also already blowing things up, but it does make sure we will fit into 32bit for sure.

    2. Sure, though there's never a reason that this should be a negative value. However, given the other kinds of checks we're doing this should be an unsigned value so we have defined wraparound behavior from the compiler.
  5. Is d_offset tracking in sector or bytes?
    1. it is tracking sector, it is partition start sector set for us by disk_open() as result of processing partition table(s).

  6. Same question on overflow.
    1. This one is also coveres by early check about the value of the variable "size".

  7. 
      
tsoome
rm
  1. 
      
  2. It's probably worth commenting that this is also being used to guarantee that we don't overflow.

  3. Is it worth printing something out to the user in error in this case so we can better deal with it or know that this happened?
    1. I think, the only safe case would be debug print, as otherwise we can just flood the console.

  4. Looking closer, I'm not sure that the logic here is quite right. If we end up in the case where the ioctl fails (an explicit comparison above would be nice), then we're assigning something which is supposed to be a length in bytes, to a length in sectors.
    
    This means when we get to the next line we'll divide by the sector size, trying to get sectors. I think this makes sense in the case where the ioctl succeeds, but when it fails, isn't it wrong to divide by sector size?
  5. I presume elsewhere we've verified that d_offset + disk_blocks cannot overflow at this point? Can you point me to that?
    1. Yep, the d_offset is set on disk_open() based on partition table entry, so it is either 0 or non zero. ioctl() will return us either disk size (d_offset is 0), or partition size.

  6. We probably need to validate that dblk + blks >= dblk to make sure that we don't have an unsigned wraparound and check that before we get here.
  7. 
      
tsoome
rm
  1. I think this has cleaned up most things, but there are still a few outstanding questions I have from previous reviews.

  2. Don't we need to check for overflow? While we added stuff to check disk_open() the bit there is before disk_open().
    1. I did update bd_propbe() to check size received via EDD, so we are safe now.

  3. 
      
tsoome
rm
  1. 
      
  2. How about 'The sector size must be a multiple of 512 bytes.'
  3. How about: 'During bd_probe() we tested if the mulitplication of bd_sectors would overflow so it should be safe to perform here.'
  4. 
      
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...