Re: [PATCH]: grub: Fix ofdisk disk cache corruption.

[phcoder <phcoder@gmail.com>]

David Miller wrote:
> From: Pavel Roskin <proski@gnu.org>
> Date: Sun, 12 Apr 2009 02:29:15 -0400
> 
>> On Sat, 2009-04-11 at 01:08 -0700, David Miller wrote:
>>> The ieee1275 ofdisk driver doesn't use a unique value for
>>> disk->id so it's really easy to get disk corruption.  I was
>>> able to see such corruption by simply booting grub from one
>>> disk and booting a Linux kernel from another, both of which
>>> were on the same disk controller.
>> I hope you mean disk cache corruption, as in the subject, not disk
>> corruption.  GRUB only writes to disks to save environment variables,
>> and it's done very carefully.  
>>
>>> +#define OFDISK_HASH_SZ	8
>>> +static struct ofdisk_hash_ent *ofdisk_hash[OFDISK_HASH_SZ];
>>> +
>>> +static int
>>> +ofdisk_hash_fn (const char *devpath)
>>> +{
>>> +  int hash = 0;
>>> +  while (*devpath)
>>> +    hash ^= *devpath++;
>>> +  return (hash & (OFDISK_HASH_SZ - 1));
>>> +}
>> That's a 3 bit hash.  The risk of collisions is very high.  I would
>> understand if you had 8 entries for the hash values, but the hash values
>> themselves should be reasonably unique.
> 
> In my testing there weren't many collisions.
> 
This is so called "survivorship bias". If this hash generated a 
collision for you you would already have changed it
> I think fixing disk cache corruption is more important than
> arguing over the distribution properties of the hash function
> I have choosen.
> 
Yes, but weak hash has exactly the same problem, just on other systems
>> If you can save the device names, then there is no point in using
>> hashes.  You can use (long)devpath.
> 
> Sure we need the hash, to find path entries we've saved beforehand.
You can maintain a table of devpathes in cache and use the index in this 
table as id. This way is the safest
> 
>>> +  if (!op)
>>> +    op = ofdisk_hash_add (devpath);
>>>  
>>> -  grub_ieee1275_open (devpath, &dev_ihandle);
>>> +  grub_free (devpath);
>> But if you free the device names, then they are bad IDs.  The
>> probability of the same memory being reused for another name is high.
>>
>> Perhaps I misunderstand something.
> 
> The path we use is dup'd into the hash entries we create, and
> the hash entry path string is the one we use.
> 
> Therefore "devpath" is only needed across the ofdisk_hash_add()
> call.
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko