grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Miller
Subject: Re: [PATCH]: grub: Fix ofdisk disk cache corruption.
Date: Sun, 12 Apr 2009 01:01:21 -0700 (PDT)

From: Pavel Roskin <address@hidden>
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.

I think fixing disk cache corruption is more important than
arguing over the distribution properties of the hash function
I have choosen.

Right?

> 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.

> 
>> +  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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]